Adding release block label, please undo if not the case.

Please do a "narrow bisect" and figure out why this broke.

M53 Stable launch is coming soon.Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix asap so it gets chance to bake in beta before stable promotion. Thank you.

I bisected: https://chromium.googlesource.com/chromium/src/+log/64ad58e1d8451857f6a763651727c9d39375ea3b..a4a128d5c500528e70139c14c51bb326fe357746

r397066 -> nasko

Adding asargent@ to see if he can help. I am going to be away for non-trivial amount of time in August, so I don't know when I'll have the time to look at this issue.

BTW, I think if I reload the PDF twice, it loads up the second time. It is rather strange and it would be nice to fix. IMO this probably doesn't have to be a release blocker, because not many people switch PDF viewers like this every day, so the impact is low. Though I wonder if there are other use cases that are similiarly affected.

@thestig: As per your comment #6, if the above issue is of low severity then do remove the blocker label. We have a M53 stable release scheduled in coming few weeks.

I really appreciate your help.


Thank you! 

M53 Stable launch is coming VERY soon.Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix asap so it gets chance to bake in beta before stable promotion later this month. Thank you.

Given this is rare case and based on comment#6 removing the stable blocker label. Please re-tag it if you think other wise.

The issue is that the checks in AllowCrossRendererResourceLoad disallow PageTransition that is not triggerable by the web. Even after installing the extension and just navigating in a tab, the same error is hit because the transition is TYPED | ADDRESS_BAR, which is not web triggerable. However, a reload doesn't go through AllowCrossRendererResourceLoad.

The main question is why does a top-level navigation to a PDF file result in call to AllowCrossRendererResourceLoad. I'll poke at this and post an update once I have more data.

+Rob FYI.

Nasko and I chatted about this a bit offline and TL;DR I do think this should be working, and that there's something we need to change in our AllowCrossRendererResourceLoad logic.

AllowCrossRendererResourceLoad is hit because the extension does a redirect for a request coming from regular renderer process to an extension URL.

It seems like a scenario we should support, but I need to think through the implications of allowing the redirect to go through. It might be a safe first step to just whitelist the page transition indicating omnibox navigation.

Marking the bug as available, as I will not be able to fix it in time for branch cut. If anyone else wants to take a stab at it, go ahead. 

I think that it's safe to remove the transition check.

The PageTransitionIsWebTriggerable logic was introduced in  https://crbug.com/137435#c12 , and was used to disable the web_accessible_resources check for non-web transitions. In other words, it allowed any non-web-triggered transition.

71	    bool transition_allowed =
72	        !content::PageTransitionIsWebTriggerable(transition_type);
73	
74	    if (!is_empty_origin && !is_own_resource &&
75	        !is_dev_tools && !transition_allowed) {
  ....
84	      return false; // <-- Block request
85	    }
86	  }
87	
88	  return true; // <-- Allow request if not web-triggerable

Today, the check is still similar: https://chromium.googlesource.com/chromium/src/+/71331253d6537b9409518dec2368388c5d73cb94/chrome/renderer/extensions/resource_request_policy.cc#93

If we only look at the transition_allowed variable, we get this:

71	    bool transition_allowed =
72	        !content::PageTransitionIsWebTriggerable(transition_type);
75	    if (!transition_allowed)
84	      return false; // <-- Block request
88	  return true; // <-- Allow request if not web-triggerable

Inlining the variable..

75	    if (!!content::PageTransitionIsWebTriggerable(transition_type)) // <-- Note: Double negation.
84	      return false; // <-- Block request
88	  return true; // <-- Allow request if not web-triggerable

The above renderer logic was re-implemented in the browser in  https://crbug.com/173688#c14  (https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_protocols.cc?revision=192121&pathrev=192121#l309)
... except the logic is
1) put at the wrong place (i.e. before the web_accessible_resources check) and
2) inverted.

309	  if (!content::PageTransitionIsWebTriggerable(info->GetPageTransition()))
310	    return false;

If anything, the request should unconditionally be allowed if the page transition is non-web-triggered.
The only thing that stops me from unconditionally accepting the request is that in the renderer the condition is combined with 3 other variables, whose state is uncertain in the browser. So it is safer to not check for the page transition and add it back later if needed.

