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

It's not r407603. That change was just changing auto to auto* for raw pointers.

From above Narrow bisect range,re-assigning the suspect:r407585?

Can we get a bisect to a single CL now?

#3 has nothing to do with this bug, it just switches 2 statement to avoid a use-after-free bug, and one of the statement is UMA reporting, so doesn't really matter.

Requesting a per-revision bisect.

Friendly neighborhood extension triager. CC'ing myself to await results of narrow bisect. 

Issue is still reproducible on Mac 10.11.6, win10 using latest canary (55.0.2864.0)


Bisect result from Linux:
=========================
https://chromium.googlesource.com/chromium/src/+log/98fd60a637e0cd2609cb8fb909503bb6cb9b40c6..3e3be13746968b4e07c0ffdc707b05a14676e435

lukasza@, Could you please take a look into this.

If 3e3be13 (aka r407586) is indeed responsible for this regression (I am trying to repro on my machine right now to confirm), then it should be relatively easy and safe to revert this CL (well, probably just the changes in chrome/renderer/chrome_content_renderer_client.cc).

Yup - I can confirm that r407586 is responsible for this regression.

Thanks for looking into this, lukasza@.  Reverting is an option, but it might be more useful to understand why a cross-process POST submission isn't working in this case.  It would be better to fix that if we can.

This issue is tagged as Stable blocker. M54 Stable release is scheduled for the first week of OCT, please have the fix baked/verified in canary and request a merge to M54 ASAP.

The POST navigation request for http://export.highcharts.com/ gets handled by ExtensionViewHost::OpenURLFromTab and it always returns NULL, except for a few, special-cased window dispositions.  In the repro the params.disposition is set to 1 = WindowOpenDisposition.CURRENT_TAB.

The javascript in the extension used in the original repro is obfuscated and difficult to read, but AFAICT the navigation is trigerred by a form.submit() call - I see the following callstacks:

#1 0x7f87128d240a blink::HTMLFormElement::scheduleFormSubmission()
#2 0x7f87128d1f84 blink::HTMLFormElement::submit()
#3 0x7f871c004a5a v8::internal::FunctionCallbackArguments::Call()
#4 0x7f871c0aec62 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#5 0x7f871c0adaa5 v8::internal::Builtin_Impl_HandleApiCall()
#6 0x7f871c0ad6d1 v8::internal::Builtin_HandleApiCall()

[13486:13486:0919/160111:ERROR:chrome_content_renderer_client.cc(1098)] ChromeContentRendererClient::ShouldFork; url=http://export.highcharts.com/; stack=#0 0x7f872033903e base::debug::StackTrace::StackTrace()
#1 0x7f87220ec08d ChromeContentRendererClient::ShouldFork()
#2 0x7f871e73eda6 content::RenderFrameImpl::decidePolicyForNavigation()
#3 0x7f871a7dfb40 blink::FrameLoaderClientImpl::decidePolicyForNavigation()
#4 0x7f8712cb2051 blink::FrameLoader::shouldContinueForNavigationPolicy()
#5 0x7f8712cb0b5f blink::FrameLoader::startLoad()
#6 0x7f8712cad596 blink::FrameLoader::load()
#7 0x7f8712cc17e4 blink::ScheduledFormSubmission::fire()
#8 0x7f8712cbfbad blink::NavigationScheduler::navigateTask()

+rdevlin.cronin@

The big question is: Should ExtensionViewHost::OpenURLFromTab support WindowOpenDisposition::CURRENT_TAB?

Notes:

1. Window dispositions allowed by ExtensionViewHost::OpenURLFromTab have the following comment: 

      // Only allow these from hosts that are bound to a browser (e.g. popups).
      // Otherwise they are not driven by a user gesture.

   but in this case there *is* a user gesture (the extension submits a form in response
   to user clicking on "Download PNG image").

2. OpenURL is used not to navigate to a web page, but to *download* a png image.

