Do not immediately block cross-renderer extension resource loads for non-web-triggered transitions

Do not immediately block cross-renderer extension resource loads for non-web-triggered transitions

This is a very conservative fix designed to fix the regression.
The page transition logic was and is not in sync with the renderer-side check, see https://crbug.com/633963#c14.

BUG=633963
TEST=manually using steps from bug report

Committed: https://crrev.com/45ec6fecc32bff5541c237c98228122f3252902e
Cr-Commit-Position: refs/heads/master@{#413258}

[robwu, 2016-08-17 10:13:57]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 10:14:11]

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

[robwu, 2016-08-17 10:15:23]

rob@robwu.nl changed reviewers:
+ nasko@chromium.org, rdevlin.cronin@chromium.org

[robwu, 2016-08-17 10:15:23]

rob@robwu.nl changed required reviewers:
+ rdevlin.cronin@chromium.org

[robwu, 2016-08-17 10:15:23]

Devlin: PTAL
Nasko: FYI

[commit-bot: I haz the power, 2016-08-17 10:43:55]

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

[commit-bot: I haz the power, 2016-08-17 10:43:56]

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

[robwu, 2016-08-17 12:36:17]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 12:36:33]

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, 2016-08-17 13:11:26]

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

[commit-bot: I haz the power, 2016-08-17 13:11:27]

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

[nasko, 2016-08-17 16:31:44]

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
File extensions/browser/url_request_util.cc (left):

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
extensions/browser/url_request_util.cc:52: if
(!ui::PageTransitionIsWebTriggerable(info->GetPageTransition())) {
As per my response in the bug, I think this should be moved at the end of all
the checks, not completely removed.

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
File extensions/browser/url_request_util.cc (right):

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
extensions/browser/url_request_util.cc:47: return false;
Why this rewrite in this patch? I think the goal is to have it backported to M53
before it hits stable, which means it should be minimally invasive.

[robwu, 2016-08-18 08:03:34]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 08:03:47]

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

[robwu, 2016-08-18 08:08:02]

Description was changed from

==========
Do not block cross-renderer extension resource loads for non-web-triggered
transitions

See https://crbug.com/633963#c14 for why this change makes sense.

BUG=633963
TEST=manually using steps from bug report
==========

to

==========
Do not immediately block cross-renderer extension resource loads for
non-web-triggered transitions

This is a very conservative fix designed to fix the regression.
The page transition logic was and is not in sync with the renderer-side check,
see https://crbug.com/633963#c14.

BUG=633963
TEST=manually using steps from bug report
==========

[robwu, 2016-08-18 08:09:58]

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
File extensions/browser/url_request_util.cc (left):

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
extensions/browser/url_request_util.cc:52: if
(!ui::PageTransitionIsWebTriggerable(info->GetPageTransition())) {
On 2016/08/17 16:31:44, nasko (out until 08-30) wrote:
> As per my response in the bug, I think this should be moved at the end of all
> the checks, not completely removed.

I restored this check and moved it to the end out of caution, but I do believe
that this check should be removed.
Or, if the check is to be kept, then the renderer-side check should be adjusted
to match the logic here.

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
File extensions/browser/url_request_util.cc (right):

https://codereview.chromium.org/2249423002/diff/20001/extensions/browser/url_...
extensions/browser/url_request_util.cc:47: return false;
On 2016/08/17 16:31:44, nasko (out until 08-30) wrote:
> Why this rewrite in this patch? I think the goal is to have it backported to
M53
> before it hits stable, which means it should be minimally invasive.

Without this change, the following test would fail when I remove the page
transition check:
WebViewTests/WebViewTest.Shim_TestLoadAbortChromeExtensionURLWrongPartition
WebViewTests/WebViewPluginTest.TestLoadPluginInternalResource

The transition in these tests is PAGE_TRANSITION_AUTO_TOPLEVEL, which explains
why the requests were previously rejected.

If I follow your suggestion of moving the transition check to the end of the
method, then these tests fail again. So I've restored the check, but limited it
to when |is_guest| is true.

[commit-bot: I haz the power, 2016-08-18 08:46:27]

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

[commit-bot: I haz the power, 2016-08-18 08:46:28]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-18 16:58:40]

