Implementing timeout support for XHR

Implementing timeout support for XHR

Adding timeout support to DocumentThreadableLoader. Implementation in higher level of Blink's
networking stack in order to allow ResourceHandle abstraction to be reduced to WebUrlLoader after Darin's feedback
in https://codereview.chromium.org/14246006/#msg15
Remove WebKit's legacy #ifdef ENABLE_XHR_TIMEOUT and enable the feature by default.
Late timeout changes/resets after XHR is sent are not implemented at this point, will file comment on spec.

BUG=231959
TEST=Unskipping 11 LayoutTests in http/tests/xmlhttprequest/timeout
R=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152531

[darin (slow to review), 2013-04-16 18:50:52]

Note:  This doesn't handle the synchronous XHR case.  You probably want to move
this all the way into the browser process.  Hacking up support for synchronous
XHR timeouts in the renderer means it'll be hard to cancel the actual network
request.

[darin (slow to review), 2013-04-16 18:59:33]

Nevermind.  I see now that the spec does not support the synchronous XHR case
when the script context is a document.  (Synchronous XHR from a worker gets
translated to an asynchronous request on the main thread.)

[darin (slow to review), 2013-04-16 19:03:45]

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
File Source/WebCore/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
Source/WebCore/platform/network/chromium/ResourceHandle.cpp:188: static const
int timeoutError = -1001;
Hmm, the network layer can also generate net::ERR_TIMED_OUT independently of
this code.  It looks like that would not be seen by XMLHttpRequest as a timeout.
 I assume that's intentional?

[Dominik Röttsches, 2013-04-17 08:26:52]

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
File Source/WebCore/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
Source/WebCore/platform/network/chromium/ResourceHandle.cpp:188: static const
int timeoutError = -1001;
On 2013/04/16 19:03:45, darin wrote:
> Hmm, the network layer can also generate net::ERR_TIMED_OUT independently of
> this code.  It looks like that would not be seen by XMLHttpRequest as a
timeout.
>  I assume that's intentional?

Yes, the way I read the spec, the ontimeout event should only fire in response
to an "author specified timeout", not due to implementation dependent network
timeouts. The latter would be handled by the more general onerror case.

[jamesr, 2013-04-17 17:51:05]

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
File Source/WebCore/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
Source/WebCore/platform/network/chromium/ResourceHandle.cpp:3: * Copyright (C)
2013 Intel Corporation. All rights reserved.
are we doing this?

[d.roettsches, 2013-04-17 20:08:45]

On 2013/04/17 17:51:05, jamesr wrote:
>
https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
> File Source/WebCore/platform/network/chromium/ResourceHandle.cpp (right):
> 
>
https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
> Source/WebCore/platform/network/chromium/ResourceHandle.cpp:3: * Copyright (C)
> 2013 Intel Corporation. All rights reserved.
> are we doing this?

Sorry, this came from the original WebKit patch - what's the rule here, should I
remove this line?

[abarth-chromium, 2013-04-18 05:34:09]

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
File Source/WebCore/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/1/Source/WebCore/platform/netwo...
Source/WebCore/platform/network/chromium/ResourceHandle.cpp:3: * Copyright (C)
2013 Intel Corporation. All rights reserved.
On 2013/04/17 17:51:05, jamesr wrote:
> are we doing this?

In general, yes.

[Dominik Röttsches, 2013-04-18 11:04:46]

Darin, Adam, James, I rebased the patch. Do you think we can move forward with
this? Or is there something you would like me to address? Thanks.

[abarth-chromium, 2013-04-19 06:54:39]

