Refactor subresource filter activation to support PlzNavigate cleanly.

Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (which is sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1] to suppress resetting the activation state if the URL of the provisional
load matched the URL for which activation was intended (sent along in the IPC).

This CL removes this workaround, and refactors activation so that:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time as long as it arrives
    before DidCommitProvisionalLoad (but after the prior load committing).
 -- As a general rule, no activation IPC message is sent anymore for failed
    navigations:
   -- This has already been the case for aborts; plus for failed navigations
      when PlzNavigate is disabled. (ReadyToCommitNavigation has never been
      invoked in these scenarios.)
   -- For failed navigations when PlzNavigate is enabled, there is now an
      explicit suppression for sending the activation message.
 -- In the edge case when the provisional load fails on the renderer-side
    after FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415
Review-Url: https://codereview.chromium.org/2669833004
Cr-Commit-Position: refs/heads/master@{#450681}
Committed: https://chromium.googlesource.com/chromium/src/+/c53cedcb18d6344ae238ef6290bcbd0308bc40a6

[engedy, 2017-02-03 13:23:03]

Description was changed from

==========
Refactor subresource filter activation to have proper PlzNavigate support.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
used to expect that an activation IPC message will arrive between
DidStartProvisionalLoad and DidCommitProvisionalLoad if the decision was made
on the browser side wanted to activate filtering for a given navigation. By
default, if no message arrived, that meant no activation for a given load,
therefore the SubresourceFilter used to reset the activation state to DISABLED
in DidStartProvisionalLoad. (This was also the mechanism to undo activation in
case the provisional load did not commit.)

With PlzNavigate, however, the activation IPC message (sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which meant that the activation state got reset immediately after being set
from the IPC. A workaround was implemented in [1] to suppress this if the URL
of the provisional load matched the URL (sent along in the IPC) for which
activation was intended.

This CL removes this somewhat hacky solution, and refactors activation the
following way:
 -- DidStartProvisionalLoad plays no role anymore.
 -- The activation IPC message can now be sent any time before
    DidCommitProvisionalLoad.
 -- In case of an activation IPC followed by the provisional load failing, the
    activation is now undone in response to DidFailProvisionalLoad, so that
    there will be no activation for a subsequent successful load.
 -- There is no DidFailProvisionalLoad with PlzNavigate, in which case there
    might be activation on the subsequently rendered error page, which shouldA
    have no effect.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

to

==========
Refactor subresource filter activation to support PlzNavigate properly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
used to expect that an activation IPC message will arrive between
DidStartProvisionalLoad and DidCommitProvisionalLoad if the decision was made
on the browser side wanted to activate filtering for a given navigation. By
default, if no message arrived, that meant no activation for a given load,
therefore the SubresourceFilter used to reset the activation state to DISABLED
in DidStartProvisionalLoad. (This was also the mechanism to undo activation in
case the provisional load did not commit.)

With PlzNavigate, however, the activation IPC message (sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which meant that the activation state got reset immediately after being set
from the IPC. A workaround was implemented in [1] to suppress this if the URL
of the provisional load matched the URL (sent along in the IPC) for which
activation was intended.

This CL removes this somewhat hacky solution, and refactors activation the
following way:
 -- DidStartProvisionalLoad plays no role anymore.
 -- The activation IPC message can now be sent any time before
    DidCommitProvisionalLoad.
 -- In case of an activation IPC followed by the provisional load failing, the
    activation is now undone in response to DidFailProvisionalLoad, so that
    there will be no activation for a subsequent successful load.
 -- There is no DidFailProvisionalLoad with PlzNavigate, in which case there
    might be activation on the subsequently rendered error page, which shouldA
    have no effect.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

[engedy, 2017-02-03 15:27:31]

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

[commit-bot: I haz the power, 2017-02-03 15:27:44]

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-02-03 15:29:28]

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

[commit-bot: I haz the power, 2017-02-03 15:29:29]

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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[engedy, 2017-02-03 15:41:50]

Description was changed from

==========
Refactor subresource filter activation to support PlzNavigate properly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
used to expect that an activation IPC message will arrive between
DidStartProvisionalLoad and DidCommitProvisionalLoad if the decision was made
on the browser side wanted to activate filtering for a given navigation. By
default, if no message arrived, that meant no activation for a given load,
therefore the SubresourceFilter used to reset the activation state to DISABLED
in DidStartProvisionalLoad. (This was also the mechanism to undo activation in
case the provisional load did not commit.)

With PlzNavigate, however, the activation IPC message (sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which meant that the activation state got reset immediately after being set
from the IPC. A workaround was implemented in [1] to suppress this if the URL
of the provisional load matched the URL (sent along in the IPC) for which
activation was intended.

This CL removes this somewhat hacky solution, and refactors activation the
following way:
 -- DidStartProvisionalLoad plays no role anymore.
 -- The activation IPC message can now be sent any time before
    DidCommitProvisionalLoad.
 -- In case of an activation IPC followed by the provisional load failing, the
    activation is now undone in response to DidFailProvisionalLoad, so that
    there will be no activation for a subsequent successful load.
 -- There is no DidFailProvisionalLoad with PlzNavigate, in which case there
    might be activation on the subsequently rendered error page, which shouldA
    have no effect.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

to

==========
Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1], which suppresses resetting the activation state if the URL the provisional
load matches the URL (sent along in the IPC) for which activation is intended.

This CL removes this somewhat hacky solution, and refactors activation the
following way:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time before
    DidCommitProvisionalLoad.
 -- PlzNavigate: For failed navigations, even though ReadyToCommitNavigation
    is invoked, no activation IPC message is sent anymore.
 -- PlzNavigate: if the provisional load fails on the renderer-side after
    FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

[engedy, 2017-02-03 15:51:07]

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

[commit-bot: I haz the power, 2017-02-03 15:51:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-02-03 16:00:40]

Description was changed from

==========
Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1], which suppresses resetting the activation state if the URL the provisional
load matches the URL (sent along in the IPC) for which activation is intended.

This CL removes this somewhat hacky solution, and refactors activation the
following way:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time before
    DidCommitProvisionalLoad.
 -- PlzNavigate: For failed navigations, even though ReadyToCommitNavigation
    is invoked, no activation IPC message is sent anymore.
 -- PlzNavigate: if the provisional load fails on the renderer-side after
    FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

to

==========
Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (which is sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1] to suppress resetting the activation state if the URL of the provisional
load matches the URL for which activation is intended (sent along in the IPC).

This CL removes this workaround, and refactors activation so that:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time as long as it arrives
    before DidCommitProvisionalLoad (but after the prior load committing).
 -- As a general rule, no activation IPC message is sent anymore for failed
    navigations:
   -- This has already been the case for aborts, and for failed navigations
      when PlzNavigate is disabled (ReadyToCommitNavigation has never been
      invoked in these scenarios).
   -- For failed navigations when PlzNavigate is enabled, there is now an
      explicit suppression for sending the activation message.
 -- In the edge case when the provisional load fails on the renderer-side
    after FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

[engedy, 2017-02-03 16:00:40]

engedy@chromium.org changed reviewers:
+ clamy@chromium.org, pkalinnikov@chromium.org

[engedy, 2017-02-03 16:01:28]

@Pavel, @Camille, please review.
@Tanja, FYI.

[commit-bot: I haz the power, 2017-02-03 17:17:58]

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

[commit-bot: I haz the power, 2017-02-03 17:17:59]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-02-06 11:44:44]