Oh, I guess note #1 doesn't apply here, because in the repro we only look at window open disposition and don't check whether we are bound to a browser.  Nevermind.


Also - I've tried adding CURRENT_TAB to the list of handled dispositions in ExtensionViewHost::OpenURLFromTab and in this case:

1) HTTP request doesn't include the following header (compared to what happens prior to r407586): 

Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryiScts1BcYc2BX6de

2) The request is opened in a new tab.

Comments 14-17: Oh no, that's the OpenURLFromTab problem we're grappling with in  issue 647772 , with no good answers.  [+lfg, alexmos]

This is a very weird case.  If I understand correctly, the extension popup is trying to navigate itself to a web URL (via POST) to end up downloading something?  I'm curious what would happen if the URL 404'd and tried to show a web page.  I got the impression that we tried to disallow navigations to web URLs in popups.  There's probably better ways to kick off a download without initiating a navigation.

For back compat, though, maybe OpenURLFromTab and looking into the issues in comment 17 is an option?  Not sure if that's better or worse.  

lukasza@: Can you test what would happen if a normal extension page in a tab did a navigation to a web download URL?  I'm curious if that breaks in the same way, or if this is just a popup problem.  (Seems to be a common thing, along with  issue 647772 .)

RE: If I understand correctly, the extension popup is trying to navigate itself to a web URL (via POST) to end up downloading something?

Correct - AFAICT, the extension pop-up uses javascript to submit a form targeting a web page that generates a png based on POST data and replies with headers to treat the reply as an attachment/download:
...
Content-Type: image/png;charset=utf-8
Content-Disposition: attachment; filename=chart.png
...


RE:  issue 647772 

Per  https://crbug.com/647772#c1  I thought that the issue only happens when going through RenderFrameProxyHost::OnOpenURL.  In the repro above, we are going through RenderFrameHostImpl::OnOpenURL instead.

RE: Can you test what would happen if a normal extension page in a tab did a navigation to a web download URL?

It breaks in a similar way to what I described in the second part of #c17 (well, the same way, except that it doesn't open any new tabs).

My hunch currently is that OpenURL path is not plumbing through Content-Type header from the http request.  For example - NavigatorImpl::RequestOpenURL doesn't have an explicit parameter for content type nor a generic parameter for extra http headers.  Maybe content type can / should be added to content::ResourceRequestBody?  I'll try to add a test for this by cloning http/tests/navigation/form-targets-cross-site-frame-post.html layout test and adding form.enctype to the form (to check if it gets preserved with and without --site-per-process / i.e. with and without OpenUrl code path).

I've opened a separate bug to track the problem of dropping Content-Type header from http request, when going through "OpenURL" code path -  issue 648648 .

Let's continue using the current bug (i.e.  issue 646261 ) for

1) figuring out how to fix the regression in M54 (i.e. easy revert VS right long-term fix) and

2) what to do with CURRENT_TAB window disposition in ExtensionViewHost::OpenURLFromTab (and other delegates? :-) in the long-term.

Notes to myself to remind me of the outcome of a discussion between creis@, alexmos@, nasko@ and me:


In extension pop-ups and background pages:
- We want to block navigations to a webpage
- We want to allow navigations that end up treated as a download

In extension page in a tab, we want to allow renderer-initiated top-level navigation to a webpage (with --isolate-extensions this will transfer to a different renderer process so it is okay).


WebContentsDelegate::OpenURLFromTab is too early - at this point we don't yet know if a navigation will be a download (vs if it will load a web page).  Therefore, we should probably modify ExtensionViewHost::OpenURLFromTab to allow navigations with window disposition of CURRENT_TAB.

To still block navigations to a webpage in extension pop-ups and background pages, we can lean on WebContentsDelegate::ShouldTransferNavigation:
  // Allows the delegate to optionally cancel navigations that attempt to
  // transfer to a different process between the start of the network load and
  // commit.  Defaults to true.
  virtual bool ShouldTransferNavigation();
