Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563163002:
Add some tests for ThrottlingResourceHandler. (Closed)
|
Created:
4 years ago by mmenke Modified:
3 years, 11 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd some tests for ThrottlingResourceHandler.
Also introduce a new wrapper class for testing ResourceHandlers,
which will be useful to help make an upcoming API chance a bit simpler.
Also add support for on ResourceThrottle calling Resume() after another
does an out-of-band-cancel. We have nothing to prevent that case,
but a DCHECK could be triggered in the (unlikely) case it happened.
BUG=672581
Review-Url: https://codereview.chromium.org/2563163002
Cr-Commit-Position: refs/heads/master@{#443043}
Committed: https://chromium.googlesource.com/chromium/src/+/b4836c494c0ce6a8c5cf285736efc7cf1ae5fc91
Patch Set 1 #Patch Set 2 : More tests.... #Patch Set 3 : Another test #Patch Set 4 : Placate clang, add test, update comments #Patch Set 5 : net_error -> error_code, to match ResourceController API #Patch Set 6 : Merge #
Total comments: 18
Patch Set 7 : Response to comments #Patch Set 8 : Merge (Trivial merge conflict in includes) #Patch Set 9 : Fix merge #Patch Set 10 : Fix leaks #
Total comments: 5
Patch Set 11 : Add comments #
Dependent Patchsets: Messages
Total messages: 53 (38 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...
mmenke@chromium.org changed reviewers: + rdsmith@chromium.org
The test file is large, but the tests are mostly unexciting expects. The new tests can be updated to work with the new API by just updating my new test helper class, as intended. They also catch a bug in the other CL (Which I knew about, hence the tests), which is also good. My plan is to, in a followup CL (Or multiple CLs), update a lot of tests to use the new MockResourceLoader, and then (finally) proceed with the review of the big CL.
Description was changed from ========== Add some tests for ThrottlingResourceHandler. Also introduce a new wrapper class for testing ResourceHandlers, which will be useful to help make an upcoming API chance a bit simpler. BUG=672581 ========== to ========== Add some tests for ThrottlingResourceHandler. Also introduce a new wrapper class for testing ResourceHandlers, which will be useful to help make an upcoming API chance a bit simpler. Also add support for on ResourceThrottle calling Resume() after another does an out-of-band-cancel. We have nothing to prevent that case, but a DCHECK could be triggered in the (unlikely) case it happened. BUG=672581 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This looks good to me except for the change of the DCHECK to a conditional in ThrottlingResourceHandler and the follow-on concerns: Basically, since I don't like it, I want to understand the ResourceThrottle OOBC case better before I sign off on that. But the code and tests all look good (modulo my comments https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... File content/browser/loader/throttling_resource_handler.cc (right): https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler.cc:128: // currently active throttle called Cancel() or if it was another one. I'd really like to understand this use case better--as per my comment on the other CL, the NavigationResourceThrottle code looks to me as if it only cancels asynchronously, rather than OOB. Can you give me a pointer to a case where this happens that I can reproduce and poke at in a debugger? (The fact that this used to be a DCHECK() and is now a conditional suggests to me it's not easy to do.) https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... File content/browser/loader/throttling_resource_handler_unittest.cc (right): https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:199: // passed through to the TestResourceHandler, so is uninterested for these nit: "is uninterested" ? https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:350: // asynchronously, for a total of 6 combinations. However: This comment is true if all that is being tested is the cancel case; I think it would be clearer if that was explicitly called out in the comment. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:371: ASSERT_EQ(MockResourceLoader::Status::IDLE, Worth a comment that this the the "URLRequest" responding to being cancelled? Given that it's part of the protocol I could imagine us changing in the future, leaving breadcrumbs as to how to change the code might be worthwhile. (Just an idea, not pushing it.) https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:386: EXPECT_EQ(1, test_handler_->on_response_completed_called()); I wonder if it's worthwhile putting in a helper routine that expects that nothing's changed except for the test_handler_->on_response_compelted_called() before/after the OnResponseCompleted(ERR_UNEXPECTED) call? It seems like that's a conceptually clean thing to assert, and it would save a lot of code. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:422: // The first throttle also defers and then resumes the request. I presume this is because that's a "more complex" case than synchronously completing? https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:677: TEST_F(ThrottlingResourceHandlerTest, OutOfBandCancelBeforeWillStart) { As noted elsewhere, I'd like to understand the OOBC from throttles use cases in the current Chromium code; it seems extra weird to me, and part of me would rather move towards forbidding it than thoroughly testing it.
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...
Finally have time to get back to this stuff. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... File content/browser/loader/throttling_resource_handler.cc (right): https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler.cc:128: // currently active throttle called Cancel() or if it was another one. On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > I'd really like to understand this use case better--as per my comment on the > other CL, the NavigationResourceThrottle code looks to me as if it only cancels > asynchronously, rather than OOB. Can you give me a pointer to a case where this > happens that I can reproduce and poke at in a debugger? (The fact that this > used to be a DCHECK() and is now a conditional suggests to me it's not easy to > do.) I was wrong about NavigationResourceThrottle (It's the NavigationResourceHandler that does it out of band, so isn't a problem here). *However* it looks like SafeBrowsingResourceThrottle does out-of-band checks on Android (When database_manager_->ChecksAreAlwaysAsync() is true). When the result comes it, it cancels the request if it's a prefetch, and shows a blocking page otherwise. So that's a pretty rare case. However, ResourceThrottle cancels aren't too common, so I could well see other ones that do that. While, longer term, I'm fine with banning the practice, if we do that, we should crash when the condition is violated. DCHECKs on rare races just aren't good enough (Note that to trigger, the DCHECK would have needed the SafeBrowsingThrottle to non-blockingly asynchonously check a prefetch resource in OnWillStartRequest, the request to advance to WillProcessResponse/WillRedirect, then either the SafeBrowsingThrottle or another ResourceThrottle cancel the request, then either while posting the teardown task, or after the RedirectToFileHandler deferred destruction, another resource throttle to cancel the request. So yea...It's not common. Even if other throttles do this more often than the SafeBrowsing one, it's still an unlikely condition to hit, but as-is, code's not correct. You suggested I planned to add OutOfBandCancel to ResourceThrottle: I'm actually not sure if we should do that. It should be a brain-dead API that external consumers can use. I think it may be better to just have the ThrottlingResourceHandler handle OOB cancels (Possibly by just waiting for them to be in-band cancels, or be asynchronously doing in-band cancels). I'd also be fine with forbitting it, as long as we turned it into a crash. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... File content/browser/loader/throttling_resource_handler_unittest.cc (right): https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:199: // passed through to the TestResourceHandler, so is uninterested for these On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > nit: "is uninterested" ? Done. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:350: // asynchronously, for a total of 6 combinations. However: On 2016/12/20 22:16:42, Randy Smith - Not in Mondays wrote: > This comment is true if all that is being tested is the cancel case; I think it > would be clearer if that was explicitly called out in the comment. Done. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:371: ASSERT_EQ(MockResourceLoader::Status::IDLE, On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > Worth a comment that this the the "URLRequest" responding to being cancelled? > Given that it's part of the protocol I could imagine us changing in the future, > leaving breadcrumbs as to how to change the code might be worthwhile. (Just an > idea, not pushing it.) Done. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:386: EXPECT_EQ(1, test_handler_->on_response_completed_called()); On 2016/12/20 22:16:42, Randy Smith - Not in Mondays wrote: > I wonder if it's worthwhile putting in a helper routine that expects that > nothing's changed except for the test_handler_->on_response_compelted_called() > before/after the OnResponseCompleted(ERR_UNEXPECTED) call? It seems like that's > a conceptually clean thing to assert, and it would save a lot of code. TestHandler already does that, but I'm not sure that saves us anything - that doesn't catch the case that OnWillStart was unexpected called before OnResponseCompleted was called in this test, for instance. TestHandler does have enough checks that if on_will_start_called is 0, we don't need to check request_redirected or response_started (And TestThrottles have the same property for will_start_request_called() and the next two), but seemed most readable and least regression-prone (When copying/pasting to make new tests) to just keep all 10 checks at the end of every request. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:422: // The first throttle also defers and then resumes the request. On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > I presume this is because that's a "more complex" case than synchronously > completing? It's so that it's a sync cancel with ThrottlingResourceHandler::Resume() at the top of the callstack instead of ThrottlingResourceHandler::OnWillStart(), like the test case where the first one cancels synchronously. Added a comment about that to all 3 of these tests. (This is somewhat hinted at by the long comment before these tests, but not directly stated - elaborated there, too) I think we definitely need tests for sync cancel on Resume() case. I can add 3 tests for second throttle cancel sync cancel after first throttle sync succeeds, just didn't add them because I wasn't sure how useful they'd be, and because each of these tests is huge. :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mmenke@chromium.org to run a CQ dry run
Patchset #8 (id:140001) has been deleted
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... File content/browser/loader/throttling_resource_handler.cc (right): https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler.cc:128: // currently active throttle called Cancel() or if it was another one. On 2017/01/04 22:02:29, mmenke wrote: > On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > > I'd really like to understand this use case better--as per my comment on the > > other CL, the NavigationResourceThrottle code looks to me as if it only > cancels > > asynchronously, rather than OOB. Can you give me a pointer to a case where > this > > happens that I can reproduce and poke at in a debugger? (The fact that this > > used to be a DCHECK() and is now a conditional suggests to me it's not easy to > > do.) > > I was wrong about NavigationResourceThrottle (It's the NavigationResourceHandler > that does it out of band, so isn't a problem here). *However* it looks like > SafeBrowsingResourceThrottle does out-of-band checks on Android (When > database_manager_->ChecksAreAlwaysAsync() is true). > > When the result comes it, it cancels the request if it's a prefetch, and shows a > blocking page otherwise. So that's a pretty rare case. However, > ResourceThrottle cancels aren't too common, so I could well see other ones that > do that. While, longer term, I'm fine with banning the practice, if we do that, > we should crash when the condition is violated. DCHECKs on rare races just > aren't good enough (Note that to trigger, the DCHECK would have needed the > SafeBrowsingThrottle to non-blockingly asynchonously check a prefetch resource > in OnWillStartRequest, the request to advance to > WillProcessResponse/WillRedirect, then either the SafeBrowsingThrottle or > another ResourceThrottle cancel the request, then either while posting the > teardown task, or after the RedirectToFileHandler deferred destruction, another > resource throttle to cancel the request. > > So yea...It's not common. Even if other throttles do this more often than the > SafeBrowsing one, it's still an unlikely condition to hit, but as-is, code's not > correct. > > You suggested I planned to add OutOfBandCancel to ResourceThrottle: I'm > actually not sure if we should do that. It should be a brain-dead API that > external consumers can use. I think it may be better to just have the > ThrottlingResourceHandler handle OOB cancels (Possibly by just waiting for them > to be in-band cancels, or be asynchronously doing in-band cancels). I'd also be > fine with forbitting it, as long as we turned it into a crash. Sorry, I was wrong. For the problematic case, have to have the SafeBrowsingThrottle cancel out of band and then another ResourceController resume. Another problematic case is if one throttle cancels out of band and then we get a OnResponseStarted call. The reason I didn't fix that is we won't get a crash (At least in this RT) in that case when we switch to the new API. In the OOB-cancel and then resume case, we will get one, if we were deferred during the OOB cancel.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping! (For the MojoAsyncResourceHandler tests, too. Feel free to put off the big one). I'm planning to add some tests for RedirectToFileResourceHandler using the new test class, since I think I've found a bug in it. Also be nice to have more tests for my big refactor.
On 2017/01/06 20:40:07, mmenke wrote: > Friendly ping! (For the MojoAsyncResourceHandler tests, too. Feel free to put > off the big one). > > I'm planning to add some tests for RedirectToFileResourceHandler using the new > test class, since I think I've found a bug in it. Also be nice to have more > tests for my big refactor. Sorry, getting back to this now. Tonight or early tomorrow.
On 2017/01/10 23:08:29, Randy Smith - Not in Mondays wrote: > On 2017/01/06 20:40:07, mmenke wrote: > > Friendly ping! (For the MojoAsyncResourceHandler tests, too. Feel free to > put > > off the big one). > > > > I'm planning to add some tests for RedirectToFileResourceHandler using the new > > test class, since I think I've found a bug in it. Also be nice to have more > > tests for my big refactor. > > Sorry, getting back to this now. Tonight or early tomorrow. Nothing to apologize for, I'm in no rush.
Only remaining concern is the use-after-free comment below. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... File content/browser/loader/throttling_resource_handler.cc (right): https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler.cc:128: // currently active throttle called Cancel() or if it was another one. On 2017/01/05 18:50:00, mmenke wrote: > On 2017/01/04 22:02:29, mmenke wrote: > > On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > > > I'd really like to understand this use case better--as per my comment on the > > > other CL, the NavigationResourceThrottle code looks to me as if it only > > cancels > > > asynchronously, rather than OOB. Can you give me a pointer to a case where > > this > > > happens that I can reproduce and poke at in a debugger? (The fact that this > > > used to be a DCHECK() and is now a conditional suggests to me it's not easy > to > > > do.) > > > > I was wrong about NavigationResourceThrottle (It's the > NavigationResourceHandler > > that does it out of band, so isn't a problem here). *However* it looks like > > SafeBrowsingResourceThrottle does out-of-band checks on Android (When > > database_manager_->ChecksAreAlwaysAsync() is true). > > > > When the result comes it, it cancels the request if it's a prefetch, and shows > a > > blocking page otherwise. So that's a pretty rare case. However, > > ResourceThrottle cancels aren't too common, so I could well see other ones > that > > do that. While, longer term, I'm fine with banning the practice, if we do > that, > > we should crash when the condition is violated. DCHECKs on rare races just > > aren't good enough (Note that to trigger, the DCHECK would have needed the > > SafeBrowsingThrottle to non-blockingly asynchonously check a prefetch resource > > in OnWillStartRequest, the request to advance to > > WillProcessResponse/WillRedirect, then either the SafeBrowsingThrottle or > > another ResourceThrottle cancel the request, then either while posting the > > teardown task, or after the RedirectToFileHandler deferred destruction, > another > > resource throttle to cancel the request. > > > > So yea...It's not common. Even if other throttles do this more often than the > > SafeBrowsing one, it's still an unlikely condition to hit, but as-is, code's > not > > correct. > > > > You suggested I planned to add OutOfBandCancel to ResourceThrottle: I'm > > actually not sure if we should do that. It should be a brain-dead API that > > external consumers can use. I think it may be better to just have the > > ThrottlingResourceHandler handle OOB cancels (Possibly by just waiting for > them > > to be in-band cancels, or be asynchronously doing in-band cancels). I'd also > be > > fine with forbitting it, as long as we turned it into a crash. > > Sorry, I was wrong. > > For the problematic case, have to have the SafeBrowsingThrottle cancel out of > band and then another ResourceController resume. Another problematic case is if > one throttle cancels out of band and then we get a OnResponseStarted call. The > reason I didn't fix that is we won't get a crash (At least in this RT) in that > case when we switch to the new API. In the OOB-cancel and then resume case, we > will get one, if we were deferred during the OOB cancel. Oh, what a mess. Ok. * For purposes of this CL, the current plan (just allow it on the normal Cancel interface) is ok. * Long term, my inclination is to forbid it and make things that need to do out of band cancel take some other path (e.g. record the information they need to tell the RDHI to cancel the request). This makes the systems that need to support the corner case take solid responsibility for it. * Your argument about CHECK vs. DCHECK makes sense to me. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... File content/browser/loader/throttling_resource_handler_unittest.cc (right): https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:386: EXPECT_EQ(1, test_handler_->on_response_completed_called()); On 2017/01/04 22:02:29, mmenke wrote: > On 2016/12/20 22:16:42, Randy Smith - Not in Mondays wrote: > > I wonder if it's worthwhile putting in a helper routine that expects that > > nothing's changed except for the test_handler_->on_response_compelted_called() > > before/after the OnResponseCompleted(ERR_UNEXPECTED) call? It seems like > that's > > a conceptually clean thing to assert, and it would save a lot of code. > > TestHandler already does that, but I'm not sure that saves us anything - that > doesn't catch the case that OnWillStart was unexpected called before > OnResponseCompleted was called in this test, for instance. > > TestHandler does have enough checks that if on_will_start_called is 0, we don't > need to check request_redirected or response_started (And TestThrottles have the > same property for will_start_request_called() and the next two), but seemed most > readable and least regression-prone (When copying/pasting to make new tests) to > just keep all 10 checks at the end of every request. Acknowledged. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:422: // The first throttle also defers and then resumes the request. On 2017/01/04 22:02:29, mmenke wrote: > On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > > I presume this is because that's a "more complex" case than synchronously > > completing? > > It's so that it's a sync cancel with ThrottlingResourceHandler::Resume() at the > top of the callstack instead of ThrottlingResourceHandler::OnWillStart(), like > the test case where the first one cancels synchronously. Added a comment about > that to all 3 of these tests. (This is somewhat hinted at by the long comment > before these tests, but not directly stated - elaborated there, too) > > I think we definitely need tests for sync cancel on Resume() case. I can add 3 > tests for second throttle cancel sync cancel after first throttle sync succeeds, > just didn't add them because I wasn't sure how useful they'd be, and because > each of these tests is huge. :( After several flip-flops, I agree with you--the different is in the code above the ThrottlingResourceHandler::On* routines, and there are strictly more such routines in the callstack in the Resume case. Ok. https://codereview.chromium.org/2563163002/diff/100001/content/browser/loader... content/browser/loader/throttling_resource_handler_unittest.cc:677: TEST_F(ThrottlingResourceHandlerTest, OutOfBandCancelBeforeWillStart) { On 2016/12/20 22:16:41, Randy Smith - Not in Mondays wrote: > As noted elsewhere, I'd like to understand the OOBC from throttles use cases in > the current Chromium code; it seems extra weird to me, and part of me would > rather move towards forbidding it than thoroughly testing it. As noted elsewhere, accepted for this CL. https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... File content/browser/loader/mock_resource_loader.cc (right): https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... content/browser/loader/mock_resource_loader.cc:55: response.get(), &defer); Ok, I'm sure there's something subtle I'm missing :-}, but doesn't this turn the leak into a use-after-free? The response will be deleted when the last scoped_refptr<> pointing to it goes out of scope, which looks to me to be at the end of this routine. If OnRequestRedirected results in a CALLBACK_PENDING state, doesn't that mean the resource_handler_ may have held onto a pointer to the response? (Same for next routine.)
https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... File content/browser/loader/mock_resource_loader.cc (right): https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... content/browser/loader/mock_resource_loader.cc:55: response.get(), &defer); On 2017/01/11 19:57:15, Randy Smith - Not in Mondays wrote: > Ok, I'm sure there's something subtle I'm missing :-}, but doesn't this turn the > leak into a use-after-free? The response will be deleted when the last > scoped_refptr<> pointing to it goes out of scope, which looks to me to be at the > end of this routine. If OnRequestRedirected results in a CALLBACK_PENDING > state, doesn't that mean the resource_handler_ may have held onto a pointer to > the response? > > (Same for next routine.) Great question! This is indeed rather unclear. I did this to mimic the real ResourceLoader's behavior - see https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c.... If ResourceHandlers process the event asynchronously, they should hold onto a pointer themselves if they want to keep it alive. I've added comments to both these methods. Think I'll modify the other two test CLs I've sent you to use std::move to pass in the ResourceResponse (Both text fixtures currently hand on to their own reference to the response when they call this method, currently), with more comments about it.
On 2017/01/11 20:39:14, mmenke wrote: > https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... > File content/browser/loader/mock_resource_loader.cc (right): > > https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... > content/browser/loader/mock_resource_loader.cc:55: response.get(), &defer); > On 2017/01/11 19:57:15, Randy Smith - Not in Mondays wrote: > > Ok, I'm sure there's something subtle I'm missing :-}, but doesn't this turn > the > > leak into a use-after-free? The response will be deleted when the last > > scoped_refptr<> pointing to it goes out of scope, which looks to me to be at > the > > end of this routine. If OnRequestRedirected results in a CALLBACK_PENDING > > state, doesn't that mean the resource_handler_ may have held onto a pointer to > > the response? > > > > (Same for next routine.) > > Great question! This is indeed rather unclear. > > I did this to mimic the real ResourceLoader's behavior - see > https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c.... > If ResourceHandlers process the event asynchronously, they should hold onto a > pointer themselves if they want to keep it alive. I've added comments to both > these methods. > > Think I'll modify the other two test CLs I've sent you to use std::move to pass > in the ResourceResponse (Both text fixtures currently hand on to their own > reference to the response when they call this method, currently), with more > comments about it. Oops, added the comments to one of the other CLs in the chain. I always do that. :( I've now added them to this one.
Ah, thanks! LGTM. (You can decide whether and when to act on my comment below.) https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... File content/browser/loader/mock_resource_loader.cc (right): https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... content/browser/loader/mock_resource_loader.cc:55: response.get(), &defer); On 2017/01/11 20:39:14, mmenke wrote: > On 2017/01/11 19:57:15, Randy Smith - Not in Mondays wrote: > > Ok, I'm sure there's something subtle I'm missing :-}, but doesn't this turn > the > > leak into a use-after-free? The response will be deleted when the last > > scoped_refptr<> pointing to it goes out of scope, which looks to me to be at > the > > end of this routine. If OnRequestRedirected results in a CALLBACK_PENDING > > state, doesn't that mean the resource_handler_ may have held onto a pointer to > > the response? > > > > (Same for next routine.) > > Great question! This is indeed rather unclear. > > I did this to mimic the real ResourceLoader's behavior - see > https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c.... > If ResourceHandlers process the event asynchronously, they should hold onto a > pointer themselves if they want to keep it alive. I've added comments to both > these methods. > > Think I'll modify the other two test CLs I've sent you to use std::move to pass > in the ResourceResponse (Both text fixtures currently hand on to their own > reference to the response when they call this method, currently), with more > comments about it. Shouldn't there be a comment to this effect in resource_handler.h before the routines that take a ResourceResponse? It's only slightly surprising that the RR can be deallocated on return from the call (One hand: I think of that as default for raw pointers, other hand: It's not clear how to think about incoming arguments when the request is deferred), but it's also slightly surprising that it's good practice in a resource handler to take a reference can be taken to keep it around (One hand: it's generally how scoped_refptr<>s work, other hand: It's generally *not* how raw pointers work, so it isn't clear from the interface that that's expected.) A comment to clarify all the surprises would be welcome (by me, at least).
Thanks for the review! https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... File content/browser/loader/mock_resource_loader.cc (right): https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... content/browser/loader/mock_resource_loader.cc:55: response.get(), &defer); On 2017/01/11 21:11:02, Randy Smith - Not in Mondays wrote: > On 2017/01/11 20:39:14, mmenke wrote: > > On 2017/01/11 19:57:15, Randy Smith - Not in Mondays wrote: > > > Ok, I'm sure there's something subtle I'm missing :-}, but doesn't this turn > > the > > > leak into a use-after-free? The response will be deleted when the last > > > scoped_refptr<> pointing to it goes out of scope, which looks to me to be at > > the > > > end of this routine. If OnRequestRedirected results in a CALLBACK_PENDING > > > state, doesn't that mean the resource_handler_ may have held onto a pointer > to > > > the response? > > > > > > (Same for next routine.) > > > > Great question! This is indeed rather unclear. > > > > I did this to mimic the real ResourceLoader's behavior - see > > > https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c.... > > If ResourceHandlers process the event asynchronously, they should hold onto a > > pointer themselves if they want to keep it alive. I've added comments to both > > these methods. > > > > Think I'll modify the other two test CLs I've sent you to use std::move to > pass > > in the ResourceResponse (Both text fixtures currently hand on to their own > > reference to the response when they call this method, currently), with more > > comments about it. > > Shouldn't there be a comment to this effect in resource_handler.h before the > routines that take a ResourceResponse? It's only slightly surprising that the > RR can be deallocated on return from the call (One hand: I think of that as > default for raw pointers, other hand: It's not clear how to think about incoming > arguments when the request is deferred), but it's also slightly surprising that > it's good practice in a resource handler to take a reference can be taken to > keep it around (One hand: it's generally how scoped_refptr<>s work, other hand: > It's generally *not* how raw pointers work, so it isn't clear from the interface > that that's expected.) A comment to clarify all the surprises would be welcome > (by me, at least). SGTM. I'll update the RH interface update CL with a comment with that information (Since that CL rewrites the interface, seems simpler to do it there, rather than do it here, merge up to the final CL, merge the comment, and rewrite the comment). May make sense to switch the argument to a scoped_refptr<ResourceResponse>, to make lifetimes clearer, but that CL does enough already. :)
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...
https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... File content/browser/loader/mock_resource_loader.cc (right): https://codereview.chromium.org/2563163002/diff/200001/content/browser/loader... content/browser/loader/mock_resource_loader.cc:55: response.get(), &defer); On 2017/01/11 21:16:37, mmenke wrote: > On 2017/01/11 21:11:02, Randy Smith - Not in Mondays wrote: > > On 2017/01/11 20:39:14, mmenke wrote: > > > On 2017/01/11 19:57:15, Randy Smith - Not in Mondays wrote: > > > > Ok, I'm sure there's something subtle I'm missing :-}, but doesn't this > turn > > > the > > > > leak into a use-after-free? The response will be deleted when the last > > > > scoped_refptr<> pointing to it goes out of scope, which looks to me to be > at > > > the > > > > end of this routine. If OnRequestRedirected results in a CALLBACK_PENDING > > > > state, doesn't that mean the resource_handler_ may have held onto a > pointer > > to > > > > the response? > > > > > > > > (Same for next routine.) > > > > > > Great question! This is indeed rather unclear. > > > > > > I did this to mimic the real ResourceLoader's behavior - see > > > > > > https://cs.chromium.org/chromium/src/content/browser/loader/resource_loader.c.... > > > If ResourceHandlers process the event asynchronously, they should hold onto > a > > > pointer themselves if they want to keep it alive. I've added comments to > both > > > these methods. > > > > > > Think I'll modify the other two test CLs I've sent you to use std::move to > > pass > > > in the ResourceResponse (Both text fixtures currently hand on to their own > > > reference to the response when they call this method, currently), with more > > > comments about it. > > > > Shouldn't there be a comment to this effect in resource_handler.h before the > > routines that take a ResourceResponse? It's only slightly surprising that the > > RR can be deallocated on return from the call (One hand: I think of that as > > default for raw pointers, other hand: It's not clear how to think about > incoming > > arguments when the request is deferred), but it's also slightly surprising > that > > it's good practice in a resource handler to take a reference can be taken to > > keep it around (One hand: it's generally how scoped_refptr<>s work, other > hand: > > It's generally *not* how raw pointers work, so it isn't clear from the > interface > > that that's expected.) A comment to clarify all the surprises would be > welcome > > (by me, at least). > > SGTM. I'll update the RH interface update CL with a comment with that > information (Since that CL rewrites the interface, seems simpler to do it there, > rather than do it here, merge up to the final CL, merge the comment, and rewrite > the comment). > > May make sense to switch the argument to a scoped_refptr<ResourceResponse>, to > make lifetimes clearer, but that CL does enough already. :) FWIW, I'd support the interface change (at some time, in some CL :-}); I think it makes it very likely that assumptions about lifetime and argument usage will be correct without needing to read the comment documentation.
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484169415005390,
"parent_rev": "9445305e2f3022015e983398fe3aa56feea9a418", "commit_rev":
"b4836c494c0ce6a8c5cf285736efc7cf1ae5fc91"}
Message was sent while issue was closed.
Description was changed from ========== Add some tests for ThrottlingResourceHandler. Also introduce a new wrapper class for testing ResourceHandlers, which will be useful to help make an upcoming API chance a bit simpler. Also add support for on ResourceThrottle calling Resume() after another does an out-of-band-cancel. We have nothing to prevent that case, but a DCHECK could be triggered in the (unlikely) case it happened. BUG=672581 ========== to ========== Add some tests for ThrottlingResourceHandler. Also introduce a new wrapper class for testing ResourceHandlers, which will be useful to help make an upcoming API chance a bit simpler. Also add support for on ResourceThrottle calling Resume() after another does an out-of-band-cancel. We have nothing to prevent that case, but a DCHECK could be triggered in the (unlikely) case it happened. BUG=672581 Review-Url: https://codereview.chromium.org/2563163002 Cr-Commit-Position: refs/heads/master@{#443043} Committed: https://chromium.googlesource.com/chromium/src/+/b4836c494c0ce6a8c5cf285736ef... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/b4836c494c0ce6a8c5cf285736ef... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