LGTM % nits.

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:167:
// TODO(engedy): Add a test where this is needed (unsupported MIME type?)
nit: Add period.

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
File components/subresource_filter/content/renderer/subresource_filter_agent.h
(right):

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
components/subresource_filter/content/renderer/subresource_filter_agent.h:81:
ActivationLevel activation_level_from_next_commit_ =
nit: I would s/from/for/ here and below.

[engedy, 2017-02-07 12:47:07]

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:167:
// TODO(engedy): Add a test where this is needed (unsupported MIME type?)
On 2017/02/06 11:44:44, pkalinnikov wrote:
> nit: Add period.

Actually, I'm planning to address this TODO before landing. Camille, do you have
a good test scenario for us here?

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
File components/subresource_filter/content/renderer/subresource_filter_agent.h
(right):

https://codereview.chromium.org/2669833004/diff/60001/components/subresource_...
components/subresource_filter/content/renderer/subresource_filter_agent.h:81:
ActivationLevel activation_level_from_next_commit_ =
On 2017/02/06 11:44:44, pkalinnikov wrote:
> nit: I would s/from/for/ here and below.

Done.

[engedy, 2017-02-07 12:48:09]

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

[commit-bot: I haz the power, 2017-02-07 12:48:22]

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-02-07 12:50:15]

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