AFAIU, the check via WebContentsDelegate::ShouldTransferNavigation should be done in RenderFrameHostManager::OnCrossSiteResponse (at this point downloads were already handled, but we are still deciding whether to allow a cross-process transfer / navigation).  I think we just need to make the call to transferring_render_frame_host->frame_tree_node()->navigator()->RequestTransferURL conditional on ShouldTransferNavigation - this will automagically reset/clear cross_site_transferring_request_ and transfer_navigation_handle_ as well as call frame_tree_node_->DidStopLoading() if needed.


content-layer tests can probably be done with NoTransferRequestDelegate in content/browser/cross_site_transfer_browsertest.cc (maybe: one test that downloads still succeed + one test that navigations fail).  We would still need chrome-layer tests to make sure that web navigations in extension pop-ups (or background pages) fail (i.e. that WebContentsDelegate::ShouldTransferNavigation properly returns false in case of extension-related delegates).

Comment 22: Yes, that's essentially right.  And it's actually even easier-- ShouldTransferNavigation is already wired up correctly, being checked within RequestTransferURL.  If we return without doing the transfer, RFHM::OnCrossSiteResponse will reset the transferring request and cancel the URLRequest.  (I think this should work even after clamy's upcoming CL to remove CrossSiteResourceHandler.)

So, the only change that should be needed here is to support CURRENT_TAB from the extension delegate's OpenURLFromTab and return false from ShouldTransferNavigation.

2 interesting observations:


Observation #1 - CURRENT_TAB navigation in extension pop-up targets currently active *tab* (not the popup).

I think it is interesting to see what happens when I add CURRENT_TAB to the list of supported WindowOpenDispositions in ExtensionViewHost::OpenURLFromTab.  Note that ExtensionViewHost::OpenURLFromTab calls Browser::OpenURL(open_url_params) which will call Browser::OpenURLFromTab with |WebContents* source| argument set to NULL (rather than using |source| set to the WebContents associated with the extension pop-up).  This makes sense for WindowOpenDispositions other than CURRENT_TAB, but doesn't seem to make sense for CURRENT_TAB.

Do we want to stop such navigations (as claimed in #c22)?  Or maybe such navigations are okay if the web content opens in a currently active tab?  (I guess a different answer might apply to extension popups VS extension background pages...)


Observation #2 - absence of CURRENT_TAB in ExtensionViewHost::OpenURLFromTab meant that not only top-level navigations to web content were disallowed, but also top-level navigations to extension content (in same or other extension) were disallowed.

If we want to preserve the old behavior, than blocking only navigatations that *transfer* (by returning false from WebContentsDelegate::ShouldTransferNavigation(...)) is not sufficient.

OTOH, maybe the new behavior is okay?  (this is an open question - I don't know the answer...).

It turns out that navigating an extension popup via ShouldFork / OpenURL does *not* count as a "transfer" and therefore the idea of blocking web navigations by overriding ExtensionViewHost::ShouldTransferNavigations won't work :-(

Brainstorming some other ideas:

- Block by overriding ShouldAllowOpenURL.
    Pros: Called by NavigatorImpl::RequestOpenURL
          rather than by NavigatorImpl::RequestTransferURL
    Cons: Today much of context is missing in ShouldAllowOpenURL:
        - Is this a main-frame VS subframe navigation?
        - Are we navigating a chrome extensions site instance
          1) in a tab vs 2) in an extension pop-up or background page.

- Do not go through ShouldFork / OpenURL at all (for extensions only?)
  and then block via ExtensionViewHost::ShouldTransferNavigations.

WIP CL (so far I think I have all the tests I want for this bug) is at https://codereview.chromium.org/2352083003 (sharing so somebody else can
pick it up in case I get run over by a proverbial bus :-P ).