Looks reasonable.  Some detailed feedback below.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
File Source/core/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:186: {
Usually we add an ASSERT_UNUSED that the timer parameter to this function
matches the timer member variable that we expect.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:188: static const int
timeoutError = -1001;
Do we have this constant elsewhere in the project?  It seems odd to hard-code
this value.  Is it an artifact of Blink or of the spec?

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:189: static const char*
const errorDomain = "WebKitNetworkError";
Is this constant defined somewhere else?  It seems odd to have it here as a
literal.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:192:
m_loader->cancel();
Does this object have a cancel method that does this work?  It seems like we
should call a function to do this work rather than replicating this detailed
work here.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
File Source/core/platform/network/chromium/ResourceHandleInternal.h (right):

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandleInternal.h:89: void
timerFired(Timer<ResourceHandleInternal>*);
Presumably this function should be private.

[Dominik Röttsches, 2013-04-19 13:45:52]

Thanks for the thorough review, Adam.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
File Source/core/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:62: , m_timer(this,
&ResourceHandleInternal::timerFired)
I renamed this to m_timeoutTimer to be hopefully a bit more descriptive.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:186: {
On 2013/04/19 06:54:39, abarth wrote:
> Usually we add an ASSERT_UNUSED that the timer parameter to this function
> matches the timer member variable that we expect.

Thanks, added.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:188: static const int
timeoutError = -1001;
On 2013/04/19 06:54:39, abarth wrote:
> Do we have this constant elsewhere in the project?  It seems odd to hard-code
> this value.  Is it an artifact of Blink or of the spec?

It's not from the spec. It was rather a WebKit artifact, using this value from
CFNetwork. Removed. An empty-initialized ResourceError + setIsTimeout(true) is
enough here.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:189: static const char*
const errorDomain = "WebKitNetworkError";
On 2013/04/19 06:54:39, abarth wrote:
> Is this constant defined somewhere else?  It seems odd to have it here as a
> literal.

AFAIR, it was also used by the CFNetwork backend and used here for consistency.
Since WebCore doesn't actually care about these values, I removed it here.

We could use the values that base/net defines as kErrorDomain = "net" and
net::ERR_TIMED_OUT (-7) - but we don't need to emulate that here.

https://codereview.chromium.org/14246006/diff/9001/Source/core/platform/netwo...
Source/core/platform/network/chromium/ResourceHandle.cpp:192:
m_loader->cancel();
On 2013/04/19 06:54:39, abarth wrote:
> Does this object have a cancel method that does this work?  It seems like we
> should call a function to do this work rather than replicating this detailed
> work here.

It does have a cancel() method. However, if we call it before sending the
timeout callback, it sends a different error, then sets m_client = 0, and we
can't fire the right callback. Calling it afterwards crashes, because it seems
our own instance of ResourceHandleInternal gets immediately deleted after the
timeout callback. So - trying to squeeze in between WebURLLoader and WebCore - I
don't see a good way to avoid this kind of code duplication here.

[Dominik Röttsches, 2013-04-19 13:49:14]

New version uploaded, review comments addressed.

[abarth-chromium, 2013-04-19 16:29:48]

https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
File Source/core/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
Source/core/platform/network/chromium/ResourceHandle.cpp:195: }
I'm still concerned that this function copy/pastes code from
ResourceHandleInternal::cancel().  For example, do we need to set the m_client
to 0 after this call?  ResourceHandleInternal::cancel() does that...

Maybe we should add an optional ResourceError parameter to
ResourceHandleInternal::cancel.  If the error parameter is present, then cancel
can call m_client->didFail before clearing the m_client pointer.

https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
File Source/core/platform/network/chromium/ResourceHandleInternal.h (right):

https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
Source/core/platform/network/chromium/ResourceHandleInternal.h:89: void
timerFired(Timer<ResourceHandleInternal>*);
Looks like you missed my comment about making this function private.

[d.roettsches, 2013-04-19 21:20:15]

On 2013/04/19 16:29:48, abarth wrote:
>
https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
> File Source/core/platform/network/chromium/ResourceHandle.cpp (right):
> 
>
https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
> Source/core/platform/network/chromium/ResourceHandle.cpp:195: }
> I'm still concerned that this function copy/pastes code from
> ResourceHandleInternal::cancel().  For example, do we need to set the m_client
> to 0 after this call?  ResourceHandleInternal::cancel() does that...
> 
> Maybe we should add an optional ResourceError parameter to
> ResourceHandleInternal::cancel.  If the error parameter is present, then
cancel
> can call m_client->didFail before clearing the m_client pointer.

Agree, that's better. Updated the patch locally to implement this approach.
However, I get corrupted buffer check crashes, some with "memory written to
after freed" if I keep the m_client = 0 in the cancel() function. Will
investigate why.

> 
>
https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
> File Source/core/platform/network/chromium/ResourceHandleInternal.h (right):
> 
>
https://codereview.chromium.org/14246006/diff/15001/Source/core/platform/netw...
> Source/core/platform/network/chromium/ResourceHandleInternal.h:89: void
> timerFired(Timer<ResourceHandleInternal>*);
> Looks like you missed my comment about making this function private.

Yes, sorry I missed this, will move it to private.

[Dominik Röttsches, 2013-04-22 15:47:45]

I updated the patch to give cancel() a ResourceError reference argument,
initialized to an empty error. Since this brings back the issue of instantiating
a non-empty error using constants, I would appreciate hearing your opinion on
how this is done best without violating layering or coding conventions.

https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
File Source/core/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
Source/core/platform/network/chromium/ResourceHandle.cpp:89:
m_client->didFail(m_owner, error);
didFail() here triggers a WebCore::ResourceLoader::releaseResources() which in
turn deletes the ResourceHandle which this ResourceHandleInternal is owned by.
So, we cannot and don't need to set m_client to 0 after this.