[commit-bot: I haz the power, 2017-02-07 12:50:16]

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...)

[engedy, 2017-02-07 13:09:29]

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

[commit-bot: I haz the power, 2017-02-07 13:09:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-02-07 13:09:56]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-02-07 13:53:12]

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

[commit-bot: I haz the power, 2017-02-07 13:53:13]

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

[engedy, 2017-02-08 10:21:23]

@Camille, friendly ping.

[engedy, 2017-02-08 12:52:24]

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

[commit-bot: I haz the power, 2017-02-08 12:52:42]

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-02-08 14:37:43]

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

[commit-bot: I haz the power, 2017-02-08 14:37:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[clamy, 2017-02-08 17:07:56]

@engedy: sorry I didn't have time to look at this today, will do a review
tomorrow.

[engedy, 2017-02-08 17:23:42]

On 2017/02/08 17:07:56, clamy (slow - catching up) wrote:
> @engedy: sorry I didn't have time to look at this today, will do a review
> tomorrow.

No worries, and thanks for the heads-up!

[clamy, 2017-02-09 10:32:07]

Thanks! This looks like a nice refactoring. A few questions below.

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
File components/subresource_filter/content/browser/DEPS (right):

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/browser/DEPS:5: "+net/base",
Is this because you have to compare the error code of the NavigationHandle to
net::OK?

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:211:
// ReadyToCommitNavigation when browser-side-navigation is enabled.
Can you add a TODO to remove this function when crbug.com/688393 is fixed?
Thanks!

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:322:
// called in production code. It is called with browser-side navigation
I'm not sure I understand this comment. We should call ReadyToCommitNavigation
both with and without PlzNavigate in production code. The issue is with the
testing code.

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:168: 
nit: maybe you could put the following two lines in a helper function (example
name ResetActivationStateForNextCommit) that you would call from here and from
DidFailProvisionalLoad?

[engedy, 2017-02-09 13:40:37]

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

[commit-bot: I haz the power, 2017-02-09 13:40:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-02-09 13:42:01]

Please take another look.

(Sorry for rebasing and addressing comments in the same patch set, there was a
lot of overlap.)

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
File components/subresource_filter/content/browser/DEPS (right):

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/browser/DEPS:5: "+net/base",
On 2017/02/09 10:32:06, clamy (slow - catching up) wrote:
> Is this because you have to compare the error code of the NavigationHandle to
> net::OK?

Yeah. I wasn't sure if it's OK to sneakily just rely on transitive includes.

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:211:
// ReadyToCommitNavigation when browser-side-navigation is enabled.
On 2017/02/09 10:32:06, clamy (slow - catching up) wrote:
> Can you add a TODO to remove this function when crbug.com/688393 is fixed?
> Thanks!

Done.

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:322:
// called in production code. It is called with browser-side navigation
On 2017/02/09 10:32:06, clamy (slow - catching up) wrote:
> I'm not sure I understand this comment. We should call ReadyToCommitNavigation
> both with and without PlzNavigate in production code. The issue is with the
> testing code.

What I meant to write here: for *failed* navigations, ReadyToCommitNavigation is
invoked only for PlzNavigate, as otherwise the only hook into the network stack
is the NavigationResourceThrottle, and ResourceThrottle::WillProcessResponse is
not invoked by ThrottlingResourceHandler for failures such as network errors.

Clarified the comment.

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:168: 
On 2017/02/09 10:32:06, clamy (slow - catching up) wrote:
> nit: maybe you could put the following two lines in a helper function (example
> name ResetActivationStateForNextCommit) that you would call from here and from
> DidFailProvisionalLoad?

Done.

[clamy, 2017-02-09 14:09:42]

Thanks! Lgtm.

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
File components/subresource_filter/content/browser/DEPS (right):

https://codereview.chromium.org/2669833004/diff/160001/components/subresource...
components/subresource_filter/content/browser/DEPS:5: "+net/base",
On 2017/02/09 13:42:01, engedy wrote:
> On 2017/02/09 10:32:06, clamy (slow - catching up) wrote:
> > Is this because you have to compare the error code of the NavigationHandle
to
> > net::OK?
> 
> Yeah. I wasn't sure if it's OK to sneakily just rely on transitive includes.

It's fine, just wanted to check if there was another usage.

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:179:
// TODO(engedy): Add a test where this is needed (unsupported MIME type?)
nit: add a . at the end of the comment.

[engedy, 2017-02-09 14:11:05]

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:179:
// TODO(engedy): Add a test where this is needed (unsupported MIME type?)
On 2017/02/09 14:09:42, clamy (slow - catching up) wrote:
> nit: add a . at the end of the comment.

Actually, can you recommend a scenario to exercise this? Then I'd add a test
instead of the TODO.

[engedy, 2017-02-09 14:19:48]

engedy@chromium.org changed reviewers:
+ dcheng@chromium.org, mattm@chromium.org

[engedy, 2017-02-09 14:21:45]

@Daniel, please review IPC changes (one parameter removed).

@Matt, please review:
 -- minor refactoring in c/b/safe_browsing,
 -- plus adding DEPS on net/base for net_errors.h.

[engedy, 2017-02-09 14:22:46]

For clarity, IPC changes in `subresource_filter_messages.h`.

[commit-bot: I haz the power, 2017-02-09 14:33:55]

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

[commit-bot: I haz the power, 2017-02-09 14:33:56]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-02-09 14:49:24]

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:179:
// TODO(engedy): Add a test where this is needed (unsupported MIME type?)
On 2017/02/09 14:11:05, engedy wrote:
> On 2017/02/09 14:09:42, clamy (slow - catching up) wrote:
> > nit: add a . at the end of the comment.
> 
> Actually, can you recommend a scenario to exercise this? Then I'd add a test
> instead of the TODO.

