Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543633004:
Fix a pair of ResourceLoader cancellation/error bugs. (Closed)
|
Created:
4 years ago by mmenke Modified:
4 years ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, cbentzel+watch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix a pair of ResourceLoader cancellation/error bugs.
Both of these would result in two completion messages being passed
down the ResourceHandler chain.
Also add a bunch of tests of various flows through ResourceLoader, one
of which is disabled due to yet another bug (Which I'll fix in another
CL), and make the ResourceLoader tests use the same TestResourceHandler
that the MIME sniffing / intercepting ResourceHandler tests use.
BUG=669709
Committed: https://crrev.com/1d5e2e389193fc814291e64469a9b28d3412a9e9
Cr-Commit-Position: refs/heads/master@{#436732}
Patch Set 1 #Patch Set 2 : Check RDHDelegate calls #Patch Set 3 : Fix cert tests, add out-of-band cancel tests #
Total comments: 9
Patch Set 4 : Update comments #Patch Set 5 : Remove old test handler (No longer being used) #
Total comments: 29
Patch Set 6 : Response to comments #Patch Set 7 : Merge #
Dependent Patchsets: Messages
Total messages: 47 (30 generated)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix a pair of ResourceLoader cancellation/error bugs. Both of these would result in two completion messages being passed down the ResourceHandler chain. Also add a bunch of tests of various flows through ResourceLoader, one of which is disabled due to yet another bug (Which I'll fix in another CL). BUG=669709 ========== to ========== Fix a pair of ResourceLoader cancellation/error bugs. Both of these would result in two completion messages being passed down the ResourceHandler chain. Also add a bunch of tests of various flows through ResourceLoader, one of which is disabled due to yet another bug (Which I'll fix in another CL), and make the ResourceLoader tests use the same TestResourceHandler that the MIME sniffing / intercepting ResourceHandler tests use. BUG=669709 ==========
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
I'm perhaps going overboard with the EXPECTS, but I really don't trust the ResourceLoader code. https://codereview.chromium.org/2543633004/diff/40001/net/url_request/url_req... File net/url_request/url_request_test_job.h (right): https://codereview.chromium.org/2543633004/diff/40001/net/url_request/url_req... net/url_request/url_request_test_job.h:73: static GURL test_url_redirect_to_url_2(); It's not documented here, but we can't use test_url_redirect_to_url_2() because test_url_2() blocks after each callback, and needs to be explicitly resumed repeatedly. (I really hate this class, and want to eventually get rid of it, but have never had time to do so)
On 2016/12/01 16:37:18, mmenke wrote: > I'm perhaps going overboard with the EXPECTS, but I really don't trust the > ResourceLoader code. Worth noting it's not just the ResourceLoader code I don't trust, but the URLRequest/URLRequestJob API as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Which specific test failures prompted the ResourceLoader() change? I'm wanting to make sure I understand how those failures drove the non-test changes. (Only looked at RL so far.) https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:356: // the cancellation. I'm confused by this comment. We're dealing here with ResourceHandler cancellations, correct? They way I follow the code is that if the RH synchronously cancels the request, CompleteResponseStarted() will note that and cancel the request, and ResourceLoader::CancelRequestInternal will post a task to ResponseCompleted(). So I agree with avoiding ResponseCompleted() here, but I'm not sure the comment is accurate or precise, and I'd like to have it so to reduce the chances of these kind of errors creeping back in (though your refactor will help). If I'm missing something, let me know :-}. If I'm not, what about: // If the handler deferred the request, it will resume the request later. If the request was // cancelled, CompleteResponseStarted() above will have posted a task to call ResponseCompleted(). https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:566: // On error or cancellation, wait for notification of failure. I'm sure I'm missing a pathway, but ReadMore includes a call into URLRequest::Read(). If that fails synchronously, how do we find out? https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:566: // On error or cancellation, wait for notification of failure. How about "... wait for arrival of ResponseCompleted() task posted by Cancel()"? I'd like to at least gesture at the pathway by which the notification will occur.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Without the ResourceLoader changes, ResourceLoaderTest.SyncCancelOnWillRead, ResourceLoaderTest.SyncCancelOnResponseStarted and ResourceLoaderTest.RequestFailsOnReadSync tests all fail, due to double ResponseCompleted calls. They also continue to advance through ResourceHandler states unexpectedly after cancellation/failure. With only the line 565 fix, there are no double cancels, just some unexpected ResourceHandler calls after cancellation. https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:356: // the cancellation. On 2016/12/01 20:12:52, Randy Smith - Not in Mondays wrote: > I'm confused by this comment. We're dealing here with ResourceHandler > cancellations, correct? They way I follow the code is that if the RH > synchronously cancels the request, CompleteResponseStarted() will note that and > cancel the request, and ResourceLoader::CancelRequestInternal will post a task > to ResponseCompleted(). So I agree with avoiding ResponseCompleted() here, but > I'm not sure the comment is accurate or precise, and I'd like to have it so to > reduce the chances of these kind of errors creeping back in (though your > refactor will help). > > If I'm missing something, let me know :-}. If I'm not, what about: > > // If the handler deferred the request, it will resume the request later. If > the request was > // cancelled, CompleteResponseStarted() above will have posted a task to call > ResponseCompleted(). CompleteResponseStarted() will have cancelled the request, and the request will call back into us (With a read error...Which, yes, is weird). I've updated the wording. https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:566: // On error or cancellation, wait for notification of failure. On 2016/12/01 20:12:52, Randy Smith - Not in Mondays wrote: > I'm sure I'm missing a pathway, but ReadMore includes a call into > URLRequest::Read(). If that fails synchronously, how do we find out? > On synchronous failure, URLRequest::Read calls back into us on error, in addition of synchronously telling us about the failure. The followup CL (Which will re-enable the DISABLED test) will actually change that, and change how this code works. And hopefully not break anything.... I've audited all non-test URLRequest::Delegates. https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:566: // On error or cancellation, wait for notification of failure. On 2016/12/01 20:12:52, Randy Smith - Not in Mondays wrote: > How about "... wait for arrival of ResponseCompleted() task posted by Cancel()"? > I'd like to at least gesture at the pathway by which the notification will > occur. Note that Cancel only posts that task if the request is not "pending". i.e., if the last read that completed was not EOF. Otherwise, the URLRequest will call back into us to tell us about the cancellation. (In some cases, both happens, but that's why one of the tests is disabled)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
For the record, a) Based on the code tracing I've been doing I am completely down with the avalanche of EXPECTs you've put in, and b) Thank you for doing this--this is major goodness. I don't have any major issues at this point, but there are enough comments that I'd rather just do another round instead of conditionally stamping. You may well correct some misapprehension of mine, which would then prompt more review :-}. https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:356: // the cancellation. On 2016/12/01 20:47:43, mmenke wrote: > On 2016/12/01 20:12:52, Randy Smith - Not in Mondays wrote: > > I'm confused by this comment. We're dealing here with ResourceHandler > > cancellations, correct? They way I follow the code is that if the RH > > synchronously cancels the request, CompleteResponseStarted() will note that > and > > cancel the request, and ResourceLoader::CancelRequestInternal will post a task > > to ResponseCompleted(). So I agree with avoiding ResponseCompleted() here, > but > > I'm not sure the comment is accurate or precise, and I'd like to have it so to > > reduce the chances of these kind of errors creeping back in (though your > > refactor will help). > > > > If I'm missing something, let me know :-}. If I'm not, what about: > > > > // If the handler deferred the request, it will resume the request later. If > > the request was > > // cancelled, CompleteResponseStarted() above will have posted a task to call > > ResponseCompleted(). > > CompleteResponseStarted() will have cancelled the request, and the request will > call back into us (With a read error...Which, yes, is weird). I've updated the > wording. So just to confirm I understand what's going on: * If the request *wasn't* pending (which isn't true at this point in the code), RL::CancelRequestInternal, called from CompleteResponseStarted in the RH synchronous cancel case, will post a task to run RL::ResponseCompleted, which will execute *after* this code finishes. * The URLRequest::CancelWithError call within CompleteResponseStarted will (through DoCancel) call URLRequestJob::Kill, which will (NotifyCanceled -> NotifyDone) result in a PostTask of URLRJ::CompleteNotifyDone, which in the has_handled_response_ case will call request_->NotifyReadCompleted() which will end up back in RL, also after this code completes. * has_handled_response_ != was_pending so these two cases are not necessarily exclusive-but-one-will-happen. ? https://codereview.chromium.org/2543633004/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:566: // On error or cancellation, wait for notification of failure. On 2016/12/01 20:47:43, mmenke wrote: > On 2016/12/01 20:12:52, Randy Smith - Not in Mondays wrote: > > I'm sure I'm missing a pathway, but ReadMore includes a call into > > URLRequest::Read(). If that fails synchronously, how do we find out? > > > > On synchronous failure, URLRequest::Read calls back into us on error, in > addition of synchronously telling us about the failure. The followup CL (Which > will re-enable the DISABLED test) will actually change that, and change how this > code works. And hopefully not break anything.... I've audited all non-test > URLRequest::Delegates. Wow. I keep feeling like I've gotten to the bottom of the rabbit hole, and I keep being wrong. Will we have a reasonably easy-to-specify interface contract for URLRequest error signalling after this CL and the next one? If not, I'm probably going to want to request some documentation enhancement around what that contract actually is in one of those CLs. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/intercepting_resource_handler_unittest.cc:239: // entirely? I'm assuming you have some way of tracking all of these corner cases and making sure they don't get dropped on the floor? (This is a transformation of a "file a bug?" request that was my knee jerk reflex--I don't actually think that's the right answer, since there are so many of these small things, but I would like to make sure we don't lose track of them. And TODOs are better for code archeology than actually remembering to come back to something.) https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:380: // cancelled and either the request will call into |this| with a bogus The "If cancelled" seems to me to refer to the URLRequest and so "the URLRequest has already been canceled" seems redundant. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:382: // request was completed. If deferred, nothing to do until resumed. I think the last sentence is redundant with the first? https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:382: // request was completed. If deferred, nothing to do until resumed. Suggest changing to ", or, if the request was completed, a task posted from ResourceLoader::CancelRequestInternal will run OnResponseCompleted." (Deciding on how far down the rabbit hole to go in comments is tricky, and if you don't want that level of detail I'm good with that. I'm just reading the above and feeling like "a posted task" is missing some level of context.) https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:399: GURL test_url() const { nit, suggestion: Given the contrast below with test urls that don't redirect, this wrapping strikes me as obscuring more than it helps. Maybe get rid of it and hoist the n:URLRTJ:t_u_r_t_u() call up, or calling it "test_redirect_url" or something like it? (Suggestion because of spending some time trying to understand the "this doesn't need redirects" comment in ClientCertResourceLoaderTest::SetUp()). https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:479: nit, suggestion: It would feel more natural to me to have this next to SetUpResourceLoader(). https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:519: EXPECT_EQ(0, did_finish_loading_); Huh. I presume given the abundance of EXPECTs that this means that a DIdFinishLoading can be received without a previous DidStartRequest. [Later] And now I see the RequestFailsOnStart test below. Is failing on start the only case in which this happens? Is there documentation somewhere about that pathway? https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:754: // Spinning the message loop should not advance the state further. Hmmmm. This is testing to make sure that state doesn't go beyond the intended final state by spinning the message loop, but is not testing that nothing changes; it's possible that the WaitUntilDeferred() call didn't get as far as we expect and that the second spin catches the state up. I'm not sure whether I want to request the obvious change (check the state twice, before/after); it balloons the test, but given how wonky this code is, I'm not convinced that the failure mode I describe is unreasonable. Maybe think about some test helper routine that checks for expected state on the test & r_p_r_h variables all at once, so that only a couple of lines of code will be duplicated? I'll leave this one up to you, but I do want to raise the issue. I recognize I'm being paranoid, it's just that that feels more justified than usual :-}. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:933: TEST_F(ResourceLoaderTest, AsyncCancelOnWillStart) { Suggestion, nitty nit: I'm wanting to be precise in my descriptions of all this stuff since I'm finding it *really* easy to mis-think about it, and I'm specifically wanting to be precise about Sync vs. Async in-band vs. Async out-of-band. These tests are (just barely) Async out of band, so I'd rather call them AsyncCancelAfter* rather than AsyncCancelOn*. But I'm fine if you disagree--I'm mostly making the statement to keep the distinction clear in my mind as I read. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:1011: // version of this test for explanation. FWIW, I read over the explanation on the sync test twice and haven't figured out how it explains the double spin. I presume the first one is to move the request through until Eof is reached, and the second is to handle the PostTask that CancelWithError() generates? And that we don't get a double completion because we aren't going through the sync return pathway in the ResourceLoader, which is what dispatches the extra completion message? https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/test_resource_handler.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/test_resource_handler.cc:171: controller_->Resume(); Would it make sense to have an assertion/expectation that the resource handler previously deferred? https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/test_resource_handler.cc:175: controller_->CancelWithError(net_error); Should this set canceled_?
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please let me know if I've missed anything, or if you want more of a followup to any response. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/intercepting_resource_handler_unittest.cc:239: // entirely? On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > I'm assuming you have some way of tracking all of these corner cases and making > sure they don't get dropped on the floor? (This is a transformation of a "file > a bug?" request that was my knee jerk reflex--I don't actually think that's the > right answer, since there are so many of these small things, but I would like to > make sure we don't lose track of them. And TODOs are better for code archeology > than actually remembering to come back to something.) Erm...Not really. I think there's a high chance of this getting dropped on the floor, actually, unless I figure out a good way to go the removal route (The problem with removing is the mime sniffer - it needs to know when things are done to delay teardown. It could just delay teardown instead, like the RedirectToFileHandler does, though if I had to choose between the two, I'd go with delaying the 0-byte read). https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:380: // cancelled and either the request will call into |this| with a bogus On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > The "If cancelled" seems to me to refer to the URLRequest and so "the URLRequest > has already been canceled" seems redundant. Done. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:382: // request was completed. If deferred, nothing to do until resumed. On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > Suggest changing to ", or, if the request was completed, a task posted from > ResourceLoader::CancelRequestInternal will run OnResponseCompleted." > > (Deciding on how far down the rabbit hole to go in comments is tricky, and if > you don't want that level of detail I'm good with that. I'm just reading the > above and feeling like "a posted task" is missing some level of context.) Done. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:382: // request was completed. If deferred, nothing to do until resumed. On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > I think the last sentence is redundant with the first? Done. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:399: GURL test_url() const { On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > nit, suggestion: Given the contrast below with test urls that don't redirect, > this wrapping strikes me as obscuring more than it helps. Maybe get rid of it > and hoist the n:URLRTJ:t_u_r_t_u() call up, or calling it "test_redirect_url" or > something like it? > > (Suggestion because of spending some time trying to understand the "this doesn't > need redirects" comment in ClientCertResourceLoaderTest::SetUp()). Done - called it test_redirect_url(), and hoisted the other URL up here, too, calling it test_direct_url(). https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:479: On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > nit, suggestion: It would feel more natural to me to have this next to > SetUpResourceLoader(). It's down here to it doesn't interrupt the two testing::Test overrides. I've moved it above SetUp, but happy to move elsewhere. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:519: EXPECT_EQ(0, did_finish_loading_); On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > Huh. I presume given the abundance of EXPECTs that this means that a > DIdFinishLoading can be received without a previous DidStartRequest. [Later] > And now I see the RequestFailsOnStart test below. Is failing on start the only > case in which this happens? Is there documentation somewhere about that > pathway? There's no documentation. I believe these calls were added just to avoid a direct dependency from the ResourceLoader on the RDH, or maybe just so these tests don't need a real RDH. The calls between ResourceLoader and RDH were just abstracted away. DidStartRequest is just used for the little status display at the lower left, to make sure the update timer is running (So unstarted requests don't matter, though they don't really hurt). DidFinishLoading does cleanup, so always has to be called, or we have a leak. I'd like to revisit this API a bit at some point, but one thing at a time. :( DidStartRequest would probably be pretty easy to remove, at least, but probably want to put that off until we decide how to handle things in a post-mojo world. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:754: // Spinning the message loop should not advance the state further. On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > Hmmmm. This is testing to make sure that state doesn't go beyond the intended > final state by spinning the message loop, but is not testing that nothing > changes; it's possible that the WaitUntilDeferred() call didn't get as far as we > expect and that the second spin catches the state up. > > I'm not sure whether I want to request the obvious change (check the state > twice, before/after); it balloons the test, but given how wonky this code is, > I'm not convinced that the failure mode I describe is unreasonable. Maybe think > about some test helper routine that checks for expected state on the test & > r_p_r_h variables all at once, so that only a couple of lines of code will be > duplicated? > > I'll leave this one up to you, but I do want to raise the issue. I recognize > I'm being paranoid, it's just that that feels more justified than usual :-}. I've just copied the "most imporant" EXPECT_EQ(1...) test before the extra wait on all of these, though I'm not sure they get us anything. Seems like they're only testing that the TestResourceHandler defers in the method we told it to. Similar checks make no sense in the cancel tests - those all wait until completion, which makes the on_response_completed_called check already pretty redundant. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:933: TEST_F(ResourceLoaderTest, AsyncCancelOnWillStart) { On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > Suggestion, nitty nit: I'm wanting to be precise in my descriptions of all this > stuff since I'm finding it *really* easy to mis-think about it, and I'm > specifically wanting to be precise about Sync vs. Async in-band vs. Async > out-of-band. These tests are (just barely) Async out of band, so I'd rather > call them AsyncCancelAfter* rather than AsyncCancelOn*. But I'm fine if you > disagree--I'm mostly making the statement to keep the distinction clear in my > mind as I read. I'd call these async in-band. The async is triggered by the OnWillStart, but asynchronously, and it's blocking the request in the meantime. I consider OnWillStart to last from when the first ResourceHandler's OnWillStart method is called to when the last one's OnWillStart method completes. Neither view is right or wrong, just think using "after" is more confusing here. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:1011: // version of this test for explanation. On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > FWIW, I read over the explanation on the sync test twice and haven't figured out > how it explains the double spin. I presume the first one is to move the request > through until Eof is reached, and the second is to handle the PostTask that > CancelWithError() generates? And that we don't get a double completion because > we aren't going through the sync return pathway in the ResourceLoader, which is > what dispatches the extra completion message? The issue is URLRequest/URLRequestJob. If you look at URLRequestJob::NotifyDone, which is called on completion and on failure, it posts a task to run URLRequestJob::CompleteNotifyDone asynchronously. URLRequestJob::CompleteNotifyDone checks if the request failed, and if so, tells us about a failed read. So what happens it URLRequestJob::Read sees the request succeeded, it calls NotifyDone() and returns telling us the read succeeded, with 0 bytes read, and we're done. NotifyDone() posts a task to call CompleteNotifyDone. If the request isn't canceled, CompleteNotifyDone sees the request succeeded, and does nothing. If the request is cancelled before CompleteNotifyDone runs, URLRequestJob sees the error and tells us about the failure. So the extra message loop spin is to wait for CompleteNotifyDone to complete. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/test_resource_handler.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/test_resource_handler.cc:171: controller_->Resume(); On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > Would it make sense to have an assertion/expectation that the resource handler > previously deferred? Maybe in a followup CL? This one is already kinda big, and I have a 3rd CL where I beef up testing a little. Or do you want it before landing my ResourceLoader changes? https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/test_resource_handler.cc:175: controller_->CancelWithError(net_error); On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > Should this set canceled_? Done (And missed doing that in the redirect case, too)
Just realized we need more tests, I'll add them in another CL. In all of these tests, URLRequest::Read completes synchronously, so we need eof and non-eof async read tests (For the sync/async, sync/async cancel cases), which I think is 6 more tests? I'll add them in a followup CL.
https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/test_resource_handler.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/test_resource_handler.cc:171: controller_->Resume(); On 2016/12/02 21:26:52, mmenke wrote: > On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > > Would it make sense to have an assertion/expectation that the resource handler > > previously deferred? > > Maybe in a followup CL? This one is already kinda big, and I have a 3rd CL > where I beef up testing a little. Or do you want it before landing my > ResourceLoader changes? I mean my ResourceLoader in this CL - would want to make the change before my major refactor... Though my refactor would remove all those checks, anyways. Maybe not worth it?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM; notes below are suggestions. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/intercepting_resource_handler_unittest.cc:239: // entirely? On 2016/12/02 21:26:52, mmenke wrote: > On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > > I'm assuming you have some way of tracking all of these corner cases and > making > > sure they don't get dropped on the floor? (This is a transformation of a > "file > > a bug?" request that was my knee jerk reflex--I don't actually think that's > the > > right answer, since there are so many of these small things, but I would like > to > > make sure we don't lose track of them. And TODOs are better for code > archeology > > than actually remembering to come back to something.) > > Erm...Not really. I think there's a high chance of this getting dropped on the > floor, actually, unless I figure out a good way to go the removal route (The > problem with removing is the mime sniffer - it needs to know when things are > done to delay teardown. It could just delay teardown instead, like the > RedirectToFileHandler does, though if I had to choose between the two, I'd go > with delaying the 0-byte read). How would you feel about creating a bug something like "clean up sharp edges on the ResourceHandler API" and just use it to store notes about this sorta stuff? Or dumping things like this you run into into one of the bugs you already have? https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:1011: // version of this test for explanation. On 2016/12/02 21:26:52, mmenke wrote: > On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > > FWIW, I read over the explanation on the sync test twice and haven't figured > out > > how it explains the double spin. I presume the first one is to move the > request > > through until Eof is reached, and the second is to handle the PostTask that > > CancelWithError() generates? And that we don't get a double completion > because > > we aren't going through the sync return pathway in the ResourceLoader, which > is > > what dispatches the extra completion message? > > The issue is URLRequest/URLRequestJob. If you look at > URLRequestJob::NotifyDone, which is called on completion and on failure, it > posts a task to run URLRequestJob::CompleteNotifyDone asynchronously. > URLRequestJob::CompleteNotifyDone checks if the request failed, and if so, tells > us about a failed read. > > So what happens it URLRequestJob::Read sees the request succeeded, it calls > NotifyDone() and returns telling us the read succeeded, with 0 bytes read, and > we're done. NotifyDone() posts a task to call CompleteNotifyDone. If the > request isn't canceled, CompleteNotifyDone sees the request succeeded, and does > nothing. If the request is cancelled before CompleteNotifyDone runs, > URLRequestJob sees the error and tells us about the failure. So the extra > message loop spin is to wait for CompleteNotifyDone to complete. That makes sense, but I still think there's a lacuna in the commenting. Specifically, the comment on the Sync test doesn't explain that the notification from the URLRequestJob is done through a PostTask, and thus that we have to spin the message loop to get it. Willing to add a note about that (can just be an addendum here that mentions the notification mentioned in the sync case is done through a PostTask)? https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/test_resource_handler.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/test_resource_handler.cc:171: controller_->Resume(); On 2016/12/02 21:39:54, mmenke wrote: > On 2016/12/02 21:26:52, mmenke wrote: > > On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > > > Would it make sense to have an assertion/expectation that the resource > handler > > > previously deferred? > > > > Maybe in a followup CL? This one is already kinda big, and I have a 3rd CL > > where I beef up testing a little. Or do you want it before landing my > > ResourceLoader changes? > > I mean my ResourceLoader in this CL - would want to make the change before my > major refactor... Though my refactor would remove all those checks, anyways. > Maybe not worth it? Hmmm. Good point. Let's not worry about it until after your refactor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the review! I'll file a bug about the EOF-inconsistency. https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2543633004/diff/80001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:1011: // version of this test for explanation. On 2016/12/06 20:46:44, Randy Smith - Not in Mondays wrote: > On 2016/12/02 21:26:52, mmenke wrote: > > On 2016/12/02 20:38:55, Randy Smith - Not in Mondays wrote: > > > FWIW, I read over the explanation on the sync test twice and haven't figured > > out > > > how it explains the double spin. I presume the first one is to move the > > request > > > through until Eof is reached, and the second is to handle the PostTask that > > > CancelWithError() generates? And that we don't get a double completion > > because > > > we aren't going through the sync return pathway in the ResourceLoader, which > > is > > > what dispatches the extra completion message? > > > > The issue is URLRequest/URLRequestJob. If you look at > > URLRequestJob::NotifyDone, which is called on completion and on failure, it > > posts a task to run URLRequestJob::CompleteNotifyDone asynchronously. > > URLRequestJob::CompleteNotifyDone checks if the request failed, and if so, > tells > > us about a failed read. > > > > So what happens it URLRequestJob::Read sees the request succeeded, it calls > > NotifyDone() and returns telling us the read succeeded, with 0 bytes read, and > > we're done. NotifyDone() posts a task to call CompleteNotifyDone. If the > > request isn't canceled, CompleteNotifyDone sees the request succeeded, and > does > > nothing. If the request is cancelled before CompleteNotifyDone runs, > > URLRequestJob sees the error and tells us about the failure. So the extra > > message loop spin is to wait for CompleteNotifyDone to complete. > > That makes sense, but I still think there's a lacuna in the commenting. > Specifically, the comment on the Sync test doesn't explain that the notification > from the URLRequestJob is done through a PostTask, and thus that we have to spin > the message loop to get it. Willing to add a note about that (can just be an > addendum here that mentions the notification mentioned in the sync case is done > through a PostTask)? The next CL in this chain removes the extra spin, and fixes the issue (Amdittedly, there's still some ugliness in URLRequestJob to avoid this case, which will hopefully go away once I rework the suspend mode stuff). Anyhow, given that I already have a CL out for review that removes this extra spin, doesn't seem worth commenting on.
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481058602645750,
"parent_rev": "632b10ee27d8593418f4c5cdbbff88f501edabe3", "commit_rev":
"ef19fd55212094517b56a7b3142faa9aebfea274"}
Message was sent while issue was closed.
Description was changed from ========== Fix a pair of ResourceLoader cancellation/error bugs. Both of these would result in two completion messages being passed down the ResourceHandler chain. Also add a bunch of tests of various flows through ResourceLoader, one of which is disabled due to yet another bug (Which I'll fix in another CL), and make the ResourceLoader tests use the same TestResourceHandler that the MIME sniffing / intercepting ResourceHandler tests use. BUG=669709 ========== to ========== Fix a pair of ResourceLoader cancellation/error bugs. Both of these would result in two completion messages being passed down the ResourceHandler chain. Also add a bunch of tests of various flows through ResourceLoader, one of which is disabled due to yet another bug (Which I'll fix in another CL), and make the ResourceLoader tests use the same TestResourceHandler that the MIME sniffing / intercepting ResourceHandler tests use. BUG=669709 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix a pair of ResourceLoader cancellation/error bugs. Both of these would result in two completion messages being passed down the ResourceHandler chain. Also add a bunch of tests of various flows through ResourceLoader, one of which is disabled due to yet another bug (Which I'll fix in another CL), and make the ResourceLoader tests use the same TestResourceHandler that the MIME sniffing / intercepting ResourceHandler tests use. BUG=669709 ========== to ========== Fix a pair of ResourceLoader cancellation/error bugs. Both of these would result in two completion messages being passed down the ResourceHandler chain. Also add a bunch of tests of various flows through ResourceLoader, one of which is disabled due to yet another bug (Which I'll fix in another CL), and make the ResourceLoader tests use the same TestResourceHandler that the MIME sniffing / intercepting ResourceHandler tests use. BUG=669709 Committed: https://crrev.com/1d5e2e389193fc814291e64469a9b28d3412a9e9 Cr-Commit-Position: refs/heads/master@{#436732} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1d5e2e389193fc814291e64469a9b28d3412a9e9 Cr-Commit-Position: refs/heads/master@{#436732}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2557433005/ by carlosk@chromium.org. The reason for reverting is: Preemptively reverting because this seems to be the most likely cause of these webkit_tests crashing: http/tests/serviceworker/registration.html virtual/disable-mojo-service-worker/http/tests/serviceworker/register-link-element.html http/tests/serviceworker/chromium/register-error-messages.html virtual/mojo-loading/http/tests/serviceworker/register-link-element.html virtual/disable-mojo-service-worker/http/tests/serviceworker/registration.html virtual/service-worker-navigation-preload/http/tests/serviceworker/register-link-element.html virtual/service-worker-navigation-preload/http/tests/serviceworker/registration.html http/tests/serviceworker/register-link-element.html virtual/disable-mojo-service-worker/http/tests/serviceworker/chromium/register-error-messages.html virtual/service-worker-navigation-preload/http/tests/serviceworker/chromium/register-error-messages.html virtual/mojo-loading/http/tests/serviceworker/chromium/register-error-messages.html virtual/mojo-loading/http/tests/serviceworker/registration.html If this is not the actual root I'll re-revert it back..
Message was sent while issue was closed.
Example failures: - https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1... - https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1... - https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu... - https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7...
Message was sent while issue was closed.
FYI the revert did fix those test crashes. (it also caused iOS build errors that caused the tree to close probably due to some crazy combination of #include changes (which I could not pinpoint). But that's a different story... :) )
Message was sent while issue was closed.
On 2016/12/07 00:38:13, carlosk wrote: > FYI the revert did fix those test crashes. > > (it also caused iOS build errors that caused the tree to close probably due to > some crazy combination of #include changes (which I could not pinpoint). But > that's a different story... :) ) Hrm...Only test_url_request_context is built on iOS, and that doesn't modify includes. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