https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
Source/core/platform/network/chromium/ResourceHandle.cpp:196: static const char*
const errorDomain = "net";
Since the cancel() method now checks for isNull(), I need to use the non-trivial
constructor here, initializing it with some values.

Ideally, we should not copy but use the values from net/base/net_error_list.h -
however, the way to retrieve them here while adhering to proper layering would
mean adding a function for retrieving a WebURLError to the WebURLLoader
interface and synthesizing such an error on the Chromium side.

Since the actual values do not have any relevance in this case (only the
isTimeout() flag makes the difference for the didFail call) and a method for
this purpose does not 100% belong into the WebURLLoader class, I was not sure
whether that's worth the interface change. How do you see this?

https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
File Source/core/platform/network/chromium/ResourceHandleInternal.h (right):

https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
Source/core/platform/network/chromium/ResourceHandleInternal.h:91: void
timerFired(Timer<ResourceHandleInternal>*);
Now private.

[darin (slow to review), 2013-04-22 16:24:37]

Apologies.  I have a potentially disruptive comment:

We have talked about thinning WebCore/platform/ to the point where it can almost
evaporate.  In the case of ResourceHandle, this would mean using WebURLLoader
directly from the current places where ResourceHandle gets used.  This patch is
working against such a goal since it is adding more code at the ResourceHandle
level.

Moreover, since this user-requested-timeout feature is only really needed for
XHR, maybe it would anyways be better to move this code closer to XHR.  I'd
suggest looking at moving this feature to DocumentThreadableLoader instead.

-Darin

[Dominik Röttsches, 2013-04-22 16:45:58]

On 2013/04/22 16:24:37, darin wrote:
> I'd suggest looking at moving this feature to DocumentThreadableLoader
instead.

Adam, what do you think? I am fine looking into this if there is a reasonable
consensus. I would just like to be reasonably sure about support for the
approach before I start.

[abarth-chromium, 2013-04-22 17:41:28]

On 2013/04/22 16:45:58, Dominik Röttsches wrote:
> On 2013/04/22 16:24:37, darin wrote:
> > I'd suggest looking at moving this feature to DocumentThreadableLoader
> instead.
> 
> Adam, what do you think? I am fine looking into this if there is a reasonable
> consensus. I would just like to be reasonably sure about support for the
> approach before I start.

Yeah, that sounds like a better approach.  That also avoids the tricky lifetime
issue we're running into where cancelation deletes |this|.

[darin (slow to review), 2013-04-22 17:46:38]

https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
File Source/core/platform/network/chromium/ResourceHandle.cpp (right):

https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
Source/core/platform/network/chromium/ResourceHandle.cpp:79: if
(m_request.timeoutInterval() > 0)
I'm also curious if we can remove ResourceRequest::timeoutInterval() if that
won't be implemented by the ResourceHandle implementation now.

[d.roettsches, 2013-04-22 20:12:44]

On 2013/04/22 17:46:38, darin wrote:
>
https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
> File Source/core/platform/network/chromium/ResourceHandle.cpp (right):
> 
>
https://codereview.chromium.org/14246006/diff/24001/Source/core/platform/netw...
> Source/core/platform/network/chromium/ResourceHandle.cpp:79: if
> (m_request.timeoutInterval() > 0)
> I'm also curious if we can remove ResourceRequest::timeoutInterval() if that
> won't be implemented by the ResourceHandle implementation now.

Yes, if we move the timeout functionality up, timeoutInterval() can be removed.
Originally it was used for exposing a CFNetwork method. If you're not in a
hurry, I'll keep this on my list and file a separate patch for the removal, once
I made progress with reimplementing the timeout nearer to XHR.

[Dominik Röttsches, 2013-05-15 14:31:15]

Updated patch, based on Darin's suggestions to put the logic into
DocumentThreadableLoader.

[Dominik Röttsches, 2013-05-15 15:24:50]

On 2013/05/15 14:31:15, Dominik Röttsches wrote:
> Updated patch, based on Darin's suggestions to put the logic into
> DocumentThreadableLoader.

Looks like as a non-committer I can't get the trybots to actually test the
patch. Tests pass locally on Linux in Chromium browser and run-webkit-tests.

[abarth-chromium, 2013-05-15 15:36:16]

I sent it to the bots for you.

[Dominik Röttsches, 2013-05-15 15:55:39]

On 2013/05/15 15:36:16, abarth wrote:
> I sent it to the bots for you.

Thanks - rebased after the KURL::operator String() change - could you push it to
the bots again?

[Dominik Röttsches, 2013-05-16 11:57:27]

I suspect, the windows bot failure is a line ending issue - which I will look
into. The reason for the mac failure seems flakiness.
Would you be okay with starting to review the new version, Adam or Darin?