I can think of two cases where this could be reached in PlzNavigate right now:
unsupported MIME type or a violation of the frame-ancestor directive (which we
check in the renderer). The frame-ancestor one might be the easiest to check.

Though be also aware of https://codereview.chromium.org/2687593002/ which will
cause DidFailProvisionalLoad in all case of navigation failures in PlzNavigate.
I think you already filter those beforehand so this should not impact this
patch.

[mattm, 2017-02-09 21:15:09]

lgtm

[dcheng, 2017-02-14 09:17:17]

ipc lgtm

(many apologies for the delayed review)

[engedy, 2017-02-15 12:07:38]

Description was changed from

==========
Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (which is sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1] to suppress resetting the activation state if the URL of the provisional
load matches the URL for which activation is intended (sent along in the IPC).

This CL removes this workaround, and refactors activation so that:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time as long as it arrives
    before DidCommitProvisionalLoad (but after the prior load committing).
 -- As a general rule, no activation IPC message is sent anymore for failed
    navigations:
   -- This has already been the case for aborts, and for failed navigations
      when PlzNavigate is disabled (ReadyToCommitNavigation has never been
      invoked in these scenarios).
   -- For failed navigations when PlzNavigate is enabled, there is now an
      explicit suppression for sending the activation message.
 -- In the edge case when the provisional load fails on the renderer-side
    after FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

to

==========
Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (which is sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1] to suppress resetting the activation state if the URL of the provisional
load matched the URL for which activation was intended (sent along in the IPC).

This CL removes this workaround, and refactors activation so that:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time as long as it arrives
    before DidCommitProvisionalLoad (but after the prior load committing).
 -- As a general rule, no activation IPC message is sent anymore for failed
    navigations:
   -- This has already been the case for aborts; plus for failed navigations
      when PlzNavigate is disabled. (ReadyToCommitNavigation has never been
      invoked in these scenarios.)
   -- For failed navigations when PlzNavigate is enabled, there is now an
      explicit suppression for sending the activation message.
 -- In the edge case when the provisional load fails on the renderer-side
    after FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

[engedy, 2017-02-15 12:10:31]

Thanks for the reviews!

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
File components/subresource_filter/content/renderer/subresource_filter_agent.cc
(right):

