Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
273993002:
Allow XHR timeout attribute to be overridden after send(), per spec (Closed)
|
Created:
6 years, 7 months ago by caitp (gmail) Modified:
6 years, 4 months ago CC:
Daniel Bratell, Nate Chapin, sigbjorn Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAllow XHR timeout attribute to be overridden after send(), per spec
http://www.w3.org/TR/XMLHttpRequest/#the-timeout-attribute hints that this attribute may be overridden
after a request has been sent. The web-platform-tests suite has tests verifying that this is handled
correctly, and other browsers such as Gecko-based browsers are successfully passing them, while Blink
and WebKit are not. Therefore, there is an API-compat issue.
BUG=371639, 387470, 387569
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179730
Patch Set 1 #
Total comments: 9
Patch Set 2 : Remove Timer methods, fix code style #Patch Set 3 : Fixed accuracy issues with removed Timer methods #
Total comments: 8
Patch Set 4 : Addressed tyoshino's review comments #
Total comments: 3
Patch Set 5 : Avoid potential race-condition between load and timeout #
Total comments: 14
Patch Set 6 : Minor changes based on Eric's comments #Patch Set 7 : Reset m_requestStartedSeconds in a few extra places +fix bitrot #
Total comments: 1
Patch Set 8 : Rebase + fix test expectations #
Total comments: 7
Patch Set 9 : Fix ASAN failures #
Total comments: 2
Patch Set 10 : Rewrite comments regarding DocumentThreadableLoader end-of-life #Patch Set 11 : Add CORS preflight-failure test case #
Total comments: 1
Patch Set 12 : Rename test failure text and rename test case (this is not a CORS error) #Patch Set 13 : Remove further mentions of "preflight failure" from test case #
Total comments: 10
Patch Set 14 : Comments addressed #Patch Set 15 : Fixed bitrot #Patch Set 16 : sync'd again #Patch Set 17 : Rebase again #Messages
Total messages: 85 (0 generated)
Reasons for this CL: The spec hints that browsers should be able to do this (although I think there are good reasons why browsers wouldn't want to do this, so maybe someone should lobby annevk to change this). In addition to this, other vendors (at least Gecko-based browsers) do support this behaviour. Finally, it gets Blink passing a couple of W3C web-platform-tests that it was previously failing, which is nice. Things I'm not totally sure about: This is my first CL which touches anything crossing multiple threads, so there's a good chance ASAN tests will show problems with it. I can't think of good names for various methods which are added, but I'm open to suggestions if anyone thinks this is worth having. "overrideTimeout" on ThreadableLoader is probably not ideal, nor is "startExact" or "startOneShotExact" on Timer. Finally, I'm not totally sure if this will resolve the new timeout relative to the start of the request (as specified in the spec) in cases where preflights are used, due to not having a good understanding of how much work cross-origin requests have to do before calling loadRequest(). I think that's everything, hopefully I can get some good feedback on this CL, because it's my biggest one yet. Cheers, and thanks in advance for reviewing.
Thanks for the CL. On 2014/05/09 04:54:04, caitp wrote: > Reasons for this CL: > > The spec hints that browsers should be able to do this (although I think there > are good reasons why browsers wouldn't want to do this, so maybe someone should > lobby annevk to change this). > Agreed. > In addition to this, other vendors (at least Gecko-based browsers) do support > this behaviour. > > Finally, it gets Blink passing a couple of W3C web-platform-tests that it was > previously failing, which is nice. > Yeah, nice. > Things I'm not totally sure about: > > This is my first CL which touches anything crossing multiple threads, so there's > a good chance ASAN tests will show problems with it. > The code for bridging between main and worker looks good. > I can't think of good names for various methods which are added, but I'm open to > suggestions if anyone thinks this is worth having. "overrideTimeout" on > ThreadableLoader is probably not ideal, nor is "startExact" or > "startOneShotExact" on Timer. > > Finally, I'm not totally sure if this will resolve the new timeout relative to > the start of the request (as specified in the spec) in cases where preflights > are used, due to not having a good understanding of how much work cross-origin > requests have to do before calling loadRequest(). > Hmm, I think it should be fine to just use the time when fetch started as base. > I think that's everything, hopefully I can get some good feedback on this CL, > because it's my biggest one yet. Cheers, and thanks in advance for reviewing.
https://codereview.chromium.org/273993002/diff/1/Source/core/loader/DocumentT... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/1/Source/core/loader/DocumentT... Source/core/loader/DocumentThreadableLoader.cpp:144: if (m_async) { if (!m_async) return; ... https://codereview.chromium.org/273993002/diff/1/Source/core/loader/DocumentT... Source/core/loader/DocumentThreadableLoader.cpp:155: m_timeoutTimer.startOneShotExact(resolvedTime, FROM_HERE); any reason you chose to add this new method to Timer than calculating the time from now until the new timeout? https://codereview.chromium.org/273993002/diff/1/Source/core/loader/DocumentT... Source/core/loader/DocumentThreadableLoader.cpp:409: m_requestStarted = monotonicallyIncreasingTime(); move this to between L419 and L420 (outside of if-clause)? https://codereview.chromium.org/273993002/diff/1/Source/platform/Timer.cpp File Source/platform/Timer.cpp (right): https://codereview.chromium.org/273993002/diff/1/Source/platform/Timer.cpp#ne... Source/platform/Timer.cpp:215: void TimerBase::startExact(double nextFireInterval, double repeatInterval, const TraceLocation& caller) s/nextFireInterval/nextFireTime/ https://codereview.chromium.org/273993002/diff/1/Source/platform/Timer.cpp#ne... Source/platform/Timer.cpp:224: single blank line
https://codereview.chromium.org/273993002/diff/1/Source/core/loader/DocumentT... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/1/Source/core/loader/DocumentT... Source/core/loader/DocumentThreadableLoader.cpp:155: m_timeoutTimer.startOneShotExact(resolvedTime, FROM_HERE); On 2014/05/09 08:04:36, tyoshino wrote: > any reason you chose to add this new method to Timer than calculating the time > from now until the new timeout? For IEEE floating point math, you lose accuracy if you mix small additions with large ones in a weird order, so adding the extra subtract is potentially an issue. But you're right, it's better to have less code, and it's probably not going to introduce major flakiness, so I'll remove the Timer methods
https://codereview.chromium.org/273993002/diff/1/Source/platform/Timer.h File Source/platform/Timer.h (right): https://codereview.chromium.org/273993002/diff/1/Source/platform/Timer.h#newc... Source/platform/Timer.h:48: void startExact(double nextFireInterval, double repeatInterval, const TraceLocation& caller); As a user of this API I would wonder about the difference between start and startExact. Note that I'm not an owner of this code or the XHR code so my opinions are not normative. :-)
On 2014/05/09 13:54:45, Daniel Bratell wrote: > https://codereview.chromium.org/273993002/diff/1/Source/platform/Timer.h > File Source/platform/Timer.h (right): > > https://codereview.chromium.org/273993002/diff/1/Source/platform/Timer.h#newc... > Source/platform/Timer.h:48: void startExact(double nextFireInterval, double > repeatInterval, const TraceLocation& caller); > As a user of this API I would wonder about the difference between start and > startExact. > > Note that I'm not an owner of this code or the XHR code so my opinions are not > normative. :-) PTAL when you guys have some time --- I've fixed the style problems, and removed the Timer methods. As expected, removing the ability to set an exact nextFire time causes some problems for the timer, and breaks some of the other tests --- so to get around this, I avoid performing any operation when the attribute is being set to the same value. But this is a hack, it gets around one particular accuracy issue, but not the other one. So I kind of think having special methods for the Timer to specify an accurate nextFireTime is ideal here. SGTY?
On 2014/05/09 19:24:15, caitp wrote: > As expected, removing the ability to set an exact nextFire time causes some > problems for the timer Specifically, it causes accuracy problems when resolving the time. It is Gecko's strategy, but they're using integral time units, so they don't have the same issue
Nevermind the accuracy issues, that's fixed.
please add abarth@ for OWNER review once you address my comments. https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... File LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override.html (right): https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override.html:40: xhr.timeout = 1000; how about using value clearly greater than one passed to stallFor? https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... File LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js (right): https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js:2: testRunner.dumpAsText(false); why this? https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... File LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js (right): https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js:24: xhr.timeout = 1000; ditto https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js:33: xhr.abort(); 4 space indentation https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js:37: remove this blank line https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... File LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js (right): https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js:4: } remove the space at the head https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... Source/core/loader/DocumentThreadableLoader.cpp:155: double elapsedTime = (monotonicallyIncreasingTime() - m_requestStarted); remove parentheses https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... File Source/core/loader/DocumentThreadableLoader.h (right): https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... Source/core/loader/DocumentThreadableLoader.h:106: double m_requestStarted; please add a comment to explain on what kind of event this is set. https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Threa... File Source/core/loader/ThreadableLoader.h (right): https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Threa... Source/core/loader/ThreadableLoader.h:89: // may override their timeout setting after sending). please explain that the timeout will be updated to (the time the load was started) + timeoutMilliseconds, not from now. https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Worke... File Source/core/loader/WorkerThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Worke... Source/core/loader/WorkerThreadableLoader.cpp:181: void WorkerThreadableLoader::MainThreadBridge::overrideTimeout(unsigned long timeoutMilliseconds) please move this definition to between l168 and l170 since currently they're organized as a list like mainThreadFoo, foo, mainThreadBar, bar, ...
On 2014/05/12 09:21:17, tyoshino wrote: > please add abarth@ for OWNER review once you address my comments. > > https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... > File LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override.html > (right): > > https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override.html:40: > xhr.timeout = 1000; > how about using value clearly greater than one passed to stallFor? > > https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... > File > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js > (right): > > https://codereview.chromium.org/273993002/diff/1/LayoutTests/http/tests/xmlht... > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js:2: > testRunner.dumpAsText(false); > why this? > > https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... > File > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js > (right): > > https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js:24: > xhr.timeout = 1000; > ditto > > https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js:33: > xhr.abort(); > 4 space indentation > > https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override-worker.js:37: > > remove this blank line > > https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... > File > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js > (right): > > https://codereview.chromium.org/273993002/diff/40001/LayoutTests/http/tests/x... > LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js:4: > } > remove the space at the head > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... > File Source/core/loader/DocumentThreadableLoader.cpp (right): > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... > Source/core/loader/DocumentThreadableLoader.cpp:155: double elapsedTime = > (monotonicallyIncreasingTime() - m_requestStarted); > remove parentheses > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... > File Source/core/loader/DocumentThreadableLoader.h (right): > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Docum... > Source/core/loader/DocumentThreadableLoader.h:106: double m_requestStarted; > please add a comment to explain on what kind of event this is set. > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Threa... > File Source/core/loader/ThreadableLoader.h (right): > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Threa... > Source/core/loader/ThreadableLoader.h:89: // may override their timeout setting > after sending). > please explain that the timeout will be updated to (the time the load was > started) + timeoutMilliseconds, not from now. > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Worke... > File Source/core/loader/WorkerThreadableLoader.cpp (right): > > https://codereview.chromium.org/273993002/diff/40001/Source/core/loader/Worke... > Source/core/loader/WorkerThreadableLoader.cpp:181: void > WorkerThreadableLoader::MainThreadBridge::overrideTimeout(unsigned long > timeoutMilliseconds) > please move this definition to between l168 and l170 since currently they're > organized as a list like mainThreadFoo, foo, mainThreadBar, bar, ... Comments addressed, and @abarth added as a reviewer as suggested... So, the tests added here are pretty close to tests present in web-platform-tests, it might be a good idea to just use the WPT tests instead. I know there are some WPT tests already in the tree, but I'm not totally sure how to run them correctly, so if anyone can fill me in on that, that would be useful.
https://codereview.chromium.org/273993002/diff/80001/Source/core/loader/Docum... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/80001/Source/core/loader/Docum... Source/core/loader/DocumentThreadableLoader.cpp:158: m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE); We should start the timer only when there's active fetch. There's no convenient way to know whether there's an active fetch. We cannot check that by looking at m_timeoutTimer.is_active() since it may not be active because the user didn't set timeout option when the XHR is started. Maybe the right thing to do is checking if resource() is NULL or not but currently resource() is cleared when cancel() is called or didTimeout is called, but not when notifyFinished() is called. Could you please try clearing resource at the end of notifyFinished (and maybe also m_client)?
On 2014/05/09 08:04:27, tyoshino wrote: > > I can't think of good names for various methods which are added, but I'm open > to > > suggestions if anyone thinks this is worth having. "overrideTimeout" on > > ThreadableLoader is probably not ideal, nor is "startExact" or > > "startOneShotExact" on Timer. > > > > Finally, I'm not totally sure if this will resolve the new timeout relative to > > the start of the request (as specified in the spec) in cases where preflights > > are used, due to not having a good understanding of how much work cross-origin > > requests have to do before calling loadRequest(). > > > > Hmm, I think it should be fine to just use the time when fetch started as base. Sorry, I meant when the DocumentThreadableLoader is started. Not for each loadRequest (i.e. including preflight). Blink is currently not conforming to the spec as you pointed out. Let's fix it later.
https://codereview.chromium.org/273993002/diff/80001/Source/core/loader/Docum... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/80001/Source/core/loader/Docum... Source/core/loader/DocumentThreadableLoader.cpp:158: m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE); On 2014/05/15 09:15:43, tyoshino wrote: > We should start the timer only when there's active fetch. There's no convenient > way to know whether there's an active fetch. We cannot check that by looking at > m_timeoutTimer.is_active() since it may not be active because the user didn't > set timeout option when the XHR is started. Maybe the right thing to do is > checking if resource() is NULL or not but currently resource() is cleared when > cancel() is called or didTimeout is called, but not when notifyFinished() is > called. > > Could you please try clearing resource at the end of notifyFinished (and maybe > also m_client)? What about just `m_requestStarted > 0.0` ? It gets reset on cancel/timeout/error/finish --- But I think in practice, there's always an active fetch, since DocumentThreadableLoader starts a request immediately after being constructed, provided it doesn't fail
https://codereview.chromium.org/273993002/diff/80001/Source/core/loader/Docum... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/80001/Source/core/loader/Docum... Source/core/loader/DocumentThreadableLoader.cpp:158: m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE); On 2014/05/15 11:51:03, caitp wrote: > On 2014/05/15 09:15:43, tyoshino wrote: > > We should start the timer only when there's active fetch. There's no > convenient > > way to know whether there's an active fetch. We cannot check that by looking > at > > m_timeoutTimer.is_active() since it may not be active because the user didn't > > set timeout option when the XHR is started. Maybe the right thing to do is > > checking if resource() is NULL or not but currently resource() is cleared when > > cancel() is called or didTimeout is called, but not when notifyFinished() is > > called. > > > > Could you please try clearing resource at the end of notifyFinished (and maybe > > also m_client)? > > What about just `m_requestStarted > 0.0` ? It gets reset on Sounds fine as far as well commented. > cancel/timeout/error/finish --- But I think in practice, there's always an > active fetch, since DocumentThreadableLoader starts a request immediately after > being constructed, provided it doesn't fail I'm concerned about race between overrideTimeout call and completion of loading. XMLHttpRequest is checking if m_loader is empty or not, but even with that check when it's run in a worker, overrideTimeout call goes through the task queue and can be run after notifyFinished in the main thread.
Okay, I did try `clearResource()` in DocumentThreadableLoader::notifyFinish, but unfortunately it caused many XHR tests to fail, so I've fallen back on testing `m_requestStarted` instead. There should be no real performance hit since the double needs to be loaded into FPU registers regardless, so it's all good. Tests are passing locally still. @tyoshino can you set up a try job for just blink layout tests to see if this works correctly on other platforms?
So, I'm pretty sure the windows failure is unrelated to this CL. There was nothing similar on the same trybot, but looking at the tests, they don't look like they should be affected at all.
https://codereview.chromium.org/273993002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override.html (right): https://codereview.chromium.org/273993002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override.html:8: <p> Verify that a timeout ProgressEvent is dispatched and have the expected values.</p> remove https://codereview.chromium.org/273993002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override.html:9: <div id="logEvent"></div> remove https://codereview.chromium.org/273993002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js (right): https://codereview.chromium.org/273993002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/xmlhttprequest/workers/resources/xmlhttprequest-timeout-override.js:17: var worker = createWorker('resources/xmlhttprequest-timeout-override-worker.js'); can we just new Worker here? https://codereview.chromium.org/273993002/diff/100001/LayoutTests/http/tests/... File LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-timeout-override.html (right): https://codereview.chromium.org/273993002/diff/100001/LayoutTests/http/tests/... LayoutTests/http/tests/xmlhttprequest/workers/xmlhttprequest-timeout-override.html:6: <p>Test that the XMLHttpRequest timeout can be overridden in a Worker context. If this test passes, there should be a single PASS below.</p> we see PASS and timeout
abarth@ is days off. Adding eseidel@ for OWNER review.
lgtm https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:159: double resolvedTime = nextFire > elapsedTime ? nextFire - elapsedTime : 0.0; std:min(nextfire - elapsedTime, 0) may be simpler. https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.h (right): https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.h:106: double m_requestStarted; // Time an asynchronous fetch request is started (loadRequest) I might consider labeling the units here. m_requestStartedSeconds;
On 2014/05/21 21:41:49, eseidel wrote: > lgtm > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > File Source/core/loader/DocumentThreadableLoader.cpp (right): > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:159: double resolvedTime = > nextFire > elapsedTime ? nextFire - elapsedTime : 0.0; > std:min(nextfire - elapsedTime, 0) may be simpler. > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > File Source/core/loader/DocumentThreadableLoader.h (right): > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.h:106: double m_requestStarted; // > Time an asynchronous fetch request is started (loadRequest) > I might consider labeling the units here. > > m_requestStartedSeconds; Thanks for the prompt review =) These comments seem reasonable, I will deal with these shortly
lgtm please reset m_requestStarted when loading fails. some of them might not be necessary. we can reduce them on next refactoring. https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:202: request = ResourceRequest(); m_requestStarted = 0.0; https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:268: request = ResourceRequest(); m_requestStarted = 0.0; https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:365: m_requestStarted = 0.0; not here. see below https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:375: m_client->didFinishLoading(identifier, finishTime); m_requestStarted = 0.0; inside else block https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:406: m_client->didFailAccessControlCheck(error); m_requestStarted = 0.0; https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:448: m_client->didFail(error); m_requestStarted = 0.0; unnecessary for sync case but for consistency https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:455: m_client->didFail(error); m_requestStarted = 0.0; unnecessary for sync case but for consistency https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:463: m_client->didFailRedirectCheck(); m_requestStarted = 0.0; unnecessary for sync case but for consistency
On 2014/05/28 02:21:26, tyoshino wrote: > lgtm > > please reset m_requestStarted when loading fails. some of them might not be > necessary. we can reduce them on next refactoring. > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > File Source/core/loader/DocumentThreadableLoader.cpp (right): > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:202: request = > ResourceRequest(); > m_requestStarted = 0.0; > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:268: request = > ResourceRequest(); > m_requestStarted = 0.0; > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:365: m_requestStarted = 0.0; > not here. see below > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:375: > m_client->didFinishLoading(identifier, finishTime); > m_requestStarted = 0.0; > > inside else block > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:406: > m_client->didFailAccessControlCheck(error); > m_requestStarted = 0.0; > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:448: m_client->didFail(error); > m_requestStarted = 0.0; > > unnecessary for sync case but for consistency > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:455: m_client->didFail(error); > m_requestStarted = 0.0; > > unnecessary for sync case but for consistency > > https://codereview.chromium.org/273993002/diff/100001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:463: > m_client->didFailRedirectCheck(); > m_requestStarted = 0.0; > > unnecessary for sync case but for consistency I'm not sure any of these are really necessary, because ThreadableLoader is pretty much a one-shot-per-instance deal, as far as I can tell, unless there are plans to make them more reusable in the future. However, I've added the requested changes (if I understood them correctly), so if opinions change, just ping me and I'll upload a fix.
lgtm https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:185: m_requestStartedSeconds = 0.0; If we start to clear multiple things at the same time in many places it means we should have a method to do so, or make them both part of a struct which keeps their lifetime the same. It looks like this is the only place where we clear more than one of these? Should the ResourceRequest (or some wrapper) eventually hold this start-time instead?
On 2014/05/28 22:06:36, eseidel wrote: > lgtm > > https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... > File Source/core/loader/DocumentThreadableLoader.cpp (right): > > https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... > Source/core/loader/DocumentThreadableLoader.cpp:185: m_requestStartedSeconds = > 0.0; > If we start to clear multiple things at the same time in many places it means we > should have a method to do so, or make them both part of a struct which keeps > their lifetime the same. It looks like this is the only place where we clear > more than one of these? Should the ResourceRequest (or some wrapper) eventually > hold this start-time instead? It's currently not so easy to figure out such structure. I'm refactoring DocumentThreadableLoader to make it easier to find it out. http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentThreada...
On 2014/05/29 02:16:27, tyoshino wrote: > On 2014/05/28 22:06:36, eseidel wrote: > > lgtm > > > > > https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... > > File Source/core/loader/DocumentThreadableLoader.cpp (right): > > > > > https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... > > Source/core/loader/DocumentThreadableLoader.cpp:185: m_requestStartedSeconds = > > 0.0; > > If we start to clear multiple things at the same time in many places it means > we > > should have a method to do so, or make them both part of a struct which keeps > > their lifetime the same. It looks like this is the only place where we clear > > more than one of these? Should the ResourceRequest (or some wrapper) > eventually > > hold this start-time instead? > > It's currently not so easy to figure out such structure. I'm refactoring > DocumentThreadableLoader to make it easier to find it out. > > http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentThreada... I think, one of the things that makes it difficult is that in the case of cross-origin requests, we want the request start time to be the start of the preflight request (I THINK) --- but if the lifetime is attached to both requests, and overrideTimeout had a way of getting at the request object, it would resolve its time relative to the wrong request start. So generally, this information does need to live inside of the ThreadableLoader and not within ResourceRequest, unfortunately. Since it's currently a 1-line operation to fix it, maybe it would be better to just leave it as-is for now until it needs to do something more complicated, which might warrant a method --- unless it could live inside a wrapper for ResourceRequest which would hold both the preflight and actual requests.
On 2014/06/03 17:04:06, caitp wrote: > On 2014/05/29 02:16:27, tyoshino wrote: > > On 2014/05/28 22:06:36, eseidel wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... > > > File Source/core/loader/DocumentThreadableLoader.cpp (right): > > > > > > > > > https://codereview.chromium.org/273993002/diff/140001/Source/core/loader/Docu... > > > Source/core/loader/DocumentThreadableLoader.cpp:185: m_requestStartedSeconds > = > > > 0.0; > > > If we start to clear multiple things at the same time in many places it > means > > we > > > should have a method to do so, or make them both part of a struct which > keeps > > > their lifetime the same. It looks like this is the only place where we > clear > > > more than one of these? Should the ResourceRequest (or some wrapper) > > eventually > > > hold this start-time instead? > > > > It's currently not so easy to figure out such structure. I'm refactoring > > DocumentThreadableLoader to make it easier to find it out. > > > > > http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentThreada... > > I think, one of the things that makes it difficult is that in the case of > cross-origin requests, we want the request start time to be the start of the > preflight request (I THINK) --- but if the lifetime is attached to both > requests, and overrideTimeout had a way of getting at the request object, it > would resolve its time relative to the wrong request start. I also think the timeout mechanism should count time for the whole operation including actual and preflight request. The timeout code should live in DocumentThreadableLoader or XHR. > > So generally, this information does need to live inside of the ThreadableLoader > and not within ResourceRequest, unfortunately. > > Since it's currently a 1-line operation to fix it, maybe it would be better to > just leave it as-is for now until it needs to do something more complicated, > which might warrant a method --- unless it could live inside a wrapper for > ResourceRequest which would hold both the preflight and actual requests.
hmm, it's not clear that this is ever going to get looked at by adam, and it's already probably bitrotten again, do either of you have a suggestion for a different owner?
It looks like you have reviews? I'm confused. What are you waiting for? Which Adam?
On 2014/06/20 20:42:56, eseidel wrote: > It looks like you have reviews? I'm confused. What are you waiting for? Which > Adam? This is waiting on abarth to rubber stamp it, I guess, I can't check in code into Blink, so I can't do much with this other than wait, and try to keep it up to date
The CQ bit was checked by eseidel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitpotter88@gmail.com/273993002/160001
Message was sent while issue was closed.
Change committed as 176659
Message was sent while issue was closed.
On 2014/06/20 23:51:09, I haz the power (commit-bot) wrote: > Change committed as 176659 Revert in https://codereview.chromium.org/333823006
Message was sent while issue was closed.
https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:393: m_requestStartedSeconds = 0.0; There's no reason to believe |this| is still alive at this point. Reading the review thread, folks asked for me to take a look at this CL before it landed, which didn't happen.
Message was sent while issue was closed.
https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:393: m_requestStartedSeconds = 0.0; On 2014/06/22 00:22:48, abarth wrote: > There's no reason to believe |this| is still alive at this point. > > Reading the review thread, folks asked for me to take a look at this CL before > it landed, which didn't happen. Given that this is a non-static method, there probably is a good reason to assume |this| is still alive. I see that it's not, but it's not clear that expecting 'this' to exist in an instance method is the root cause. Especially since we're referencing m_client.
Message was sent while issue was closed.
https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:393: m_requestStartedSeconds = 0.0; Unless you mean that `didFinishLoading()` might kill 'this', okay.
Message was sent while issue was closed.
On 2014/06/22 at 00:56:14, caitpotter88 wrote: > Given that this is a non-static method, there probably is a good reason to assume |this| is still alive. That's not how this class works. > Unless you mean that `didFinishLoading()` might kill 'this', okay. That's exactly how the lifecycle of this object works.
Message was sent while issue was closed.
Fix ASAN failures https://codereview.chromium.org/273993002/diff/180001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/180001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:477: m_requestStartedSeconds = 0.0; // Unnecessary for synchronous request, but for API consistency. This might cause an access violation similar to the other cases, but it didn't look like the loader was being clobbered in quite the same way (in XHR's case) for didFail. Similar might apply to line 493 --- however if these are problems, they aren't being hit in tests
https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:393: m_requestStartedSeconds = 0.0; On 2014/06/22 00:58:19, caitp wrote: > Unless you mean that `didFinishLoading()` might kill 'this', okay. Sorry that I didn't catch this. Right, didFinishLoading() may kill this. You can just put m_requestStartedSeconds = 0.0 before calling didFinishLoading(). https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:431: m_requestStartedSeconds = 0.0; ditto https://codereview.chromium.org/273993002/diff/180001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/180001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:477: m_requestStartedSeconds = 0.0; // Unnecessary for synchronous request, but for API consistency. On 2014/06/22 09:49:09, caitp wrote: > This might cause an access violation similar to the other cases, but it didn't > look like the loader was being clobbered in quite the same way (in XHR's case) > for didFail. > > Similar might apply to line 493 --- however if these are problems, they aren't > being hit in tests For synchronous loading, DocumentThreadableLoader::loadResourceSynchronously() holds a reference while we're loading. So, you don't worry about this.
When relanding this, please include 387569 to BUG line
https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:393: m_requestStartedSeconds = 0.0; It would have been hard to notice =) I'm not sure there's a good reason to reset m_requestStarted if there's a guarantee the loader will die (I guess it could be done in the destructor to handle all of these cases?) --- but it may be useful for `didFinishLoading()`, etc, to know when the entire request was started. Or it might not, I'm not sure. I could see that potentially being useful later on, though.
lgtm https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/160001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:393: m_requestStartedSeconds = 0.0; On 2014/06/23 15:55:44, caitp wrote: > I'm not sure there's a good reason to reset m_requestStarted if there's a > guarantee the loader will die (I guess it could be done in the destructor to > handle all of these cases?) --- but it may be useful for `didFinishLoading()`, > etc, to know when the entire request was started. Or it might not, I'm not sure. > I could see that potentially being useful later on, though. Feel free to postpone this and leave FIXME explaining discussion we had. I.e. I'm fine with not having this line here and L431 for now. It's not guaranteed that the loader dies on didFinishLoading, but it's not the right way to call overrideTimeout on the ThreadableLoader after receiving didFinishLoading callback and I guess no one does it.
PTAL I think that yeah, the CORS preflight-failure test is wrong since it's just a GET request, but even so, the test passes, we aren't able to override the timeout after a failure. I had asked about other cases we'd need to verify, so if there's any suggestions there, I'm happy to oblige.
lgtm https://codereview.chromium.org/273993002/diff/240001/LayoutTests/http/tests/... File LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override-after-preflight-failure.html (right): https://codereview.chromium.org/273993002/diff/240001/LayoutTests/http/tests/... LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override-after-preflight-failure.html:41: log("FAIL: Timeout overridden after error"); what we want to check is setting timeout doesn't start the timer again after the XHR is errored. So, I'd like you to include some text meaning that here. For example... FAIL: Timeout override reactivated the timer
On 2014/07/08 11:36:00, tyoshino wrote: > lgtm > > https://codereview.chromium.org/273993002/diff/240001/LayoutTests/http/tests/... > File > LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override-after-preflight-failure.html > (right): > > https://codereview.chromium.org/273993002/diff/240001/LayoutTests/http/tests/... > LayoutTests/http/tests/xmlhttprequest/ontimeout-event-override-after-preflight-failure.html:41: > log("FAIL: Timeout overridden after error"); > what we want to check is setting timeout doesn't start the timer again after the > XHR is errored. So, I'd like you to include some text meaning that here. > > For example... > > FAIL: Timeout override reactivated the timer Okay, I've made the log messages more clear and have removed mentions of preflight/CORS failures from the test, since there is no preflight request made in this case. I'll build and ASAN test again tonight just to try and avoid another regression, but I think this should be generally ok
the message change lgtm
On 2014/07/10 04:20:23, tyoshino wrote: > the message change lgtm Just checked, and the xmlhttprequest tests are all passing still, with ASAN (not tested with asan_coverage). I think it's probably okay to check-in, but I'd prefer if @abarth says it's okay first.
On 2014/07/11 04:21:09, caitp wrote: > On 2014/07/10 04:20:23, tyoshino wrote: > > the message change lgtm > > Just checked, and the xmlhttprequest tests are all passing still, with ASAN (not > tested with asan_coverage). I think it's probably okay to check-in, but I'd > prefer if @abarth says it's okay first. Feel free to ping the reviewer periodically :) It passed 2 weeks. In case the reviewer has already l-g-t-m-ed, he/she may overlooking the CL even if it's showing at the top of his/her dashboard. In such a case, feel free to send ping mail to him/her.
I'm not sure how to ping specific users on rietveld, other than sending mail to all reviewers. If there's a process for that, it hasn't been explained anywhere that I can see. In the spirit of trying, PTAL abarth@
Adam is out on vacation until Tues. I'd try to find him in #blink when he returns.
On 2014/07/24 15:00:54, caitp wrote: > I'm not sure how to ping specific users on rietveld, other than sending mail to > all reviewers. If there's a process for that, it hasn't been explained anywhere > that I can see. > > In the spirit of trying, PTAL abarth@ Yes, that's what I meant.
https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:394: } else Please add { } https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:481: m_requestStartedSeconds = 0.0; // Unnecessary for synchronous request, but for API consistency. Why doesn't this cause the same problem? Can the client not delete us unside the didFail callback here? https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:497: m_requestStartedSeconds = 0.0; // Unnecessary for synchronous request, but for API consistency. ditto
https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:157: return; How can this be called during a synchronous request? Seems like this ought to be an assert https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:166: if (timeoutMilliseconds && m_requestStartedSeconds > 0.0) { How could m_requestStartedSeconds not be > 0.0? Should that be an assert? https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:170: m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE); So, if the request has been running already for 2 seconds and we set a 1 second timeout, the value is ignored? I would have expect us to cancel the request instead because it has exceeded its timeout. This case is particularly likely when making an XMLHttpRequest from a worker because the main thread might be busy and not get around to processing the timeout override for a while. I think the current behavior is racy in that case. https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Work... File Source/core/loader/WorkerThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Work... Source/core/loader/WorkerThreadableLoader.cpp:177: } This code is really ugly, but that's not your fault. :)
https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:170: m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE); I am not exactly certain how startOneShot() will work, but to me, what this is saying is that we will timeout immediately (resolvedTime === 0.0, because of the std::max call) if we ask for a timeout which is less than the already elapsed time
https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:170: m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE); On 2014/07/31 16:43:29, caitp wrote: > I am not exactly certain how startOneShot() will work, but to me, what this is > saying is that we will timeout immediately (resolvedTime === 0.0, because of the > std::max call) if we ask for a timeout which is less than the already elapsed > time Correct. For the abarth@'s example o elapsedTime == 2 o nextFire - elapsedTime == -1 o resolvedTime == 0 We'll run didTimeout asynchronously but immediately. https://codereview.chromium.org/273993002/diff/280001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:481: m_requestStartedSeconds = 0.0; // Unnecessary for synchronous request, but for API consistency. On 2014/07/31 16:01:35, abarth wrote: > Why doesn't this cause the same problem? Can the client not delete us unside > the didFail callback here? We enter this path only when m_asyc is false. So, there should be a ref on this DocumentThreadableLoader held by the caller. loader->loadResourceSynchronously(...); It's not 100% sure that the caller doesn't clear the smart pointer holding the ref in didFail(). m_loader->loadResourceSynchronously(...); ... void didFail(const ResourceError error) { m_loader = nullptr; } I don't like code like this (deleting the object before some sync call to a method of the object), and rather want to forbid this than not expecting the object is live until we exit from loadRequest(). But I'm also fine with removing this line and L497. As comment is saying they're unnecessary.
patchset 15 lgtm
LGTM
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitpotter88@gmail.com/273993002/320001
On 2014/08/01 23:32:35, caitp wrote: > The CQ bit was checked by mailto:caitpotter88@gmail.com I hope that's okay and does a thing (rietveld is so confusing sometimes!) --- but more importantly, I really hope not to break the tree again.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20236)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20245)
On 2014/08/02 01:34:52, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_blink_rel on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20245) hmmm, I am pretty confident those failures are unrelated to this CL, the windows build failed in different ways for each retry.
The CQ bit was checked by abarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitpotter88@gmail.com/273993002/320001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20397)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20400)
The CQ bit was checked by tyoshino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitpotter88@gmail.com/273993002/320001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21224)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21229)
The CQ bit was checked by tyoshino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitpotter88@gmail.com/273993002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/loader/DocumentThreadableLoader.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/loader/DocumentThreadableLoader.cpp
Hunk #1 succeeded at 81 (offset 1 line).
Hunk #2 succeeded at 155 (offset 1 line).
Hunk #3 succeeded at 196 (offset 1 line).
Hunk #4 succeeded at 222 (offset 1 line).
Hunk #5 succeeded at 289 (offset 1 line).
Hunk #6 FAILED at 420.
Hunk #7 succeeded at 459 (offset 2 lines).
1 out of 7 hunks FAILED -- saving rejects to file
Source/core/loader/DocumentThreadableLoader.cpp.rej
Patch: Source/core/loader/DocumentThreadableLoader.cpp
Index: Source/core/loader/DocumentThreadableLoader.cpp
diff --git a/Source/core/loader/DocumentThreadableLoader.cpp
b/Source/core/loader/DocumentThreadableLoader.cpp
index
f49558151d8a1e25d4e91e85b2909f2d7cfad76b..01211338992ee92bd7a6385091529a016834e15e
100644
--- a/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/Source/core/loader/DocumentThreadableLoader.cpp
@@ -80,11 +80,14 @@ DocumentThreadableLoader::DocumentThreadableLoader(Document&
document, Threadabl
, m_simpleRequest(true)
, m_async(blockingBehavior == LoadAsynchronously)
, m_timeoutTimer(this, &DocumentThreadableLoader::didTimeout)
+ , m_requestStartedSeconds(0.0)
{
ASSERT(client);
// Setting an outgoing referer is only supported in the async code path.
ASSERT(m_async || request.httpReferrer().isEmpty());
+ m_requestStartedSeconds = monotonicallyIncreasingTime();
+
// Save any CORS simple headers on the request here. If this request
redirects cross-origin, we cancel the old request
// create a new one, and copy these headers.
const HTTPHeaderMap& headerMap = request.httpHeaderFields();
@@ -151,6 +154,26 @@ DocumentThreadableLoader::~DocumentThreadableLoader()
{
}
+void DocumentThreadableLoader::overrideTimeout(unsigned long
timeoutMilliseconds)
+{
+ ASSERT(m_async);
+ ASSERT(m_requestStartedSeconds > 0.0);
+ m_timeoutTimer.stop();
+ // At the time of this method's implementation, it is only ever called by
+ // XMLHttpRequest, when the timeout attribute is set after sending the
+ // request.
+ //
+ // The XHR request says to resolve the time relative to when the request
+ // was initially sent, however other uses of this method may need to
+ // behave differently, in which case this should be re-arranged somehow.
+ if (timeoutMilliseconds) {
+ double elapsedTime = monotonicallyIncreasingTime() -
m_requestStartedSeconds;
+ double nextFire = timeoutMilliseconds / 1000.0;
+ double resolvedTime = std::max(nextFire - elapsedTime, 0.0);
+ m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE);
+ }
+}
+
void DocumentThreadableLoader::cancel()
{
cancelWithError(ResourceError());
@@ -172,6 +195,7 @@ void DocumentThreadableLoader::cancelWithError(const
ResourceError& error)
}
clearResource();
m_client = 0;
+ m_requestStartedSeconds = 0.0;
}
void DocumentThreadableLoader::setDefersLoading(bool value)
@@ -197,6 +221,7 @@ void DocumentThreadableLoader::redirectReceived(Resource*
resource, ResourceRequ
if (!isAllowedByPolicy(request.url())) {
m_client->didFailRedirectCheck();
request = ResourceRequest();
+ m_requestStartedSeconds = 0.0;
return;
}
@@ -263,6 +288,7 @@ void DocumentThreadableLoader::redirectReceived(Resource*
resource, ResourceRequ
m_client->didFailRedirectCheck();
}
request = ResourceRequest();
+ m_requestStartedSeconds = 0.0;
}
void DocumentThreadableLoader::dataSent(Resource* resource, unsigned long long
bytesSent, unsigned long long totalBytesToBeSent)
@@ -394,8 +420,11 @@ void
DocumentThreadableLoader::handleSuccessfulFinish(unsigned long identifier,
ASSERT(!m_sameOriginRequest);
ASSERT(m_options.crossOriginRequestPolicy == UseAccessControl);
loadActualRequest();
- } else
+ } else {
+ // FIXME: Should prevent timeout from being overridden after finished
loading, without
+ // resetting m_requestStartedSeconds to 0.0
m_client->didFinishLoading(identifier, finishTime);
+ }
}
void DocumentThreadableLoader::didTimeout(Timer<DocumentThreadableLoader>*
timer)
@@ -431,6 +460,8 @@ void DocumentThreadableLoader::handlePreflightFailure(const
String& url, const S
// Prevent handleSuccessfulFinish() from bypassing access check.
m_actualRequest = nullptr;
+ // FIXME: Should prevent timeout from being overridden after preflight
failure, without
+ // resetting m_requestStartedSeconds to 0.0
m_client->didFailAccessControlCheck(error);
}
Hi caitp, I re-checked the CQ bit but patching failed. Please rebase. Thanks!
On 2014/08/07 09:18:54, tyoshino wrote: > Hi caitp, > > I re-checked the CQ bit but patching failed. Please rebase. Thanks! I guess I'm going to have to disable managed mode and rebase properly :( alright
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitpotter88@gmail.com/273993002/360001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21566)
Message was sent while issue was closed.
Change committed as 179730 |
