Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698393002:
Allow asynchronous deferral in NavigationSimulator (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow asynchronous deferral in NavigationSimulator
This patch adds functionality to the testing class NavigationSimulator,
to allow for NavigationThrottles to defer and cancel the navigation.
The new functionality is tested by new NavigationSimulator unit tests,
as well as new subresource filter unit tests exercising the untested
SubframeNavigationFilteringThrottle.
BUG=637415
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2698393002
Cr-Commit-Position: refs/heads/master@{#455752}
Committed: https://chromium.googlesource.com/chromium/src/+/5da0c292e434236e9344a532471123912cdb1fe7
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : merge in subresource filter unit tests #Patch Set 4 : Clean up #Patch Set 5 : more cleanups #Patch Set 6 : null check #Patch Set 7 : simplify #Patch Set 8 : Fix non linux build #Patch Set 9 : Just return the throttle check on Start/Redirect/COmmit #
Total comments: 10
Patch Set 10 : clamy review #
Total comments: 7
Patch Set 11 : Remove the wait API #
Total comments: 13
Patch Set 12 : engedy/clamy review #Dependent Patchsets: Messages
Total messages: 66 (40 generated)
Description was changed from ========== Allow asynchronous deferral in NavigationSimulator BUG=TODO ========== to ========== Allow asynchronous deferral in NavigationSimulator BUG=TODO CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by csharrison@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: 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
csharrison@chromium.org changed reviewers: + clamy@chromium.org
Hey clamy@ Here's a draft of a CL augmenting NavigationSimulator. This particular patch actually ended up adding PlzNavigate (in NavigationRequest) and non-PlzNavigate callback paths. I think this aligns with the future of the callback, as we can just move to use the NavigationRequest callback post-PlzNavigate. I was thinking of adding something like NavigationThrottle::ThrottleCheckResult WaitForThrottleCheckResult() but it would entail making another step (FinishCommitting) in case of Asynchronous WillProcessResponse. Interested in what you think here.
Huh, I guess I missed an incantation to get the dependent PS up. In any case, the diff looks right. LMK high level what you think (even if the code isn't perfect), as I will likely have to write tests for [1], [2], and [3] using this approach if this is the best option. [1] https://codereview.chromium.org/2696493003/ [2] https://codereview.chromium.org/2697703002/ [3] https://codereview.chromium.org/2691423006/
The CQ bit was checked by csharrison@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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Thanks! I'm generally happy with the approach, since the non-auto advance mode was something I was planning to do in the future. I'll only do a full review when the dependent patch set lands if that's ok with you, since the patch itself is still changing a little bit, in particular, the final interface has not been fully defined yet.
Sounds good to me, thanks!
clamy: Please hold off reviewing this until I ping again. I'll need to do a proper merge now that NavigationSimulator is in content/public/test, plus I think I figured a way to do this in a simpler manner.
The CQ bit was checked by csharrison@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...
OK clamy, this is now ready for review. PTAL?
BTW: Feel free to only look at //content bits. I will get subresource filter reviewed by someone else.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by csharrison@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.
Thanks Charlie! I've started reviewing your patch, and have been thinking about the interface of the NavigationSimulator we ultimately want to provide. 1) Should we just always run NavigationThrottles in asynchronous fashion by default? If we want to properly simulate navigation events anyway, this would be the right thing to do. Honeestly the main reason I didn't do that in the first place is that it was more complicated and I wanted to land a first version of the NavigationSimulator ASAP. But your followup patch seems like a good place to switch to this behavior. 2) If we do that, how much do we need a separate WaitForThrottleChecksComplete method? Couldn't we just simply return a ThrottleCheckResult from Start, Redirect and Commit? That would be the ThrottleCheckresult we got when we executed WillStartRequest, WillRedirectRequest and WillProcessResponse. If that fits your use case, I think it'd be a lot simpler. 3) Getting NavigationHandle to notify us about what result it got is very subtle with PlzNavigate right now. I suggest a dumb solution of adding a specific test callback in NavigationHandleImpl that we would run just before the real callback. This way no issues with deletion of the Navigationhandle etc... Yes that makes two callbacks, but I think it's a lot easier to reason about.
On 2017/03/03 15:35:45, clamy wrote: > Thanks Charlie! I've started reviewing your patch, and have been thinking about > the interface of the NavigationSimulator we ultimately want to provide. > > 1) Should we just always run NavigationThrottles in asynchronous fashion by > default? If we want to properly simulate navigation events anyway, this would be > the right thing to do. Honeestly the main reason I didn't do that in the first > place is that it was more complicated and I wanted to land a first version of > the NavigationSimulator ASAP. But your followup patch seems like a good place to > switch to this behavior. Yes this is reasonable. I wanted your vision of things before taking an API stance on this :) > > 2) If we do that, how much do we need a separate WaitForThrottleChecksComplete > method? Couldn't we just simply return a ThrottleCheckResult from Start, > Redirect and Commit? That would be the ThrottleCheckresult we got when we > executed WillStartRequest, WillRedirectRequest and WillProcessResponse. If that > fits your use case, I think it'd be a lot simpler. Yes! People would know to call WaitForThrottleChecksComplete if they got a DEFER result. > > 3) Getting NavigationHandle to notify us about what result it got is very subtle > with PlzNavigate right now. I suggest a dumb solution of adding a specific test > callback in NavigationHandleImpl that we would run just before the real > callback. This way no issues with deletion of the Navigationhandle etc... Yes > that makes two callbacks, but I think it's a lot easier to reason about. OK, I think that would look something like the logic I added in NavigationRequest. Thank you for your high level comments. I can try to rework the CL to address these points.
The CQ bit was checked by csharrison@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...
OK Camille: I like this CL better now that it uses your suggestion. There is no more auto advance mode, which I think is a bit of a broken model anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+engedy for subresource filter bits. Note the tests are basically the same since your last look (back when this was with the impl).
csharrison@chromium.org changed reviewers: + engedy@chromium.org
+engedy for subresource filter bits. Note the tests are basically the same since your last look (back when this was with the impl).
Thanks! I still have a few more high-level questions below before digging in the details. https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.cc:516: if (IsBrowserSideNavigationEnabled()) { What do you think of the previous suggestion to store a special test callback in NavigationHandle. We can add it here, and we won't have to special case PlzNavigate or the current architecture when generating the callbacks. https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) In fact, that's not the interface I had in mind. What I wanted was for Start, Redirect and Commit to block until all asynchronous checks have been resolved. This way callers need not to worry about whether checks are asynchronous or not, and just get notified of what the end result was. So basically, that means folding WaitForThrottleChecksComplete in all of these methods, so that we always wait for the throttle checks to complete. Does this make sense? https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:139: // running in AllowThrottleChecks mode. We no longer have a non AllowThrottleChecks mode :).
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Adding a test callback on the nav handle turned out ok. PTAL and let me know what you'd like for a API. https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.cc:516: if (IsBrowserSideNavigationEnabled()) { On 2017/03/06 15:22:15, clamy wrote: > What do you think of the previous suggestion to store a special test callback in > NavigationHandle. We can add it here, and we won't have to special case > PlzNavigate or the current architecture when generating the callbacks. Done. https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) On 2017/03/06 15:22:15, clamy wrote: > In fact, that's not the interface I had in mind. What I wanted was for Start, > Redirect and Commit to block until all asynchronous checks have been resolved. > This way callers need not to worry about whether checks are asynchronous or not, > and just get notified of what the end result was. > > So basically, that means folding WaitForThrottleChecksComplete in all of these > methods, so that we always wait for the throttle checks to complete. Does this > make sense? As discussed offline, this isn't possible with how we've designed subresource filter unit tests. These tests post a series of messages on *another* task runner which must complete before the navigation is resumed. So our code basically does: Start(); blocking_task_runner_->RunUntilIdle(); WaitForThrottleChecksComplete(); And would hang if we omit the RunUntilIdle call. https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:139: // running in AllowThrottleChecks mode. On 2017/03/06 15:22:15, clamy wrote: > We no longer have a non AllowThrottleChecks mode :). Done.
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Thanks! I like the callback on the NavigationHandle better. I chatted with Nasko and we had some more suggestion on what the interface should look like, see below. Sorry for the back and forth on this patch, we really want to make sure that the interface is as simple to use as possible so people can write tests more easily. https://codereview.chromium.org/2698393002/diff/180001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/180001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) So I had a chat with nasko, and we agreed that having all callers need to check the return value of Start, etc.. was not something we wanted to have. Instead we suggest to have two kind of methods: Start and StartAndExpectDeferral (same with Redirect, etc... as neded). This way users who don't care about running another TaskRunner in the background will just called the simple Start method, while more advanced users can use StartAndExpectDeferral. Then we'd have a somewhat generic method called Resume that people can call to have whatever was deferred resume. I'm not quite sure on whether it should return the precise throttle check, or we should just query it from the NavigationSimulator, probably the latter. Does that make sense to you?
https://codereview.chromium.org/2698393002/diff/180001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/180001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) On 2017/03/07 15:18:14, clamy wrote: > So I had a chat with nasko, and we agreed that having all callers need to check > the return value of Start, etc.. was not something we wanted to have. > > Instead we suggest to have two kind of methods: Start and StartAndExpectDeferral > (same with Redirect, etc... as neded). This way users who don't care about > running another TaskRunner in the background will just called the simple Start > method, while more advanced users can use StartAndExpectDeferral. > > Then we'd have a somewhat generic method called Resume that people can call to > have whatever was deferred resume. I'm not quite sure on whether it should > return the precise throttle check, or we should just query it from the > NavigationSimulator, probably the latter. > > Does that make sense to you? Yes this API design sounds good to me. I agree checking each method is tedious and maybe the wrong abstraction level for a lot of tests. Regarding "just query it [the throttle check] from the NavigationSimulator", were you thinking of an API like: simulator->StartAndExpectDeferral() <dostuff> simulator->Resume() EXPECT_EQ(NavigationThrottle::CANCEL, simulator->GetLastThrottleCheckResult());
https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) > As discussed offline, this isn't possible with how we've designed subresource > filter unit tests. These tests post a series of messages on *another* task > runner which must complete before the navigation is resumed. So our code > basically does: > > Start(); > blocking_task_runner_->RunUntilIdle(); > WaitForThrottleChecksComplete(); > > And would hang if we omit the RunUntilIdle call. I found it practical to look at this from the following angle: what would be the best interface for the NavigationSimulator so that it can be nicely used to test two throttles at the same time (with both throttles doing async work in the same stage)? While the current approach indeed allows verifying that things take place on the correct threads, it is a bit awkward: > Start(); > blocking_task_runner1_->RunUntilIdle(); > base::RunLoop().RunUntilIdle(); // Invoke the next throttle. > blocking_task_runner2_->RunUntilIdle(); > WaitForThrottleChecksComplete(); It looks like that for a NavigationThrottles wishing to do async processing usually do it on: 1.) a TaskRunner specified at construction time (which can be set up in unit tests so that they are aliases of a single MessageLoop), 2.) Named content::BrowserThreads, which can be easily made real threads if there is no other solution, 3.) Blocking pools (not sure how these work with TestBrowserThreadBundle, but in the worst case, we could expose a method like [1] and let tests subclass NavigationSimulator, although I am not sure that's too nice either). The threading constraints could be checked at a different layer (or some other way, or not at all?), and then we would not need two flavors of these methods either. [1]: https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?r... https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:456: // careful about relying on it as it may be going away. nit: I am not sure I understand this comment.
https://codereview.chromium.org/2698393002/diff/180001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/180001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) On 2017/03/07 15:23:33, Charlie Harrison wrote: > On 2017/03/07 15:18:14, clamy wrote: > > So I had a chat with nasko, and we agreed that having all callers need to > check > > the return value of Start, etc.. was not something we wanted to have. > > > > Instead we suggest to have two kind of methods: Start and > StartAndExpectDeferral > > (same with Redirect, etc... as neded). This way users who don't care about > > running another TaskRunner in the background will just called the simple Start > > method, while more advanced users can use StartAndExpectDeferral. > > > > Then we'd have a somewhat generic method called Resume that people can call to > > have whatever was deferred resume. I'm not quite sure on whether it should > > return the precise throttle check, or we should just query it from the > > NavigationSimulator, probably the latter. > > > > Does that make sense to you? > > Yes this API design sounds good to me. I agree checking each method is tedious > and maybe the wrong abstraction level for a lot of tests. > > Regarding "just query it [the throttle check] from the NavigationSimulator", > were you thinking of an API like: > > simulator->StartAndExpectDeferral() > <dostuff> > simulator->Resume() > EXPECT_EQ(NavigationThrottle::CANCEL, simulator->GetLastThrottleCheckResult()); > Yes. That seems quite easy to use IMO.
https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) On 2017/03/07 15:24:56, engedy (slow) wrote: > > As discussed offline, this isn't possible with how we've designed subresource > > filter unit tests. These tests post a series of messages on *another* task > > runner which must complete before the navigation is resumed. So our code > > basically does: > > > > Start(); > > blocking_task_runner_->RunUntilIdle(); > > WaitForThrottleChecksComplete(); > > > > And would hang if we omit the RunUntilIdle call. > > I found it practical to look at this from the following angle: what would be the > best interface for the NavigationSimulator so that it can be nicely used to test > two throttles at the same time (with both throttles doing async work in the same > stage)? > > While the current approach indeed allows verifying that things take place on the > correct threads, it is a bit awkward: > > > Start(); > > blocking_task_runner1_->RunUntilIdle(); > > base::RunLoop().RunUntilIdle(); // Invoke the next throttle. > > blocking_task_runner2_->RunUntilIdle(); > > WaitForThrottleChecksComplete(); > > It looks like that for a NavigationThrottles wishing to do async processing > usually do it on: > 1.) a TaskRunner specified at construction time (which can be set up in unit > tests so that they are aliases of a single MessageLoop), > 2.) Named content::BrowserThreads, which can be easily made real threads if > there is no other solution, > 3.) Blocking pools (not sure how these work with TestBrowserThreadBundle, but > in the worst case, we could expose a method like [1] and let tests subclass > NavigationSimulator, although I am not sure that's too nice either). > > The threading constraints could be checked at a different layer (or some other > way, or not at all?), and then we would not need two flavors of these methods > either. > > [1]: > https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?r... I see what you are saying. I am in favor of this approach (1) in this CL. If consumers need (2) or (3) and don't like the proposed solutions we can think of making NavigationSimulator more complicated. https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:456: // careful about relying on it as it may be going away. On 2017/03/07 15:24:56, engedy (slow) wrote: > nit: I am not sure I understand this comment. This was indicated by clamy in the subframe navigation filtering throttle CL: "I really don't want to be modifying the semantic of the callback right now. We will do it in the future though, where either NavigationHandle will be merged in the NavigationRequest or NavigationRequest methods will be called directly by NavigationHandle. This will result in the callback disappearing." This is why we are adding a new for_testing_ callback instead of reusing this one.
The CQ bit was checked by csharrison@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...
Ok, I have followed engedys advice and just used a single task runner for the subresource filter unit tests. PTAL. This simplifies the API / code for this change. engedy: Please let me know if I misunderstood you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % comments. Thanks and sorry for the extreme delay! https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) On 2017/03/07 16:42:43, Charlie Harrison wrote: > On 2017/03/07 15:24:56, engedy (slow) wrote: > > > As discussed offline, this isn't possible with how we've designed > subresource > > > filter unit tests. These tests post a series of messages on *another* task > > > runner which must complete before the navigation is resumed. So our code > > > basically does: > > > > > > Start(); > > > blocking_task_runner_->RunUntilIdle(); > > > WaitForThrottleChecksComplete(); > > > > > > And would hang if we omit the RunUntilIdle call. > > > > I found it practical to look at this from the following angle: what would be > the > > best interface for the NavigationSimulator so that it can be nicely used to > test > > two throttles at the same time (with both throttles doing async work in the > same > > stage)? > > > > While the current approach indeed allows verifying that things take place on > the > > correct threads, it is a bit awkward: > > > > > Start(); > > > blocking_task_runner1_->RunUntilIdle(); > > > base::RunLoop().RunUntilIdle(); // Invoke the next throttle. > > > blocking_task_runner2_->RunUntilIdle(); > > > WaitForThrottleChecksComplete(); > > > > It looks like that for a NavigationThrottles wishing to do async processing > > usually do it on: > > 1.) a TaskRunner specified at construction time (which can be set up in unit > > tests so that they are aliases of a single MessageLoop), > > 2.) Named content::BrowserThreads, which can be easily made real threads if > > there is no other solution, > > 3.) Blocking pools (not sure how these work with TestBrowserThreadBundle, but > > in the worst case, we could expose a method like [1] and let tests subclass > > NavigationSimulator, although I am not sure that's too nice either). > > > > The threading constraints could be checked at a different layer (or some other > > way, or not at all?), and then we would not need two flavors of these methods > > either. > > > > [1]: > > > https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?r... > > I see what you are saying. I am in favor of this approach (1) in this CL. If > consumers need (2) or (3) and don't like the proposed solutions we can think of > making NavigationSimulator more complicated. Yep, you understood correctly, but I should have phrased this better. To clarify, I did not mean to suggest using approaches (2) or (3) in this CL, I merely wanted to point out that in general, clients wishing to use NavigationSimulator will probably have some async behavior that falls into one of these categories (i.e. 1, 2, 3), so they will likely still be able to make good use of the simpler NavigationSimulator interface. My other pet peeve with the old solution was that a construct like this: Start(); blocking_task_runner_->RunUntilIdle(); WaitForThrottleChecksComplete(); does not actually verify that the async tasks are posted to |blocking_task_runner_| and not to the main MessageLoop. Furthermore, I think that because of crbug.com/631186, even if the classes being exercised (e.g. VerifiedRuleset) use a ThreadChecker inside, not even that will catch a threading problem in unittests. https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:456: // careful about relying on it as it may be going away. On 2017/03/07 16:42:43, Charlie Harrison wrote: > On 2017/03/07 15:24:56, engedy (slow) wrote: > > nit: I am not sure I understand this comment. > > This was indicated by clamy in the subframe navigation filtering throttle CL: > "I really don't want to be modifying the semantic of the callback right now. We > will do it in the future though, where either NavigationHandle will be merged in > the NavigationRequest or NavigationRequest methods will be called directly by > NavigationHandle. This will result in the callback disappearing." > > This is why we are adding a new for_testing_ callback instead of reusing this > one. Got it. Let's try to clarify in the comment that "going away" means that the member will be removed as part of a refactoring, not that the value of it might become NULL eventually during runtime. https://codereview.chromium.org/2698393002/diff/200001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc (right): https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:17: #include "components/subresource_filter/core/common/proto/rules.pb.h" nit: Do we use this and test_ruleset_utils.h? https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:55: if (async_filter_) { nit: s/async_filter_/{main_frame|parent}_{filter|dsf}/g, so that the name is more informative. https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:91: void CreateTestSubframeNavigation(const GURL& first_url, nit: CreatTestSubframeAndInitNavigation or something along these lines https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:93: navigating_subframe_ = nit: Member variable only used within this method.
Thanks for sticking through the review! I like the end result much better. LGTM % comments. https://codereview.chromium.org/2698393002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2698393002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:459: // This callback will be run when all throttle checks have been performed. Also mention that this is only used in unit tests, and add a TODO(clamy): Revisit the unit test architecture when PlzNavigate ships. https://codereview.chromium.org/2698393002/diff/200001/content/public/test/na... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2698393002/diff/200001/content/public/test/na... content/public/test/navigation_simulator.cc:129: // The request failed synchronously. nit: blank line above. https://codereview.chromium.org/2698393002/diff/200001/content/public/test/na... content/public/test/navigation_simulator.cc:155: state_ = FAILED; Could you add a TODO(clamy): Add error handling code based on the NavigationThrottleCheckResult.
The CQ bit was checked by csharrison@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 ========== Allow asynchronous deferral in NavigationSimulator BUG=TODO CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allow asynchronous deferral in NavigationSimulator This patch adds functionality to the testing class NavigationSimulator, to allow for NavigationThrottles to defer and cancel the navigation. The new functionality is tested by new NavigationSimulator unit tests, as well as new subresource filter unit tests exercising the untested SubframeNavigationFilteringThrottle. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Thanks everyone :) https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2698393002/diff/160001/content/public/test/na... content/public/test/navigation_simulator.h:74: // if (simulator->Start() == NavigationThrottle::DEFER) On 2017/03/09 11:41:34, engedy (slow) wrote: > On 2017/03/07 16:42:43, Charlie Harrison wrote: > > On 2017/03/07 15:24:56, engedy (slow) wrote: > > > > As discussed offline, this isn't possible with how we've designed > > subresource > > > > filter unit tests. These tests post a series of messages on *another* task > > > > runner which must complete before the navigation is resumed. So our code > > > > basically does: > > > > > > > > Start(); > > > > blocking_task_runner_->RunUntilIdle(); > > > > WaitForThrottleChecksComplete(); > > > > > > > > And would hang if we omit the RunUntilIdle call. > > > > > > I found it practical to look at this from the following angle: what would be > > the > > > best interface for the NavigationSimulator so that it can be nicely used to > > test > > > two throttles at the same time (with both throttles doing async work in the > > same > > > stage)? > > > > > > While the current approach indeed allows verifying that things take place on > > the > > > correct threads, it is a bit awkward: > > > > > > > Start(); > > > > blocking_task_runner1_->RunUntilIdle(); > > > > base::RunLoop().RunUntilIdle(); // Invoke the next throttle. > > > > blocking_task_runner2_->RunUntilIdle(); > > > > WaitForThrottleChecksComplete(); > > > > > > It looks like that for a NavigationThrottles wishing to do async processing > > > usually do it on: > > > 1.) a TaskRunner specified at construction time (which can be set up in > unit > > > tests so that they are aliases of a single MessageLoop), > > > 2.) Named content::BrowserThreads, which can be easily made real threads if > > > there is no other solution, > > > 3.) Blocking pools (not sure how these work with TestBrowserThreadBundle, > but > > > in the worst case, we could expose a method like [1] and let tests subclass > > > NavigationSimulator, although I am not sure that's too nice either). > > > > > > The threading constraints could be checked at a different layer (or some > other > > > way, or not at all?), and then we would not need two flavors of these > methods > > > either. > > > > > > [1]: > > > > > > https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?r... > > > > I see what you are saying. I am in favor of this approach (1) in this CL. If > > consumers need (2) or (3) and don't like the proposed solutions we can think > of > > making NavigationSimulator more complicated. > > Yep, you understood correctly, but I should have phrased this better. > > To clarify, I did not mean to suggest using approaches (2) or (3) in this CL, I > merely wanted to point out that in general, clients wishing to use > NavigationSimulator will probably have some async behavior that falls into one > of these categories (i.e. 1, 2, 3), so they will likely still be able to make > good use of the simpler NavigationSimulator interface. > > My other pet peeve with the old solution was that a construct like this: > > Start(); > blocking_task_runner_->RunUntilIdle(); > WaitForThrottleChecksComplete(); > > does not actually verify that the async tasks are posted to > |blocking_task_runner_| and not to the main MessageLoop. Furthermore, I think > that because of crbug.com/631186, even if the classes being exercised (e.g. > VerifiedRuleset) use a ThreadChecker inside, not even that will catch a > threading problem in unittests. Thanks for the clarification. https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2698393002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:456: // careful about relying on it as it may be going away. On 2017/03/09 11:41:34, engedy (slow) wrote: > On 2017/03/07 16:42:43, Charlie Harrison wrote: > > On 2017/03/07 15:24:56, engedy (slow) wrote: > > > nit: I am not sure I understand this comment. > > > > This was indicated by clamy in the subframe navigation filtering throttle CL: > > "I really don't want to be modifying the semantic of the callback right now. > We > > will do it in the future though, where either NavigationHandle will be merged > in > > the NavigationRequest or NavigationRequest methods will be called directly by > > NavigationHandle. This will result in the callback disappearing." > > > > This is why we are adding a new for_testing_ callback instead of reusing this > > one. > > Got it. Let's try to clarify in the comment that "going away" means that the > member will be removed as part of a refactoring, not that the value of it might > become NULL eventually during runtime. Done. https://codereview.chromium.org/2698393002/diff/200001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc (right): https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:17: #include "components/subresource_filter/core/common/proto/rules.pb.h" On 2017/03/09 11:41:34, engedy (slow) wrote: > nit: Do we use this and test_ruleset_utils.h? Nope, removed. https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:55: if (async_filter_) { On 2017/03/09 11:41:34, engedy (slow) wrote: > nit: s/async_filter_/{main_frame|parent}_{filter|dsf}/g, so that the name is > more informative. Done. https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:91: void CreateTestSubframeNavigation(const GURL& first_url, On 2017/03/09 11:41:34, engedy (slow) wrote: > nit: CreatTestSubframeAndInitNavigation or something along these lines Done. https://codereview.chromium.org/2698393002/diff/200001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle_unittest.cc:93: navigating_subframe_ = On 2017/03/09 11:41:34, engedy (slow) wrote: > nit: Member variable only used within this method. Done, good catch. https://codereview.chromium.org/2698393002/diff/200001/content/public/test/na... File content/public/test/navigation_simulator.cc (right): https://codereview.chromium.org/2698393002/diff/200001/content/public/test/na... content/public/test/navigation_simulator.cc:129: // The request failed synchronously. On 2017/03/09 13:18:13, clamy wrote: > nit: blank line above. Done. https://codereview.chromium.org/2698393002/diff/200001/content/public/test/na... content/public/test/navigation_simulator.cc:155: state_ = FAILED; On 2017/03/09 13:18:13, clamy wrote: > Could you add a TODO(clamy): Add error handling code based on the > NavigationThrottleCheckResult. Done.
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2698393002/#ps220001 (title: "engedy/clamy review")
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": 220001, "attempt_start_ts": 1489070174214840,
"parent_rev": "8163f69afafa00800c28c58ae942255339164ed4", "commit_rev":
"5da0c292e434236e9344a532471123912cdb1fe7"}
Message was sent while issue was closed.
Description was changed from ========== Allow asynchronous deferral in NavigationSimulator This patch adds functionality to the testing class NavigationSimulator, to allow for NavigationThrottles to defer and cancel the navigation. The new functionality is tested by new NavigationSimulator unit tests, as well as new subresource filter unit tests exercising the untested SubframeNavigationFilteringThrottle. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allow asynchronous deferral in NavigationSimulator This patch adds functionality to the testing class NavigationSimulator, to allow for NavigationThrottles to defer and cancel the navigation. The new functionality is tested by new NavigationSimulator unit tests, as well as new subresource filter unit tests exercising the untested SubframeNavigationFilteringThrottle. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2698393002 Cr-Commit-Position: refs/heads/master@{#455752} Committed: https://chromium.googlesource.com/chromium/src/+/5da0c292e434236e9344a5324711... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/5da0c292e434236e9344a5324711...
Message was sent while issue was closed.
jam@chromium.org changed reviewers: + jam@chromium.org
Message was sent while issue was closed.
CancelMethod/NavigationSimulatorTest.Cancel/4 is failing on tsan/asan with PlzNavigate enabled: https://codereview.chromium.org/2385413004/#ps1110001 https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Can you disable the test in the meantime? |