https://codereview.chromium.org/2669833004/diff/180001/components/subresource...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:179:
// TODO(engedy): Add a test where this is needed (unsupported MIME type?)
On 2017/02/09 14:49:24, clamy wrote:
> On 2017/02/09 14:11:05, engedy wrote:
> > On 2017/02/09 14:09:42, clamy (slow - catching up) wrote:
> > > nit: add a . at the end of the comment.
> > 
> > Actually, can you recommend a scenario to exercise this? Then I'd add a test
> > instead of the TODO.
> 
> I can think of two cases where this could be reached in PlzNavigate right now:
> unsupported MIME type or a violation of the frame-ancestor directive (which we
> check in the renderer). The frame-ancestor one might be the easiest to check.
> 
> Though be also aware of https://codereview.chromium.org/2687593002/ which will
> cause DidFailProvisionalLoad in all case of navigation failures in
PlzNavigate.
> I think you already filter those beforehand so this should not impact this
> patch.

Thanks! I have updated the TOOD here.

Let me add this test in a follow-up CL, as this is already the 5th time I had to
rebase this CL, and now I have a third CL depending on it.

[engedy, 2017-02-15 12:11:17]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-02-15 12:11:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkalinnikov@chromium.org, clamy@chromium.org, dcheng@chromium.org,
mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2669833004/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-15 12:11:37]

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-02-15 13:16:42]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487160677784520,
"parent_rev": "6e9b7247c80de98888c1d90ed61457538f51931a", "commit_rev":
"c53cedcb18d6344ae238ef6290bcbd0308bc40a6"}

[commit-bot: I haz the power, 2017-02-15 13:17:28]

Description was changed from

==========
Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (which is sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1] to suppress resetting the activation state if the URL of the provisional
load matched the URL for which activation was intended (sent along in the IPC).

This CL removes this workaround, and refactors activation so that:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time as long as it arrives
    before DidCommitProvisionalLoad (but after the prior load committing).
 -- As a general rule, no activation IPC message is sent anymore for failed
    navigations:
   -- This has already been the case for aborts; plus for failed navigations
      when PlzNavigate is disabled. (ReadyToCommitNavigation has never been
      invoked in these scenarios.)
   -- For failed navigations when PlzNavigate is enabled, there is now an
      explicit suppression for sending the activation message.
 -- In the edge case when the provisional load fails on the renderer-side
    after FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415
==========

to

==========
Refactor subresource filter activation to support PlzNavigate cleanly.

In a pre-PlzNavigate world, the SubresourceFilterAgent on the renderer side
expected that an activation IPC message would arrive between DidStart- and
DidCommitProvisionalLoad if the decision was made on the browser side to
activate filtering for a given navigation. By default, if no message arrived,
that meant no activation for a given load, therefore the SubresourceFilter used
to reset the activation state in DidStartProvisionalLoad to DISABLED. (This was
also the mechanism to undo activation in case the provisional load did not
commit.)

With PlzNavigate, however, the activation IPC message (which is sent from
ReadyToCommitNavigation) always arrives even before DidStartProvisionalLoad,
which means that the activation state would be reset immediately after being
set from the IPC, were it not for a workaround that has been implemented in
[1] to suppress resetting the activation state if the URL of the provisional
load matched the URL for which activation was intended (sent along in the IPC).

This CL removes this workaround, and refactors activation so that:
 -- DidStartProvisionalLoad does not play a role anymore.
 -- The activation IPC message can now be sent any time as long as it arrives
    before DidCommitProvisionalLoad (but after the prior load committing).
 -- As a general rule, no activation IPC message is sent anymore for failed
    navigations:
   -- This has already been the case for aborts; plus for failed navigations
      when PlzNavigate is disabled. (ReadyToCommitNavigation has never been
      invoked in these scenarios.)
   -- For failed navigations when PlzNavigate is enabled, there is now an
      explicit suppression for sending the activation message.
 -- In the edge case when the provisional load fails on the renderer-side
    after FrameMsg_CommitNavigation, the activation state is now reset in
    DidFailProvisionalLoad.

[1] https://codereview.chromium.org/2446503002

BUG=637415

Review-Url: https://codereview.chromium.org/2669833004
Cr-Commit-Position: refs/heads/master@{#450681}
Committed:
https://chromium.googlesource.com/chromium/src/+/c53cedcb18d6344ae238ef6290bc...
==========

[commit-bot: I haz the power, 2017-02-15 13:17:29]

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/c53cedcb18d6344ae238ef6290bc...