[subresource_filter] Remove some state from the driver factory

[subresource_filter] Remove some state from the driver factory

This CL removes almost all state from the driver factory.

In order to persist activation decisions (which are used for
logging metrics), we thread that data through the activation
state computing throttle, and store it in the throttle manager.

This changes the current behavior of the activation state
computing throttle, which previously was only notified when
activation is ENABLED or DRYRUN.

This CL also moves ActivationDecision into its own file, and
tweaks the safe browsing throttle to use a full-featured
throttle manager (i.e. one with a ruleset dealer) that
vends the proper navigation throttles, which is necessary
for verifying state aside from histograms.

BUG=todo

[Charlie Harrison, 2017-04-25 22:30:03]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 22:31:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 22:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 22:46:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-04-26 20:45:45]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 20:46:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-04-26 21:48:57]

Description was changed from

==========
[subresource_filter] Remove some state from the driver factor

This CL moves state from the driver factory to the activation state
computing throttle + the throttle manager.

We also thread ActivationDecision through the throttle, so that at
navigation commit, we always know what the activation decision is. This
particular change fixes an issue where activation is not persisted
when a navigation is cancelled before commit.

BUG=todo
==========

to

==========
[subresource_filter] Remove some state from the driver factor

This CL removes almost all state from the driver factory.

In order to persist activation decisions (which are used for
logging metrics), we thread that data through the activation
state computing throttle, and store it in the throttle manager.

This changes the current behavior of the activation state
computing throttle, which previously was only notified when
activation is ENABLED or DRYRUN.

This CL also moves ActivationDecision into its own file, and
tweaks the safe browsing throttle to use a full-featured
throttle manager (i.e. one with a ruleset dealer) that
vends the proper navigation throttles, which is necessary
for verifying state aside from histograms.

BUG=todo
==========

[commit-bot: I haz the power, 2017-04-26 21:54:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 21:54:33]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Charlie Harrison, 2017-04-26 21:56:06]

csharrison@chromium.org changed reviewers:
+ engedy@chromium.org

[Charlie Harrison, 2017-04-26 21:56:07]

engedy: PTAL for a high level look.

This is another step to killing the driver factory. Unfortunately if we care
about the ActivationDecision metrics it makes logging those a bit of a pain
(involves threading them through the activation state computing throtte).

Still, it feels good to remove these members.

Once the SB throttle lands we can remove the rest.

[Charlie Harrison, 2017-04-26 22:12:07]

Hmmm tests are failing because the metrics WCO has an implicit dependency on the
throttle manager WCO.

Moving the metrics WCO to be inserted after the throttle manager makes tests
pass again, but is relatively hacky.

Resetting the activation_decision_ in ReadyToCommitNavigation also is
unfortunate, because I've tried reasonably hard to make sure all the "resetting"
logic is in one place, DidFinishNavigation.

WDYT?

[Charlie Harrison, 2017-04-27 12:37:07]

csharrison@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Charlie Harrison, 2017-04-27 12:37:07]

Also +bmcquade for thoughts on dependency with metrics code.

[pkalinnikov, 2017-04-27 13:31:42]

Description was changed from

==========
[subresource_filter] Remove some state from the driver factor

This CL removes almost all state from the driver factory.

In order to persist activation decisions (which are used for
logging metrics), we thread that data through the activation
state computing throttle, and store it in the throttle manager.

This changes the current behavior of the activation state
computing throttle, which previously was only notified when
activation is ENABLED or DRYRUN.

This CL also moves ActivationDecision into its own file, and
tweaks the safe browsing throttle to use a full-featured
throttle manager (i.e. one with a ruleset dealer) that
vends the proper navigation throttles, which is necessary
for verifying state aside from histograms.

BUG=todo
==========

to

==========
[subresource_filter] Remove some state from the driver factory

This CL removes almost all state from the driver factory.

In order to persist activation decisions (which are used for
logging metrics), we thread that data through the activation
state computing throttle, and store it in the throttle manager.

This changes the current behavior of the activation state
computing throttle, which previously was only notified when
activation is ENABLED or DRYRUN.