I am applying the RB again because the patch is underway and contrary comment 7, this is not a rare case (and luckily it has not regressed on stable yet). My extension alone amounts to a half million users who would surely notice if this hits stable.

Thanks Rob for the investigation. However, I'm not convinced that the check should be entirely removed. I think it is fine to move it to be the last check in that method. 
The reason is that we can't trust the renderer process for security decisions, so we need to have the browser check them as well. Moving it to the end will allow all other conditions that we know we allow to take precedence and still disallow a renderer requesting a subresource fine. Since there is already a check for main frame or subframe requests, regular navigations will be handled in that block:

92	  if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
93	    *allowed = true;
94	    return true;
95	  } else if (info->GetResourceType() == content::RESOURCE_TYPE_SUB_FRAME) {

It doesn't make sense to allow subresources to have non-web triggerable transitions as per definition it is web content that is making the request.

I'll move the check to the end out of caution (for merging back), but the dissimilarity between the browser and renderer logic is very odd.

Did you have any specific reason for inverting the logic when you wrote the patch at  https://crbug.com/173688#c14 ? And if so, why did you not update the renderer-side of the check?

Please try to resolve this ASAP as we're very close to M53 Stable promotion. Please request a merge to M53 branch 2785 once change is landed/baked/verified in Canary. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45ec6fecc32bff5541c237c98228122f3252902e

commit 45ec6fecc32bff5541c237c98228122f3252902e
Author: rob <rob@robwu.nl>
Date: Fri Aug 19 21:48:01 2016

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

Review-Url: https://codereview.chromium.org/2249423002
Cr-Commit-Position: refs/heads/master@{#413258}

[modify] https://crrev.com/45ec6fecc32bff5541c237c98228122f3252902e/chrome/browser/extensions/chrome_url_request_util.cc
[modify] https://crrev.com/45ec6fecc32bff5541c237c98228122f3252902e/extensions/browser/url_request_util.cc

Please request a merge to M53 as soon as CL is baked/verified in Canary and safe to merge. Thank you.

Confirmed fixed in 54.0.2836.0, requesting to merge 45ec6fecc32bff5541c237c98228122f3252902e.

The patch resolves the regression, but I believe that the logic can be improved (comment 14), so I will keep this open and wait until Nasko returns.

Your change meets the bar and is auto-approved for M53 (branch: 2785)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a58871cac6284a5167c1cf039127d270c8a6fb8b

commit a58871cac6284a5167c1cf039127d270c8a6fb8b
Author: Rob Wu <rob@robwu.nl>
Date: Sun Aug 21 09:54:15 2016

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

Review-Url: https://codereview.chromium.org/2249423002
Cr-Commit-Position: refs/heads/master@{#413258}
(cherry picked from commit 45ec6fecc32bff5541c237c98228122f3252902e)

Review URL: https://codereview.chromium.org/2261043002 .

Cr-Commit-Position: refs/branch-heads/2785@{#695}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/a58871cac6284a5167c1cf039127d270c8a6fb8b/chrome/browser/extensions/chrome_url_request_util.cc
[modify] https://crrev.com/a58871cac6284a5167c1cf039127d270c8a6fb8b/extensions/browser/url_request_util.cc

Verified the fix on Windows 7, MAC (10.11.6) & Ubuntu Trusty (14.04) for Google Chrome Version - 54.0.2835.0 

Screen-recording is attached.

TE-Verified labels are added.

@rob: Please change the status of this issue.

Thank you.

Deleted: 633963.mp4 2.1 MB

	633963.mp4 2.1 MB View Download

 Issue 638844  has been merged into this issue.

 Issue 639140  has been merged into this issue.

Marking this as fixed to properly reflect the status of the regression.
See  bug 640072  for the follow-up.

Rechecked this issue on chrome version 53.0.2785.80 on Windows 7, Ubuntu 14.04 and MAC 10.11.6. Merge is working as intended.

No error page is displayed when refreshed the pdf page after adding the extension.

Adding TE-verified labels.

Thanks.!

Tried the bisect with the new script and was able to narrow down to a single culprit CL.

Regression Range
================
https://chromium.googlesource.com/chromium/src/+log/97a40b7bad083e7f9382a92e00adf1bd650db4a5..5cf9d45c437b7b2d899e46f2f324c147a2743eb7