[abarth-chromium, 2013-05-17 05:10:40]

The code looks good.  I'll let Darin give the official stamp to make sure he's
happy with the layer we're doing this at.

https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
Source/core/loader/DocumentThreadableLoader.cpp:159: if (error.isNull()) {
I probably would have written errorForCallback.isNull since that's the object
we're working with in this stanza.

https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
Source/core/loader/DocumentThreadableLoader.cpp:393: ResourceError
error(errorDomain, timeoutError, m_resource->url(), String());
I would probably just inline the string constant into the one place it's used. 
The numeric constant for timeoutError is helpful though.

https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
Source/core/loader/DocumentThreadableLoader.h:99: void
timedOut(Timer<DocumentThreadableLoader>*);
didTimeout?

[darin (slow to review), 2013-05-17 06:57:41]

Design wise, this seems like the right spot for this code.  I might consider an
alternative name for the cancel method that takes a ResourceError.  How about
cancelWithError?  It looks like that method can also be made private.

[Dominik Röttsches, 2013-05-17 08:14:38]

On 2013/05/17 05:10:40, abarth wrote:
> The code looks good.  I'll let Darin give the official stamp to make sure he's
> happy with the layer we're doing this at.
> 
>
https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
> File Source/core/loader/DocumentThreadableLoader.cpp (right):
> 
>
https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
> Source/core/loader/DocumentThreadableLoader.cpp:159: if (error.isNull()) {
> I probably would have written errorForCallback.isNull since that's the object
> we're working with in this stanza.

Changed to using errorForCallback.

https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
> Source/core/loader/DocumentThreadableLoader.cpp:393: ResourceError
> error(errorDomain, timeoutError, m_resource->url(), String());
> I would probably just inline the string constant into the one place it's used.

> The numeric constant for timeoutError is helpful though.

"net" inlined.

https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
> File Source/core/loader/DocumentThreadableLoader.h (right):
> 
>
https://codereview.chromium.org/14246006/diff/39001/Source/core/loader/Docume...
> Source/core/loader/DocumentThreadableLoader.h:99: void
> timedOut(Timer<DocumentThreadableLoader>*);
> didTimeout?

Yes, renamed.

On 2013/05/17 06:57:41, darin wrote:
> Design wise, this seems like the right spot for this code.  

> I might consider an
> alternative name for the cancel method that takes a ResourceError.  How about
> cancelWithError?  It looks like that method can also be made private.

Renamed to cancelWithError, moved to private.

Also, I took the freedom to add Intel copyright lines.

[abarth-chromium, 2013-05-17 16:15:49]

lgtm

OK.  Let's get this show on the road.  Thank you for being patient with this CL!

[commit-bot: I haz the power, 2013-05-17 16:15:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/14246006/4...

[commit-bot: I haz the power, 2013-05-17 17:11:00]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[d.roettsches, 2013-05-29 19:36:59]

Trying to get a windows build set up so that I can check what's up with the Win
test failures.

[Dominik Röttsches, 2013-06-03 09:07:38]

I think I found the reason for the windows test failures, let's see what the bot
says.

[Dominik Röttsches, 2013-06-03 10:53:24]

Adam, does the minor change to the test cases a new LGTM? If you're okay with
the updated patch, can you send it to CQ? Thanks!

[abarth-chromium, 2013-06-03 18:29:14]

On 2013/06/03 10:53:24, Dominik Röttsches wrote:
> Adam, does the minor change to the test cases a new LGTM?

I looked at them briefly, and it seems fine.  Generally small tweaks like that
don't require an additional review.

> If you're okay with the updated patch, can you send it to CQ? Thanks!

Will do.

[commit-bot: I haz the power, 2013-06-03 18:29:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/14246006/6...

[commit-bot: I haz the power, 2013-06-03 19:42:56]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-04 06:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/14246006/6...

[commit-bot: I haz the power, 2013-06-04 06:32:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/14246006/6...

[commit-bot: I haz the power, 2013-06-04 08:14:21]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-06-04 17:14:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/14246006/6...

[commit-bot: I haz the power, 2013-06-04 18:25:20]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[Dominik Röttsches, 2013-06-14 11:23:16]

Committed patchset #10 manually as r152437 (presubmit successful).

[Stephen White, 2013-06-14 12:47:03]

This patch seems to be causing a fair number of timeouts and crashes in the http
tests, e.g.:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

I'm afraid I'm going to have to revert.

[Dominik Röttsches, 2013-06-14 12:59:46]

I'll take a look.

[commit-bot: I haz the power, 2013-06-17 08:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/14246006/1...

[commit-bot: I haz the power, 2013-06-17 09:11:32]

Change committed as 152531