This CL also moves ActivationDecision into its own file, and
tweaks the safe browsing throttle to use a full-featured
throttle manager (i.e. one with a ruleset dealer) that
vends the proper navigation throttles, which is necessary
for verifying state aside from histograms.

BUG=todo
==========

[engedy, 2017-04-27 13:59:52]

Overall, it would be nice if we could avoid passing values produced in the
effectively-chrome/ layer (the Safe BrowsingActivationThrottle) and consumed in
the chrome/ layer (page_load_metrics) through the effectively-content/ layer
(the ThrottleManager).

It looks like that even after we finish gutting out the CSFDriverFactory, we
will not want to instantiate the ThrottleManager directly from chrome/'s
AttachTabHelpers(). Instead, we would create an effectively-chrome/ layer
WebContentsObserver that would:
 -- own the ThrottleManager,
 -- be responsible for injecting the SBActivationThrottle and ThrottleManager's
throttles in the right order, so that we can avoid abominations like [1],
 -- could store either the |activation_decision| for the last committed load
(now in the chrome/ layer) or a mapping from NavigationHandles to
SBActivationThrottles so that the MetricsObserver could query the 
|activation_decision| directly from the throttle at DidFinishNavigation-time.

WDYT?

[1]:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...

https://codereview.chromium.org/2841933003/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
(right):

https://codereview.chromium.org/2841933003/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc:161:
subres_filter->throttle_manager()
I can see two somewhat acceptable options here (read my overall comments first):

 -- Keep the semantics of this method as is, but add enough delay here to make
sure we are not calling methods named `...ForLastCommittedPageLoad` until after
the page load really has been committed. :-)

 -- Store the |activation_decison| on the SafeBrowsingActivationThrottle so that
it can be accessed anytime after ReadyToCommit. Retrieve the throttle here from
the ContentSubresourceFilterDriverFactory replacement in exchange for giving it
the |navigation_handle|.

I think the second would be the nicer solution.

[Charlie Harrison, 2017-04-27 14:14:55]

Thanks Balazs. I agree with your general vision, and I think we can make the
ChromeSubresourceFilterClient the //chrome WCO that owns the throttle manager
longer term. 

 I think the second option is fine too. The one concern I have there is with
navigations that don't go through ReadyToCommitNavigation, were we want to reset
the decision.

I'm afraid this would still require logic in DidFinishNavigation (and hit the
ordering problem on that subset of navigations).

I think this is only a problem with about:blank (?), do you know any other edge
cases?

[engedy, 2017-04-27 14:26:15]

Maybe I misunderstood something. My thinking was that each SBActivationThrottle
would start out with ActivationDecision::DISABLED (or UNKNOWN), and if there is
no ReadyToCommitNavigation (in which cases we never activate), this value will
just never change and the MetricsObserver will correctly read out that we didn't
active.

[Charlie Harrison, 2017-04-27 14:37:42]

On 2017/04/27 14:26:15, engedy wrote:
> Maybe I misunderstood something. My thinking was that each
SBActivationThrottle
> would start out with ActivationDecision::DISABLED (or UNKNOWN), and if there
is
> no ReadyToCommitNavigation (in which cases we never activate), this value will
> just never change and the MetricsObserver will correctly read out that we
didn't
> active.

Yeah that's what will happen. I'm worried that this will wrongly persist
activation on edge case navigations like to data URLs. I'm just not sure there
aren't edge cases with ReadyToCommitNavigation that I'm not thinking of.

[Charlie Harrison, 2017-05-02 03:42:22]

Actually, I think I've misunderstood your suggestion. We can retrieve the
activation decision from the SB throttle at this point, it just requires another
map from Navigation Handle -> throttle which is reasonable. Let me code it up.

[engedy, 2017-05-02 10:45:15]

On 2017/05/02 03:42:22, Charlie (ooo-ish until may 2) wrote:
> Actually, I think I've misunderstood your suggestion. We can retrieve the
> activation decision from the SB throttle at this point, it just requires
another
> map from Navigation Handle -> throttle which is reasonable. Let me code it up.

Yes, that's what I had in mind. Sorry for the lack of clarity.

[Charlie Harrison, 2017-05-02 20:30:35]

Let's close this for now.