Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691423006:
Introduce the ThrottleManager (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org, Bryan McQuade Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce the ThrottleManager
This patch adds a new class responsible for orchestrating the various
navigation throttles associated with the subresource_filter. So far
it is only exercised in unit tests.
TBR=shivanisha@chromium.org
BUG=637415
Review-Url: https://codereview.chromium.org/2691423006
Cr-Commit-Position: refs/heads/master@{#458188}
Committed: https://chromium.googlesource.com/chromium/src/+/47c0e8d177ffdb35e1e58650993200a1832e46f1
Patch Set 1 #
Total comments: 10
Patch Set 2 : simplify Delegate two only two methods, VetoActivation and OnFirstSubresourceDisallowed #Patch Set 3 : engedy initial comments #
Total comments: 18
Patch Set 4 : respond to more comments #
Total comments: 34
Patch Set 5 : address feedback #Patch Set 6 : unit test #Patch Set 7 : rebase + add same-site test #Patch Set 8 : Make sure tests work in --site-per-process #Patch Set 9 : rebase #Patch Set 10 : rebase #
Total comments: 48
Patch Set 11 : engedy review (decent refactor) #Patch Set 12 : rebase #Patch Set 13 : rebase #Patch Set 14 : fix activation state unit tests #
Total comments: 2
Patch Set 15 : update NavigationSimulator description #
Total comments: 12
Patch Set 16 : engedy review #Messages
Total messages: 67 (45 generated)
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...
csharrison@chromium.org changed reviewers: + engedy@chromium.org
Hey Balazs, This is a rough draft CL (please ignore the unit tests, that work is in flux right now with all the dependent CLs), but I was interested in your take on the high level design. Namely, I feel like the logic in the "throttle manager" (name tbd) does constrain to a small-ish API. I would like to keep it that way by delegating higher level decisions like whitelists to the parent class. Two changes I'm planning on making (in this CL): 1. Remove Delegate::GetRulesetDealerHandle and inject it in the constructor. 2. Remove OnActivationStateComputedForCommittingLoad and just send the IPC in this class. LMK what you think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Agreed with both proposed changes, flushing pending comments, and I will continue looking at the new patch set. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:35: ContentSubresourceFilterThrottleManager:: Should we make these methods private, and expose one additional method that the ChromeContentBrowserDelegate will call for each navigation? (That is, make it the responsibility of this class to figure out what throttles are needed.) https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:38: using ActivatedFrameHostMap = nit: We should at least move this down to the `private:` section inside the class. We might potentially get rid of the type aliases altogether, I think the underlying type names are more meaningful. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:55: // first load is disallowed. nit: ... for a given page load. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:74: // The subframe navigation filtering throttle must be inserted before the On second thought, does the order really matter here? There aren't any navigation stages where both of two throttles are activated. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:108: // Map of all ongoing activation computing throttles. This is needed to Phrasing suggestion: For each ongoing navigation that requires activation state computation, keeps track of the throttle that is carrying out that computation, so that the result can be retrieved when the navigation is ready to commit.
Addressed initial comments, thanks! https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:35: ContentSubresourceFilterThrottleManager:: On 2017/02/17 15:34:27, engedy wrote: > Should we make these methods private, and expose one additional method that the > ChromeContentBrowserDelegate will call for each navigation? (That is, make it > the responsibility of this class to figure out what throttles are needed.) Yeah I thought of this before and was nervous to e.g. take a vector* as input. I think the pros outweigh the cons here though, so done. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:38: using ActivatedFrameHostMap = On 2017/02/17 15:34:27, engedy wrote: > nit: We should at least move this down to the `private:` section inside the > class. We might potentially get rid of the type aliases altogether, I think the > underlying type names are more meaningful. Got rid of them. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:55: // first load is disallowed. On 2017/02/17 15:34:27, engedy wrote: > nit: ... for a given page load. Done. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:74: // The subframe navigation filtering throttle must be inserted before the On 2017/02/17 15:34:27, engedy wrote: > On second thought, does the order really matter here? There aren't any > navigation stages where both of two throttles are activated. You're right, I've removed the comment. https://codereview.chromium.org/2691423006/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:108: // Map of all ongoing activation computing throttles. This is needed to On 2017/02/17 15:34:27, engedy wrote: > Phrasing suggestion: > > For each ongoing navigation that requires activation state computation, keeps > track of the throttle that is carrying out that computation, so that the result > can be retrieved when the navigation is ready to commit. Done.
A few comments, I will take another pass on Monday. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:38: void ContentSubresourceFilterThrottleManager::OnPageStateActivationComputed( nit: For consistency, what do you think about calling this NotifyPageLevelActivationComputed (or NotifyPageActivationComputed)? https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:40: const ActivationState& state) { nit: %s/ state/ activation_state/g https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:48: OnFirstSubresourceLoadDisallowed, Note that, later, we will need to clean up this signal so that the Delegate gets at most one callback per main frame navigation, even if there are resources disallowed in multiple frames. Also, we need to integrate the signals coming from the SubresourceFilterAgents on the renderer side the same way. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:70: activated_frame_hosts_.erase(frame_host); nit: It's a bit hard to argue about |ruleset_handle_| lifetime here. In the admittedly unlikely case where there are no subframes, suppose the current main frame has activation, and we navigation away to a same-origin page that does not have activation. Do we ever free up the |ruleset_handle| in this case? https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:85: activated_frame_hosts_[navigation_handle->GetRenderFrameHost()] = nit: activated_frame_hosts_[frame_host] https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:95: void ContentSubresourceFilterThrottleManager::MaybeInsertNavigationThrottles( nit: MaybeAppendNavigationThrottles https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:112: if (navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage()) Could you please verify what happens here for navigations synchronously committed on the renderer side (like about:blank) and for error page loads? Throttles are never invoked for these, it seems wasteful to create them in the first place (and muddies up metrics).
Many thanks, hopefully I'll be able to straighten out the unit test situation soon, and get those up. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:38: void ContentSubresourceFilterThrottleManager::OnPageStateActivationComputed( On 2017/02/17 20:44:44, engedy wrote: > nit: For consistency, what do you think about calling this > NotifyPageLevelActivationComputed (or NotifyPageActivationComputed)? Sure, renamed to NotifyPageActivationComputed. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:40: const ActivationState& state) { On 2017/02/17 20:44:44, engedy wrote: > nit: %s/ state/ activation_state/g Done, here and throughout the file. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:48: OnFirstSubresourceLoadDisallowed, On 2017/02/17 20:44:44, engedy wrote: > Note that, later, we will need to clean up this signal so that the Delegate gets > at most one callback per main frame navigation, even if there are resources > disallowed in multiple frames. Also, we need to integrate the signals coming > from the SubresourceFilterAgents on the renderer side the same way. I think the coalescing should be done in this class, I can try to tackle it in this CL. I was imagining the renderer IPCing this information to this class, and I will add a TODO to that effect. This shouldn't be too hard, because there is at most one committed main frame navigation per WebContents at a time. I will encode it as a bool. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:70: activated_frame_hosts_.erase(frame_host); On 2017/02/17 20:44:44, engedy wrote: > nit: It's a bit hard to argue about |ruleset_handle_| lifetime here. > > In the admittedly unlikely case where there are no subframes, suppose the > current main frame has activation, and we navigation away to a same-origin page > that does not have activation. Do we ever free up the |ruleset_handle| in this > case? You're right, this is hard to reason about. Essentially the handle is ref-bumped per every activated RenderFrameHost, and derefed when that RenderFrameHost is no longer active. The deref is "lazy" in the sense that we only check this when a RenderFrame is deleted, which does not happen for same-site navigations. Probably we can improve things here. The "cleanest" solution would be to get rid of the lazy wart, and make sure the handle is destroyed whenever we have no activated frames. I've done that here, let me know what you think. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:85: activated_frame_hosts_[navigation_handle->GetRenderFrameHost()] = On 2017/02/17 20:44:44, engedy wrote: > nit: activated_frame_hosts_[frame_host] Done. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:95: void ContentSubresourceFilterThrottleManager::MaybeInsertNavigationThrottles( On 2017/02/17 20:44:44, engedy wrote: > nit: MaybeAppendNavigationThrottles Done. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:112: if (navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage()) On 2017/02/17 20:44:44, engedy wrote: > Could you please verify what happens here for navigations synchronously > committed on the renderer side (like about:blank) and for error page loads? > Throttles are never invoked for these, it seems wasteful to create them in the > first place (and muddies up metrics). Yes I think same-page navigations and all synchronous navigations (about:blank, etc) do not invoke throttles, I just tested this with PlzNavigate turned on and off. Turned these checks into DCHECKs.
Looks good % comments below. Looking forward to unit tests. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:48: OnFirstSubresourceLoadDisallowed, On 2017/02/17 22:12:31, Charlie Harrison wrote: > On 2017/02/17 20:44:44, engedy wrote: > > Note that, later, we will need to clean up this signal so that the Delegate > gets > > at most one callback per main frame navigation, even if there are resources > > disallowed in multiple frames. Also, we need to integrate the signals coming > > from the SubresourceFilterAgents on the renderer side the same way. > > I think the coalescing should be done in this class, I can try to tackle it in > this CL. I was imagining the renderer IPCing this information to this class, and > I will add a TODO to that effect. > > This shouldn't be too hard, because there is at most one committed main frame > navigation per WebContents at a time. I will encode it as a bool. Sounds good, let's do that in a follow-up CL. ChromeSubresourceFilterClient::ToggleNotificationVisibility already does some deduplication, it would be nice to clean that up a bit too. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:112: if (navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage()) On 2017/02/17 22:12:31, Charlie Harrison wrote: > On 2017/02/17 20:44:44, engedy wrote: > > Could you please verify what happens here for navigations synchronously > > committed on the renderer side (like about:blank) and for error page loads? > > Throttles are never invoked for these, it seems wasteful to create them in the > > first place (and muddies up metrics). > > Yes I think same-page navigations and all synchronous navigations (about:blank, > etc) do not invoke throttles, I just tested this with PlzNavigate turned on and > off. Turned these checks into DCHECKs. Awesome, thanks for checking! https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:42: DCHECK(activated_navigation->IsInMainFrame()); For DCHECK: #include "base/logging.h" https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:97: if (navigation_handle->IsInMainFrame() && navigation_handle->HasCommitted()) Do we need to check non-same-page here? https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:122: GetFrameActivationInfoForChildNavigation(navigation_handle); nit: What do you think about: FrameActivationInfo* parent_info = GetParentFrameActivationInfo(navigation_handle); https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:133: if (navigation_handle->IsInMainFrame()) style nit: {} https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:138: FrameActivationInfo* info = nit: s/info/parent_info/ https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:31: struct FrameActivationInfo { nit: Can the definition of this struct be moved to the .cc file? Looks like a forward declaration might be enough here. Would also be nice to make this nested inside the manager so that we do not pollute the namespace. nit: Could you please add a TODO to consider storing the ActivationState on the ADSF instead, and removing this struct? https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:38: class ContentSubresourceFilterThrottleManager nit: Let's describe what this is in a comment! https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:51: virtual bool ShouldVetoActivation( nit: s/Veto/Suppress/, my understanding is that a method name `ShouldX` is interpreted as "Does the embedder want the content/ layer to do X", so the subject is the content/ layer, not the embedder. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:61: // Must be called during a navigation, before the corresponding throttle in Let's make this interface less implementation-oriented and more embedder-friendly: // Sets the desired page-level |activation_state| for the currently ongoing page load, identified by its main-frame |navigation_handle|. To be called by the embedder at the latest in the WillProcessResponse stage, at latest from a NavigationThrottle that was registered before the throttles created by this manager in MaybeAppendNavigationThrottles(). If this method is not called for a main-frame navigation, the default behavior is no activation for that page load. virtual void ActivateForPageLoad( NavigationHandle* navigation_handle, ...); https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:64: content::NavigationHandle* activated_navigation, nit: s/activated_navigation/navigation/, it's not necessarily going to be activated. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:67: // content::WebContentsObserver: Could we make these protected? https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:74: void MaybeAppendNavigationThrottles( nit: Given this is the public API of this class, and of more or less the entire component, let's add a comment to describe this (and move it above the observer methods). Also mention the NavigationThrottle ordering constraint in the comment. nit: Do you think returning a vector (to be appended by the caller) would be frowned upon? It's only one more allocation per navigation, which should also be relatively fast thanks to tc_malloc? https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:90: // committed load. nit: once per committed, non-same-page navigation in the main frame. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:96: void DestroyRulesetHandleIfNecessary(); nit: s/Necessary/NoLongerUsed/ https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:98: // Map of all RenderFrameHosts that are currently activated, and their Phrasing nit: For each RenderFrameHost where the last committed load has subresource filtering activated, stores the ActivationState and owns the corresponding AsyncDocumentSubresourceFilter. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:56: : public content::RenderViewHostTestHarness, nit: Could you please check if there are any precedents for using the TestHarness also as a WebContentsObserver? I don't have a logical argument against it, but seems unusual to me.
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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hey engedy: Here is the test suite for the throttle manager, plus responding to your last comments. Note: clamy's patch no longer applies cleanly so trybots will be red. All these tests succeed locally (linux) with and without PlzNav, and with and without --site-per-process. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:42: DCHECK(activated_navigation->IsInMainFrame()); On 2017/02/20 15:58:13, engedy (slow) wrote: > For DCHECK: #include "base/logging.h" Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:97: if (navigation_handle->IsInMainFrame() && navigation_handle->HasCommitted()) On 2017/02/20 15:58:13, engedy (slow) wrote: > Do we need to check non-same-page here? Yep, great catch. Adding this to the testing TODO. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:122: GetFrameActivationInfoForChildNavigation(navigation_handle); On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: What do you think about: > > FrameActivationInfo* parent_info = > GetParentFrameActivationInfo(navigation_handle); I like it (modified slightly for the removal of FrameActivationInfo). https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:133: if (navigation_handle->IsInMainFrame()) On 2017/02/20 15:58:13, engedy (slow) wrote: > style nit: {} Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:138: FrameActivationInfo* info = On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: s/info/parent_info/ Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:31: struct FrameActivationInfo { On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: Can the definition of this struct be moved to the .cc file? Looks like a > forward declaration might be enough here. Would also be nice to make this nested > inside the manager so that we do not pollute the namespace. > > nit: Could you please add a TODO to consider storing the ActivationState on the > ADSF instead, and removing this struct? Let me just move the ActivationState into ADSF in this patch, it simplifies things. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:38: class ContentSubresourceFilterThrottleManager On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: Let's describe what this is in a comment! Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:51: virtual bool ShouldVetoActivation( On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: s/Veto/Suppress/, my understanding is that a method name `ShouldX` is > interpreted as "Does the embedder want the content/ layer to do X", so the > subject is the content/ layer, not the embedder. Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:61: // Must be called during a navigation, before the corresponding throttle in On 2017/02/20 15:58:13, engedy (slow) wrote: > Let's make this interface less implementation-oriented and more > embedder-friendly: > > // Sets the desired page-level |activation_state| for the currently ongoing page > load, identified by its main-frame |navigation_handle|. To be called by the > embedder at the latest in the WillProcessResponse stage, at latest from a > NavigationThrottle that was registered before the throttles created by this > manager in MaybeAppendNavigationThrottles(). If this method is not called for a > main-frame navigation, the default behavior is no activation for that page load. > virtual void ActivateForPageLoad( > NavigationHandle* navigation_handle, ...); Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:64: content::NavigationHandle* activated_navigation, On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: s/activated_navigation/navigation/, it's not necessarily going to be > activated. I s/activated_navigation/navigation_handle/ https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:67: // content::WebContentsObserver: On 2017/02/20 15:58:13, engedy (slow) wrote: > Could we make these protected? Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:74: void MaybeAppendNavigationThrottles( On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: Given this is the public API of this class, and of more or less the entire > component, let's add a comment to describe this (and move it above the observer > methods). Also mention the NavigationThrottle ordering constraint in the > comment. Done, I mentioned that in fact there are no ordering constraints. > > nit: Do you think returning a vector (to be appended by the caller) would be > frowned upon? It's only one more allocation per navigation, which should also be > relatively fast thanks to tc_malloc? I'm not sure, let's punt this for when we actually use the method in a follow up and see what people think. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:90: // committed load. On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: once per committed, non-same-page navigation in the main frame. Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:96: void DestroyRulesetHandleIfNecessary(); On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: s/Necessary/NoLongerUsed/ Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:98: // Map of all RenderFrameHosts that are currently activated, and their On 2017/02/20 15:58:13, engedy (slow) wrote: > Phrasing nit: For each RenderFrameHost where the last committed load has > subresource filtering activated, stores the ActivationState and owns the > corresponding AsyncDocumentSubresourceFilter. Done. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:56: : public content::RenderViewHostTestHarness, On 2017/02/20 15:58:13, engedy (slow) wrote: > nit: Could you please check if there are any precedents for using the > TestHarness also as a WebContentsObserver? I don't have a logical argument > against it, but seems unusual to me. I couldn't find any. Happy to refactor if you want. The RenderViewHostTestHarness will only change out the WebContents with SetContents(), so as long as we don't call that we shouldn't run into any lifetime concerns. I figured this was an easy way for the test harness to introspect itself :)
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.
csharrison@chromium.org changed reviewers: + shivanisha@chromium.org
+shivanisha, this is a part of the major refactor of the filtering logic to work with PlzNavigate/OOPIF.
Description was changed from ========== Introduce the ThrottleManager BUG= ========== to ========== Introduce the ThrottleManager BUG=637415 ==========
cc bmcquade
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.
https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:112: if (navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage()) To quote a popular meme: -- Tell me you turned these conditionals into DCHECKs. -- Okay. I haven't, though. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:74: void MaybeAppendNavigationThrottles( On 2017/03/01 00:02:58, Charlie Harrison wrote: > On 2017/02/20 15:58:13, engedy (slow) wrote: > > nit: Given this is the public API of this class, and of more or less the > entire > > component, let's add a comment to describe this (and move it above the > observer > > methods). Also mention the NavigationThrottle ordering constraint in the > > comment. > Done, I mentioned that in fact there are no ordering constraints. > > > > nit: Do you think returning a vector (to be appended by the caller) would be > > frowned upon? It's only one more allocation per navigation, which should also > be > > relatively fast thanks to tc_malloc? > I'm not sure, let's punt this for when we actually use the method in a follow up > and see what people think. Sounds good. https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:56: : public content::RenderViewHostTestHarness, On 2017/03/01 00:02:59, Charlie Harrison wrote: > On 2017/02/20 15:58:13, engedy (slow) wrote: > > nit: Could you please check if there are any precedents for using the > > TestHarness also as a WebContentsObserver? I don't have a logical argument > > against it, but seems unusual to me. > > I couldn't find any. Happy to refactor if you want. > > The RenderViewHostTestHarness will only change out the WebContents with > SetContents(), so as long as we don't call that we shouldn't run into any > lifetime concerns. I figured this was an easy way for the test harness to > introspect itself :) Fine by me. Both the WC and WCO seems pretty robust in terms of destruction order. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:44: base::OnceClosure first_disallowed_load_callback); optional nit: On second read, it seems a bit weird to plumb this through here, considering that this throttle is not doing any filtering. Especially given the two different code paths for main/subframes here and in the caller. Maybe we could let the ThrottleManager set it on the ADSF post-construction? https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:71: const ActivationState& GetActivationState() const; Now that the ADSF has the state, do we need this and the corresponding member variable anymore? https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:86: activation_state_.emplace(activation_state); nit: Looks like this is equivalent to: activation_state_ = activation_state; WDYT about adopting this convention? https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:65: // Make sure that if the committed navigation is in a RenderFrameHost that is Note that ReadyToCommitNavigation is not invoked for failed navigations pre-PlzNavigate, and also not invoked for synchronous loads (e.g. about:blank) regardless of PlzNavigate, so some of this clean-up will need to go into DidFinishNavigation. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:77: // and won't receive valid filters or activation states. I am not sure but I think that in all these cases ReadyToCommitNavigation might not be not called either. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:127: GetParentFilter(navigation_handle); nit: GetParentFrameFilter https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:80: // Note that there is currently no ordering constraints of the throttles. nit: no constraints on the ordering of throttles https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:83: std::vector<std::unique_ptr<content::NavigationThrottle>>* throttles); #include <vector> https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:106: // activated and has no subresource filter. nit: (and therefore has ... ).
https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:77: // and won't receive valid filters or activation states. On 2017/03/10 17:36:20, engedy (slow) wrote: > I am not sure but I think that in all these cases ReadyToCommitNavigation might > not be not called either. I checked and my previous statement is not correct. For *browser-initiated navigations* to URLs for which ShouldMakeNetworkRequestForURL() evaluates to false, there will be no throttles consulted, but RTCN is invoked.
Superb work, thanks! LGTM % outstanding comments. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:65: // Make sure that if the committed navigation is in a RenderFrameHost that is On 2017/03/10 17:36:20, engedy (slow) wrote: > Note that ReadyToCommitNavigation is not invoked for failed navigations > pre-PlzNavigate, and also not invoked for synchronous loads (e.g. about:blank) > regardless of PlzNavigate, so some of this clean-up will need to go into > DidFinishNavigation. Could you please add a unit test to ensure the ruleset is released in this case? https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:31: const char kActivatePageEnabled[] = "https://www.activate-page-enabled.com/"; nit: How about changing these from the imperative to the declarative: kTestURLWithActivation = page-with-activation.com, kTestURLWithDryRun = page-with-dryrun.com https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:36: // specific set of URLs. nit: hardcoded set of testing URLs Also, as discussed, let's allow the timing of the notification to be configured (on WillStartRequest, or on WillProcessResponse). https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:118: // content::WebContentsObserver nit: Once again, let's move these into a protected section. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:155: void CreateTestSubframeNavigation(const GURL& url, nit: CreateSubframe{And|With}TestNavigation https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:181: navigation_simulator_.reset(); Does this somehow call DidFinishNavigation? https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:205: // A main frame navigation should never have start deferred. nit: This comment is no longer meaningful. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:234: // Logged on every OnFirstSubresourceLoadDisallowed call. nit: s/Logged/Incremented/g https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:237: // Logged every time the manager asks the harness for an activation nit: s/asks an/queries/ https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:353: // Test that the disallow load notification will not repeat if a load is nit: ... will not be repeated for the first disallowed load that follows a ... https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:442: // Navigate a subframe that is not filtered, but should still activate. nit: To clarify the terminology around this unit test, let's phrase these commments like so: // Navigate a subframe to a URL that is not itself disallowed. Subresource filtering for this subframe document should still be activated. Feel free to improve the wording, the above is just a draft. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:450: CreateTestSubframeNavigation(GURL("https://www.example.com/allowed.html"), For good measure, let's make this cross-origin too (and let's make it more obviously cross-origin, like a.com and b.com).
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-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...) 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_...)
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 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...
Hey Balazs, The ReadyToCommit/DidFinishNavigation issue caused some issues. I have a solution which works pretty generally, but would appreciate another look. Also, navigations to about:blank were causing crashes in NavigationSimulator. I'll try to address that in a followup. https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:112: if (navigation_handle->IsInMainFrame() || navigation_handle->IsSamePage()) On 2017/03/10 17:36:20, engedy wrote: > To quote a popular meme: > > -- Tell me you turned these conditionals into DCHECKs. > -- Okay. I haven't, though. LOL. Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:44: base::OnceClosure first_disallowed_load_callback); On 2017/03/10 17:36:20, engedy (slow) wrote: > optional nit: On second read, it seems a bit weird to plumb this through here, > considering that this throttle is not doing any filtering. Especially given the > two different code paths for main/subframes here and in the caller. > > Maybe we could let the ThrottleManager set it on the ADSF post-construction? Sure. I added a setter on the ADSF. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:71: const ActivationState& GetActivationState() const; On 2017/03/10 17:36:20, engedy (slow) wrote: > Now that the ADSF has the state, do we need this and the corresponding member > variable anymore? Nope, removed. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/async_document_subresource_filter.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/async_document_subresource_filter.cc:86: activation_state_.emplace(activation_state); On 2017/03/10 17:36:20, engedy (slow) wrote: > nit: Looks like this is equivalent to: > > activation_state_ = activation_state; > > WDYT about adopting this convention? Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:65: // Make sure that if the committed navigation is in a RenderFrameHost that is On 2017/03/10 20:50:34, engedy wrote: > On 2017/03/10 17:36:20, engedy (slow) wrote: > > Note that ReadyToCommitNavigation is not invoked for failed navigations > > pre-PlzNavigate, and also not invoked for synchronous loads (e.g. about:blank) > > regardless of PlzNavigate, so some of this clean-up will need to go into > > DidFinishNavigation. > > Could you please add a unit test to ensure the ruleset is released in this case? Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:65: // Make sure that if the committed navigation is in a RenderFrameHost that is On 2017/03/10 20:50:34, engedy wrote: > On 2017/03/10 17:36:20, engedy (slow) wrote: > > Note that ReadyToCommitNavigation is not invoked for failed navigations > > pre-PlzNavigate, and also not invoked for synchronous loads (e.g. about:blank) > > regardless of PlzNavigate, so some of this clean-up will need to go into > > DidFinishNavigation. > > Could you please add a unit test to ensure the ruleset is released in this case? Added tests for: - Same site navigations to inactive pages -> should stop activation and lose the ruleset. - Same site navigations which *FAIL* to inactive pages -> shouldn't stop activation. I also added tests for ERR_ABORTED failure vs. committing an error page. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:65: // Make sure that if the committed navigation is in a RenderFrameHost that is On 2017/03/10 17:36:20, engedy (slow) wrote: > Note that ReadyToCommitNavigation is not invoked for failed navigations > pre-PlzNavigate, and also not invoked for synchronous loads (e.g. about:blank) > regardless of PlzNavigate, so some of this clean-up will need to go into > DidFinishNavigation. Ah that is very unfortunate. To get around that and some related issues I added the following code: 1. Add a ADSF* getter in the throttle, and use that in ReadyToCommitNavigation 2. Add a method WillSendActivation* to the throttle, notifying it that (essentially) ReadyToCommitNavigation happened, and the throttle is in an OK state (aka did not skip IPC). 3. Modified ReleaseFilter to only transfer ownership if IPC was sent. 4. Release ownership of the filter (and do necessary cleanup) in DidFinishNavigation. PTAL. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:77: // and won't receive valid filters or activation states. On 2017/03/10 17:55:55, engedy wrote: > On 2017/03/10 17:36:20, engedy (slow) wrote: > > I am not sure but I think that in all these cases ReadyToCommitNavigation > might > > not be not called either. > > I checked and my previous statement is not correct. For *browser-initiated > navigations* to URLs for which ShouldMakeNetworkRequestForURL() evaluates to > false, there will be no throttles consulted, but RTCN is invoked. Yeah I noticed this in tests. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:127: GetParentFilter(navigation_handle); On 2017/03/10 17:36:20, engedy wrote: > nit: GetParentFrameFilter Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:80: // Note that there is currently no ordering constraints of the throttles. On 2017/03/10 17:36:21, engedy (slow) wrote: > nit: no constraints on the ordering of throttles Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:83: std::vector<std::unique_ptr<content::NavigationThrottle>>* throttles); On 2017/03/10 17:36:21, engedy (slow) wrote: > #include <vector> Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h:106: // activated and has no subresource filter. On 2017/03/10 17:36:21, engedy (slow) wrote: > nit: (and therefore has ... ). Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:31: const char kActivatePageEnabled[] = "https://www.activate-page-enabled.com/"; On 2017/03/10 20:50:35, engedy wrote: > nit: How about changing these from the imperative to the declarative: > kTestURLWithActivation = http://page-with-activation.com, > kTestURLWithDryRun = http://page-with-dryrun.com Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:36: // specific set of URLs. On 2017/03/10 20:50:35, engedy wrote: > nit: hardcoded set of testing URLs > > Also, as discussed, let's allow the timing of the notification to be configured > (on WillStartRequest, or on WillProcessResponse). I couldn't think of a great criteria for which tests should be triggered on WillStartRequest, or WillProcessResponse. So, I parameterized all the tests by this bit. WDYT? Overkill? https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:118: // content::WebContentsObserver On 2017/03/10 20:50:34, engedy wrote: > nit: Once again, let's move these into a protected section. Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:155: void CreateTestSubframeNavigation(const GURL& url, On 2017/03/10 20:50:35, engedy wrote: > nit: CreateSubframe{And|With}TestNavigation Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:181: navigation_simulator_.reset(); On 2017/03/10 20:50:34, engedy wrote: > Does this somehow call DidFinishNavigation? No, it will be caused by the throttle cancelling it (during the call to Redirect()). This just ensures we don't have a vestigial simulator hanging around. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:205: // A main frame navigation should never have start deferred. On 2017/03/10 20:50:34, engedy wrote: > nit: This comment is no longer meaningful. Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:234: // Logged on every OnFirstSubresourceLoadDisallowed call. On 2017/03/10 20:50:34, engedy wrote: > nit: s/Logged/Incremented/g Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:237: // Logged every time the manager asks the harness for an activation On 2017/03/10 20:50:34, engedy wrote: > nit: s/asks an/queries/ Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:353: // Test that the disallow load notification will not repeat if a load is On 2017/03/10 20:50:35, engedy wrote: > nit: ... will not be repeated for the first disallowed load that follows a ... Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:442: // Navigate a subframe that is not filtered, but should still activate. On 2017/03/10 20:50:34, engedy wrote: > nit: To clarify the terminology around this unit test, let's phrase these > commments like so: > > // Navigate a subframe to a URL that is not itself disallowed. Subresource > filtering for this subframe document should still be activated. > > Feel free to improve the wording, the above is just a draft. Done. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:450: CreateTestSubframeNavigation(GURL("https://www.example.com/allowed.html"), On 2017/03/10 20:50:35, engedy wrote: > For good measure, let's make this cross-origin too (and let's make it more > obviously cross-origin, like a.com and b.com). Done.
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 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...
csharrison@chromium.org changed reviewers: + clamy@chromium.org
+clamy for NavigationSimulator changes. The convenience function to get the final RFH is not strictly necessary, but this has come up twice in subresource filter tests (once more in ActivationStateComputingNavthrottle tests), and it is much cleaner than observing DidFinishNavigation in the tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Lgtm % one comment. https://codereview.chromium.org/2691423006/diff/260001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2691423006/diff/260001/content/public/test/na... content/public/test/navigation_simulator.h:123: // Must be called after the simulated navigation commits. Returns the render I would rephrase that as "Must be called after the simulated navigation or an error page has committed. Returns the RenderFrameHost the navigation committed in."
Thanks! Balazs: Waiting for one more sign off from you (mostly for changes to the cleanup in the throttle manager itself). Shivani: PTAL, but just FYI that I'll probably land this if you don't get to it before Balazs does, because my progress is blocked on this, and another CL from melandory@ https://codereview.chromium.org/2691423006/diff/260001/content/public/test/na... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2691423006/diff/260001/content/public/test/na... content/public/test/navigation_simulator.h:123: // Must be called after the simulated navigation commits. Returns the render On 2017/03/20 15:50:10, clamy wrote: > I would rephrase that as "Must be called after the simulated navigation or an > error page has committed. Returns the RenderFrameHost the navigation committed > in." Done.
Shivani: forgot to say, in the case that I land this before your review, please still TBR :)
LGTM % comments, thanks a lot! https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:65: // Make sure that if the committed navigation is in a RenderFrameHost that is On 2017/03/14 23:18:31, Charlie Harrison-slow til 3-20 wrote: > On 2017/03/10 20:50:34, engedy wrote: > > On 2017/03/10 17:36:20, engedy (slow) wrote: > > > Note that ReadyToCommitNavigation is not invoked for failed navigations > > > pre-PlzNavigate, and also not invoked for synchronous loads (e.g. > about:blank) > > > regardless of PlzNavigate, so some of this clean-up will need to go into > > > DidFinishNavigation. > > > > Could you please add a unit test to ensure the ruleset is released in this > case? > > Added tests for: > - Same site navigations to inactive pages -> should stop activation and lose the > ruleset. > - Same site navigations which *FAIL* to inactive pages -> shouldn't stop > activation. > I also added tests for ERR_ABORTED failure vs. committing an error page. Acknowledged, thanks! https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:36: // specific set of URLs. On 2017/03/14 23:18:32, Charlie Harrison-slow til 3-20 wrote: > On 2017/03/10 20:50:35, engedy wrote: > > nit: hardcoded set of testing URLs > > > > Also, as discussed, let's allow the timing of the notification to be > configured > > (on WillStartRequest, or on WillProcessResponse). > > I couldn't think of a great criteria for which tests should be triggered on > WillStartRequest, or WillProcessResponse. So, I parameterized all the tests by > this bit. WDYT? Overkill? Hmm, I'm not against it as long as these parameterized tests can be found on the flakiness dashboard, and other infrastrucutre. I always had trouble with them. https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:181: navigation_simulator_.reset(); On 2017/03/14 23:18:32, Charlie Harrison-slow til 3-20 wrote: > On 2017/03/10 20:50:34, engedy wrote: > > Does this somehow call DidFinishNavigation? > > No, it will be caused by the throttle cancelling it (during the call to > Redirect()). This just ensures we don't have a vestigial simulator hanging > around. Ah, got it, thanks. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:72: void set_async_filter( nit: set_filter for consistency with methods above https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:91: // Becomes set to true when the throttle manager reaches nit: s/set to// https://codereview.chromium.org/2691423006/diff/280001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:68: if (!filter || navigation_handle->GetNetErrorCode() != net::OK || nit: Can |filter| really be false for the reason above? I would have imagined that no throttles are created for these special navigations when no throttles are consulted. So the only reason for a non-existent filter would be no page-level activation or failed/cancelled navigation. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:76: filter->set_first_disallowed_load_callback(base::Bind( nit: Could we set this later once we have taken ownership of the filter? https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:192: if (activated_frame_hosts_.size() == 0) Can it happen that |activated_frame_hosts| is empty, yet there are |ongoing_activation_throttles_| using the ruleset? https://codereview.chromium.org/2691423006/diff/280001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:40: enum PageActivationThrottleState { nit: PageActivationNotificationTiming?
Thanks Balazs. Sending to CQ now, I'll try to wire up with the safebrowsing client work. Shivani: TBR https://codereview.chromium.org/2691423006/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:36: // specific set of URLs. On 2017/03/20 18:58:14, engedy wrote: > On 2017/03/14 23:18:32, Charlie Harrison-slow til 3-20 wrote: > > On 2017/03/10 20:50:35, engedy wrote: > > > nit: hardcoded set of testing URLs > > > > > > Also, as discussed, let's allow the timing of the notification to be > > configured > > > (on WillStartRequest, or on WillProcessResponse). > > > > I couldn't think of a great criteria for which tests should be triggered on > > WillStartRequest, or WillProcessResponse. So, I parameterized all the tests by > > this bit. WDYT? Overkill? > > Hmm, I'm not against it as long as these parameterized tests can be found on the > flakiness dashboard, and other infrastrucutre. I always had trouble with them. Just did a gut check on the flakiness dashboard and it looks fine. Just have to know that they will have /0 or /1 suffixed depending on the type. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h (right): https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:72: void set_async_filter( On 2017/03/20 18:58:14, engedy wrote: > nit: set_filter for consistency with methods above Done. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h:91: // Becomes set to true when the throttle manager reaches On 2017/03/20 18:58:14, engedy wrote: > nit: s/set to// Done. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc (right): https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:68: if (!filter || navigation_handle->GetNetErrorCode() != net::OK || On 2017/03/20 18:58:14, engedy wrote: > nit: Can |filter| really be false for the reason above? I would have imagined > that no throttles are created for these special navigations when no throttles > are consulted. So the only reason for a non-existent filter would be no > page-level activation or failed/cancelled navigation. I can't reproduce this now, as NavigationSimulator is not robust to synchronous navigations. I am pretty sure I needed this when I was using the test navigation handle methods. It is possible that this was not accurate of real world code paths. However, since we now use filter existence as a proxy for some flavor of enabled ActivationState, we don't need the comment anymore. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:76: filter->set_first_disallowed_load_callback(base::Bind( On 2017/03/20 18:58:14, engedy wrote: > nit: Could we set this later once we have taken ownership of the filter? Done. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc:192: if (activated_frame_hosts_.size() == 0) On 2017/03/20 18:58:14, engedy wrote: > Can it happen that |activated_frame_hosts| is empty, yet there are > |ongoing_activation_throttles_| using the ruleset? This is pretty tricky. I think we're ok here but I'd prefer to code defensively, so I am modifying this to not release the handle if there are activation throttles. My reasoning: The manager is per web-contents. For us to be using a ruleset in the webcontents at all, the top level page must be activated. Thus, we can only reach 0 activated_frame_hosts when the top frame goes away. In this case, we shouldn't have any ongoing activation throttles. However, I do not feel totally comfortable with this reasoning, especially with calling this in RenderFrameDeleted(). It feels fragile to assume that navigation handles will always be torn down before the top RenderFrameHost goes away. https://codereview.chromium.org/2691423006/diff/280001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2691423006/diff/280001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_throttle_manager_unittest.cc:40: enum PageActivationThrottleState { On 2017/03/20 18:58:14, engedy wrote: > nit: PageActivationNotificationTiming? Done.
Description was changed from ========== Introduce the ThrottleManager BUG=637415 ========== to ========== Introduce the ThrottleManager This patch adds a new class responsible for orchestrating the various navigation throttles associated with the subresource_filter. So far it is only exercised in unit tests. TBR=shivanisha@chromium.org BUG=637415 ==========
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/2691423006/#ps300001 (title: "engedy 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": 300001, "attempt_start_ts": 1490040524435900,
"parent_rev": "8eabf60aaa6009c9383970ffbc0cfa15e50b9951", "commit_rev":
"47c0e8d177ffdb35e1e58650993200a1832e46f1"}
Message was sent while issue was closed.
Description was changed from ========== Introduce the ThrottleManager This patch adds a new class responsible for orchestrating the various navigation throttles associated with the subresource_filter. So far it is only exercised in unit tests. TBR=shivanisha@chromium.org BUG=637415 ========== to ========== Introduce the ThrottleManager This patch adds a new class responsible for orchestrating the various navigation throttles associated with the subresource_filter. So far it is only exercised in unit tests. TBR=shivanisha@chromium.org BUG=637415 Review-Url: https://codereview.chromium.org/2691423006 Cr-Commit-Position: refs/heads/master@{#458188} Committed: https://chromium.googlesource.com/chromium/src/+/47c0e8d177ffdb35e1e586509932... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/47c0e8d177ffdb35e1e586509932...
Message was sent while issue was closed.
On 2017/03/20 at 21:20:12, commit-bot wrote: > Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/47c0e8d177ffdb35e1e586509932... csharrison@, Sure, I will TBR. It might be a few days before I do as I am caught up in the cache lock work right now. |