(taking over for nasko since he's ooo)

https://codereview.chromium.org/2249423002/diff/40001/extensions/browser/url_...
File extensions/browser/url_request_util.cc (right):

https://codereview.chromium.org/2249423002/diff/40001/extensions/browser/url_...
extensions/browser/url_request_util.cc:52: if (is_guest &&
we should add a comment explaining this.

https://codereview.chromium.org/2249423002/diff/40001/extensions/browser/url_...
extensions/browser/url_request_util.cc:53:
!ui::PageTransitionIsWebTriggerable(info->GetPageTransition())) {
let's cache this in bool is_web_triggerable_transition.

https://codereview.chromium.org/2249423002/diff/40001/extensions/browser/url_...
extensions/browser/url_request_util.cc:122: if
(!ui::PageTransitionIsWebTriggerable(info->GetPageTransition())) {
and a comment/todo for this (especially if you think it should be removed).

[Devlin, 2016-08-19 01:33:30]

LGTM so this isn't blocked (was waiting to see the wording of the comments, but
we can work that out in follow-ups if needed)

[robwu, 2016-08-19 17:52:35]

I can't update the patch right now, so I'll tick the commit box to land it
and get some bake time. The commit message includes an explanation for why
those checks exist.

The page transition check is inexpensive (and most commonly not even
checked twice) so not addressing the nit is not that big of an issue.

On Aug 18, 2016 6:33 PM, <rdevlin.cronin@chromium.org> wrote:

> LGTM so this isn't blocked (was waiting to see the wording of the
> comments, but
> we can work that out in follow-ups if needed)
>
> https://codereview.chromium.org/2249423002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robwu, 2016-08-19 17:53:43]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-08-19 17:54:18]

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, 2016-08-19 19:54:52]

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

[commit-bot: I haz the power, 2016-08-19 19:54:54]

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

[robwu, 2016-08-19 20:03:39]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-08-19 20:04:15]

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, 2016-08-19 21:48:23]

Description was changed from

==========
Do not immediately block cross-renderer extension resource loads for
non-web-triggered transitions

This is a very conservative fix designed to fix the regression.
The page transition logic was and is not in sync with the renderer-side check,
see https://crbug.com/633963#c14.

BUG=633963
TEST=manually using steps from bug report
==========

to

==========
Do not immediately block cross-renderer extension resource loads for
non-web-triggered transitions

This is a very conservative fix designed to fix the regression.
The page transition logic was and is not in sync with the renderer-side check,
see https://crbug.com/633963#c14.

BUG=633963
TEST=manually using steps from bug report
==========

[commit-bot: I haz the power, 2016-08-19 21:48:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-19 21:52:45]

Description was changed from

==========
Do not immediately block cross-renderer extension resource loads for
non-web-triggered transitions

This is a very conservative fix designed to fix the regression.
The page transition logic was and is not in sync with the renderer-side check,
see https://crbug.com/633963#c14.

BUG=633963
TEST=manually using steps from bug report
==========

to

==========
Do not immediately block cross-renderer extension resource loads for
non-web-triggered transitions

This is a very conservative fix designed to fix the regression.
The page transition logic was and is not in sync with the renderer-side check,
see https://crbug.com/633963#c14.

BUG=633963
TEST=manually using steps from bug report

Committed: https://crrev.com/45ec6fecc32bff5541c237c98228122f3252902e
Cr-Commit-Position: refs/heads/master@{#413258}
==========

[commit-bot: I haz the power, 2016-08-19 21:52:46]

Patchset 3 (id:??) landed as
https://crrev.com/45ec6fecc32bff5541c237c98228122f3252902e
Cr-Commit-Position: refs/heads/master@{#413258}