I want to say out loud that I assume that it is okay to allow a top-level navigation in an extension pop-up that tries to go to *another* extension.  Such navigation was disallowed before (by omitting CURRENT_TAB disposition), but should be fine going forward.  In other words - we only care about blocking top-level navigations (in extension pop-up or background page) to destinations that are not extensions (e.g. to web pages / to http URIs).

OTOH, after just letting CURRENT_TAB go through (e.g. in https://crrev.com/2352083003/#ps40001), I see that navigating a pop-up to another extension commits about:blank, rather than the target page.  I'll try to dig into this later.


I am also staring at various content interfaces implemented by chrome layer.  ContentBrowserClient in addition to ShouldAllowOpenURL(RPH*) also has MayReuseHost and IsSuitableHost(RPH*, GURL&) which look promising.  OTOH, I don't know if //chrome layer can look at an RPH* and figure out if this RPH hosts extensions.

ShouldAllowOpenURL is called too early today - blocking via ShouldAllowOpenURL would block downloads, because today ShouldAllowOpenURL is called from NavigatorImpl::RequestOpenURL (i.e. AFAICT before the response headers had a chance to come back and say "download please" via "Content-Disposition: attachment; filename=...").

MayReuseHost and IsSuitableHost are used to figure out if an existing RPH can be reused, but I don't see how they could be used to block navigations.

CanCommitURL failure kills the renderer process, so it probably shouldn't be used to block navigations.


I don't quite like the idea of avoiding OpenURL path for extensions (and hoping that this way we can block popup->web navigations elsewhere - maybe via ShouldTransferNavigations).  If we rely on OpenURL path for embedder checks in some scenarios, then we should make sure OpenURL always works (and fix issues if it doesn't).  Saying that OpenURL path is not good enough for extensions, but using it for other things seems wrong to me.


I am at a loss here right now - I don't know how to block popup->web navigations while allowing popup->web navigations that result in a download... :-(

When https://codereview.chromium.org/2321543002/ lands, we'll have knowledge of whether the navigation is resulting in a download/stream in the NavigationHandleImpl when we call OnWillProcessResponse. It would be possible to expose this information in the public API and have a NavigationThrottle use it to cancel or not the navigation.

Thanks for the investigation.

M-54 Stable release is scheduled during the first week of OCT, please have the fix baked/verified in canary and request a merge to M54 ASAP.

creis@, nasko@, alexmos@ and me met today to discuss options.  We think that we can backport something that 1) reverts essence of r407586 (i.e. avoids forking / OpenURL for POST) + 2) blocks ext->web via ShouldTransferNavigation (with extra is_main_frame_navigation parameter).  This approach seems to work fine in my local testing.  I am running more tryjobs at https://crrev.com/2352083003/#ps60001.  If the tryjobs pass, then I just need to add one more test (ext-in-tab -> web navigation) and hopefully push it out to review on Monday.

In the long-term we might want to avoid forking / OpenURL for more things (starting with both GET and POST navigations for extensions [but not necessarily hosted apps]).  OTOH, this seems too risky for backporting to M54.

A CL implementing the fix described in #c31 is up for review at https://crrev.com/2352083003.  I've verified that this CL alone is sufficient to fix the regression reported in the current bug (because avoiding OpenURL path automatically avoids the Content-Type problem from  issue 648648 ).

adharap@  please verify the fix in latest canary.

The fix has not landed yet.

Fixing the spelling error in label.

The fix has landed yesterday, but had to be reverted due to failures on Win10 Tests x64.  The fix is (in theory) platform-agnostic, so I am suspecting that the failures are related to the way tests are written and/or run.  I am tracking the failures in  issue 651106 .  If I can't figure out the root cause of the failures by end of day today, then I think we can reland with some of the tests disabled on Windows.

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

commit 953f8c785a134b2b4bd9ff401ff27f7edef4172c
Author: lukasza <lukasza@chromium.org>
Date: Tue Sep 27 19:04:14 2016

Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
    https://crbug.com/646261 

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
    https://crbug.com/648648 ).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term we want to make all extension navigations to not go
   through OpenURL path, but this seems too risky to merge back to M54,
   so for now we only do #1 above (i.e. avoid OpenURL only for POST
   requests).

BUG= 646261 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/browser/extensions/extension_view_host.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/browser/extensions/extension_view_host.h
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/renderer/chrome_content_renderer_client.cc
[add] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json
[add] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html
[add] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html
[add] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.js
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/browser/cross_site_transfer_browsertest.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/browser/frame_host/navigator_delegate.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/browser/frame_host/navigator_delegate.h
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c/content/public/browser/web_contents_delegate.h

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

commit 20d7fecd2c5d88e2666743c8d53d9d2b2ea47395
Author: lukasza <lukasza@chromium.org>
Date: Tue Sep 27 22:23:25 2016

Revert of Allow top-level navigation in extension pop-ups if it only triggers a download. (patchset #12 id:220001 of https://codereview.chromium.org/2352083003/ )

Reason for revert:
It seems that tests introduced in this CL are failing on some non-CQ bots: https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds/4653 (and also 4654 and 4655).

Original issue's description:
> Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).
>
> Before this CL, blocking of top-level navigations in extension pop-ups
> was accomplished by routing them via OpenURL path and blocking (or
> rather - silently dropping) CURRENT_TAB navigations via
> ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
> reasons:
>
> 1. This unnecessarily blocked navigations that end-up being treated
>    as downloads (i.e. because HTTP response says Content-Disposition:
>    attachment).  This was the root cause of the regression raised in
>     https://crbug.com/646261 
>
> 2. There are still some remaining issues in handling of POST requests
>    via OpenURL path (e.g. dropping Content-Type header -
>     https://crbug.com/648648 ).
>
> 3. In the long-term we want to rely less on process isolation
>    accomplished via OpenURL - an exploited renderer process does not
>    necessarily have to go through OpenURL path and can instead choose
>    to use the regular, renderer-initiated path.
>
> After this CL:
>
> 1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
>    navigations in extension pop-ups (and background pages).
>
> 2. POST navigations do not go through OpenURL path anymore (i.e. this CL
>    effectively reverts the essence of r407586).
>
>    In the long-term we want to make all extension navigations to not go
>    through OpenURL path, but this seems too risky to merge back to M54,
>    so for now we only do #1 above (i.e. avoid OpenURL only for POST
>    requests).
>
> BUG= 646261 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/953f8c785a134b2b4bd9ff401ff27f7edef4172c
> Cr-Commit-Position: refs/heads/master@{#421287}

TBR=creis@chromium.org,rdevlin.cronin@chromium.org,sky@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 646261 

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

[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/chrome/browser/extensions/extension_view_host.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/chrome/browser/extensions/extension_view_host.h
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/chrome/renderer/chrome_content_renderer_client.cc
[delete] https://crrev.com/db4d4f8411bc8c5d2efc3ac6aa901e062a8c4433/chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json
[delete] https://crrev.com/db4d4f8411bc8c5d2efc3ac6aa901e062a8c4433/chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html
[delete] https://crrev.com/db4d4f8411bc8c5d2efc3ac6aa901e062a8c4433/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html
[delete] https://crrev.com/db4d4f8411bc8c5d2efc3ac6aa901e062a8c4433/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.js
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/browser/cross_site_transfer_browsertest.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/browser/frame_host/navigator_delegate.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/browser/frame_host/navigator_delegate.h
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/20d7fecd2c5d88e2666743c8d53d9d2b2ea47395/content/public/browser/web_contents_delegate.h

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

commit 1d02573dbb13b5d7bd76f172afdf19a6389dc2f7
Author: lukasza <lukasza@chromium.org>
Date: Wed Sep 28 21:36:08 2016

Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).
(this is relanding a slightly tweaked r421287).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
    https://crbug.com/646261 

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
    https://crbug.com/648648 ).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term (tracked in https://crbug.com/650694) we want to
   make all extension navigations to not go through OpenURL path, but
   this seems too risky to merge back to M54, so for now we only do #2
   above (i.e. avoid OpenURL only for POST requests).

BUG= 646261 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=sky@chromium.org, rdevlin.cronin@chromium.org

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

[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/browser/extensions/extension_view_host.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/browser/extensions/extension_view_host.h
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/renderer/chrome_content_renderer_client.cc
[add] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json
[add] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html
[add] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html
[add] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.js
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/browser/cross_site_transfer_browsertest.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/browser/frame_host/navigator_delegate.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/browser/frame_host/navigator_delegate.h
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/1d02573dbb13b5d7bd76f172afdf19a6389dc2f7/content/public/browser/web_contents_delegate.h

Note:Above issue seems to be fixed on latest canary version i.e 55.0.2875.0

Thanks for verifying.  Since I also can't repro the problem using 55.0.2875.0 canary on Windows, then let me mark the bug as fixed.

Since this bug is a ReleaseBlock-Stable, we need to merge the fix (r421645) to M54.  Despite touching ~310 lines in ~20 files, the fix makes relatively small and safe changes to the navigation code:

- Goes back to the M53 behavior of avoiding ShouldFork / OpenURL for POST navigations (the change in chrome/renderer/chrome_content_renderer_client.cc)

- Adds extra blocking of top-level navigations via ShouldTransferNavigation - the only new code is really in chrome/browser/extensions/extension_view_host.cc (other changes just add an extra |is_main_frame_navigation| parameter to the ShouldTransferNavigation method).

- Most of the added lines (~275) come from new tests in chrome/browser/extensions/api/extension_action/browser_action_apitest.cc, chrome/browser/extensions/process_management_browsertest.cc and chrome/test/data/extensions/api_test/browser_action/popup_with_form/

So, given above, I think we'll just need to monitor the canary (the fix has first landed in 55.0.2875.0) for a day or two and then it should be safe to merge the fix to M54.

Your change meets the bar and is auto-approved for M54 (branch: 2840)

Please merged your change to M54 (Branch:2840) ASAP, so that we could take it for next weeks Beta release.

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

commit 1fd4604fd78908736589d8ac5e3b0e3347c4c6ce
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Sep 29 22:11:53 2016

Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).
(this is relanding a slightly tweaked r421287).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
    https://crbug.com/646261 

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
    https://crbug.com/648648 ).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term (tracked in https://crbug.com/650694) we want to
   make all extension navigations to not go through OpenURL path, but
   this seems too risky to merge back to M54, so for now we only do #2
   above (i.e. avoid OpenURL only for POST requests).

BUG= 646261 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=sky@chromium.org, rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2377833002
Cr-Commit-Position: refs/heads/master@{#421645}
(cherry picked from commit 1d02573dbb13b5d7bd76f172afdf19a6389dc2f7)

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

Cr-Commit-Position: refs/branch-heads/2840@{#589}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/extension_view_host.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/extension_view_host.h
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/renderer/chrome_content_renderer_client.cc
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.js
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/cross_site_transfer_browsertest.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigator_delegate.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigator_delegate.h
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/public/browser/web_contents_delegate.h

Hmmm... I've just checked that one of the new tests (POST navigating an extension popup to a webpage) fails outside of --isolate-extensions mode (as is the case in default M54).  Based on our previous discussions I believe that this is expected and okay from security perspective.  OTOH, this might mean a need to disable this portion of the test on M54 branch and/or bots.

So far there were some build problems preventing official tests from running on M54 builds that includes the fix above (tracked in issue ).  Let's keep an eye on browser_tests results - in particular, the following failure is probably expected:

[ RUN      ] NavigatingExtensionPopupBrowserTest.Webpage
[75969:75969:0929/172259:WARNING:password_store_factory.cc(248)] Using basic (unencrypted) store for password storage. See https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_storage.md for more information about password storage options.
[75969:76061:0929/172301:WARNING:simple_synchronous_entry.cc(1054)] Could not open platform files for entry.
../../chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:897: Failure
Expected: (target_url) != (popup->GetLastCommittedURL()), actual: http://foo.com:53447/title1.html vs http://foo.com:53447/title1.html
Navigation to http://foo.com:53447/title1.html should fail in an extension pop-up
../../chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:904: Failure
Value of: popup->GetLastCommittedURL()
Expected: (is equal to chrome-extension://gmileokcjmpdlhioblkpagdlifpibmbb/popup.html) or ((is equal to chrome-extension://invalid/) or (is equal to about:blank))
  Actual: http://foo.com:53447/title1.html

The browser_test issues pointed out in #c47 above is now tracked in a separate bug - issue 651922.

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

commit d92aa0c649814fadf034a227477191605a45c91c
Author: lukasza <lukasza@chromium.org>
Date: Mon Oct 03 14:54:37 2016

Remove unnecessary whitespace from test files.

This is a follow-up that fixes up whitespace (trailing newlines) in test
files that were added in r421645.

BUG= 646261 

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

[modify] https://crrev.com/d92aa0c649814fadf034a227477191605a45c91c/chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html
[modify] https://crrev.com/d92aa0c649814fadf034a227477191605a45c91c/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html

 Issue 651673  has been merged into this issue.

Verified the merge on the latest M-54(54.0.2840.50) on Windows-10, Mac OS 10.11.6 and Linux Ubuntu 14.04. This is working as intended.

Deleted: 646261.png 212 KB

	646261.png 212 KB View Download

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

commit 1fd4604fd78908736589d8ac5e3b0e3347c4c6ce
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Sep 29 22:11:53 2016

Block navigations in extensions via ShouldTransferNavigation (not via OpenURL).
(this is relanding a slightly tweaked r421287).

Before this CL, blocking of top-level navigations in extension pop-ups
was accomplished by routing them via OpenURL path and blocking (or
rather - silently dropping) CURRENT_TAB navigations via
ExtensionViewHost::OpenURLFromTab.  This was problematic for a few
reasons:

1. This unnecessarily blocked navigations that end-up being treated
   as downloads (i.e. because HTTP response says Content-Disposition:
   attachment).  This was the root cause of the regression raised in
    https://crbug.com/646261 

2. There are still some remaining issues in handling of POST requests
   via OpenURL path (e.g. dropping Content-Type header -
    https://crbug.com/648648 ).

3. In the long-term we want to rely less on process isolation
   accomplished via OpenURL - an exploited renderer process does not
   necessarily have to go through OpenURL path and can instead choose
   to use the regular, renderer-initiated path.

After this CL:

1. ExtensionViewHost::ShouldTransferNavigation is used to block top-level
   navigations in extension pop-ups (and background pages).

2. POST navigations do not go through OpenURL path anymore (i.e. this CL
   effectively reverts the essence of r407586).

   In the long-term (tracked in https://crbug.com/650694) we want to
   make all extension navigations to not go through OpenURL path, but
   this seems too risky to merge back to M54, so for now we only do #2
   above (i.e. avoid OpenURL only for POST requests).

BUG= 646261 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=sky@chromium.org, rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2377833002
Cr-Commit-Position: refs/heads/master@{#421645}
(cherry picked from commit 1d02573dbb13b5d7bd76f172afdf19a6389dc2f7)

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

Cr-Commit-Position: refs/branch-heads/2840@{#589}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/extension_view_host.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/extension_view_host.h
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/renderer/chrome_content_renderer_client.cc
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/manifest.json
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/other_page.html
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.html
[add] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/chrome/test/data/extensions/api_test/browser_action/popup_with_form/popup.js
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/cross_site_transfer_browsertest.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigator_delegate.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigator_delegate.h
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/1fd4604fd78908736589d8ac5e3b0e3347c4c6ce/content/public/browser/web_contents_delegate.h