Hmm, I must have done something wrong while editing the bug report - it seems I forgot the attachment. Here's a test case for the bug.

Deleted: chrome_webview_reload_bug.zip 4.6 KB

chrome_webview_reload_bug.zip 4.6 KB Download

429531 (good) - 429533 (bad)
https://chromium.googlesource.com/chromium/src/+log/66d5ae12..78b7de04?pretty=fuller
Suspecting r429532 "Fix web accessible resource checks in ShouldAllowOpenURL"
	Landed in 56.0.2909.0
	Merged to 55.0.2883.52

Assigning to the Cl owner from the above bisect.Tagging with M58 for a fix.

Thanks for reporting this.  I can confirm that my r429532 is the cause, and I've started looking into this.  In the example from #1, the <webview> src is pointing at the app's resource, otherPage.html.  That initial navigation doesn't go through ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL and is allowed, but the reload() follows the OpenURL path and does go through ShouldAllowOpenURL, which blocks it because otherPage.html is from a chrome-extension:// origin and is not web-accessible.

Since the <webview> here is associated with a partition which has accessible_resources that allows otherPage.html, I think this load ought to be allowed.  Normally, that check is performed in AllowCrossRendererResourceLoadHelper (when creating a URLRequestJob for the extension resource), but ShouldAllowOpenURL blocks the load before we ever get there.  One option for a fix is just to skip webview guests in ShouldAllowOpenURL and rely on AllowCrossRendererResourceLoadHelper for enforcement.

However, it appears that AllowCrossRendererResourceLoadHelper itself is problematic and allows any app resources to load, not just those that are declared in accessible_resources.   I'm seeing this in the following setup:
- disable my ShouldAllowOpenURL check for <webview> guests
- in the app from #1, change accessible_resources for the webview partition from "*" to "otherPage.html"
- add another page, "bar.html", to the app, which isn't listed in accessible_resources.
- from the webview process, location="chrome-extension://mompbbocaiankjffcmbpajccebphhhjl/bar.html"

bar.html loads just fine, which seems wrong.  (The policy seems like it should allow otherPage.html but not bar.html.) 

For this load, AllowCrossRendererResourceLoadHelper sees that bar.html is not webview-accessible, but it doesn't set *allowed to false because of PageTransitionIsWebTriggerable here:

  if (is_guest && !ui::PageTransitionIsWebTriggerable(page_transition)) {
    *allowed = false;
    return true;
  }

  return false;  // <-- ends up returning here, i.e., no decision made

After this returns false, we go back to the caller AllowCrossRendererResourceLoad, which sets *allowed to true here:

  // Navigating the main frame to an extension URL is allowed, even if not
  // explicitly listed as web_accessible_resource.
  if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
    *allowed = true;
    return true;
  } ...

The PageTransitionIsWebTriggerable blocking the accessible_resources check seems suspicious to me.  I've found some discussion about problems with that check on  https://crbug.com/633963#c14  by rob@robwu.nl and nasko@.

Lucas or Devlin: can you confirm my understanding that bar.html should be blocked in the above example, since it's not declared in accessible_resources for the <webview> partition?

So in summary, before my r429532 fix for ShouldAllowOpenURL, it seems like we were allowing all app resources to load in <webview>, whether or not they were webview-accessible, for web-triggerable transitions.  After my r429532, we stopped allowing all of them on the OpenURL path only.  Now we'll need to go somewhere in between.

Re #5: Yes, bar.html should be disallowed in this example. Only extension resources explicitly allowed in the manifest file for the current webview partition should be allowed to load inside a webview.

While working on a test for this, I realized that renderer-initiated navigations in a webview only go through OpenURL for navigations to the *same* URL - see ChromeExtensionsRendererClient::ShouldFork:

  // If this is a reload, check whether it has the wrong process type.  We
  // should send it to the browser if it's an extension URL (e.g., hosted app)
  // in a normal process, or if it's a process for an extension that has been
  // uninstalled.  Without --site-per-process mode, we never fork processes for
  // subframes, so this check only makes sense for top-level frames.
  // TODO(alexmos,nasko): Figure out how this check should work when reloading
  // subframes in --site-per-process mode.
  if (!frame->parent() && GURL(frame->document().url()) == url) {
    if (is_extension_url != IsStandaloneExtensionProcess())
      return true;
  }

The reload condition itself seems very weird to me, but nonetheless it means that this bug will not occur as often as I believed, as ShouldAllowOpenURL will only interfere in cases like the webview reloading the original URL or navigating to same URL with different query params (basically what the repro from #1 has).  Navigations to different webview-accessible resources should not be affected and will still go through AllowCrossRendererResourceLoadHelper, though the problems I discovered there in #5 should still be fixed.

The follow-up to  bug 633963  that was mentioned in comment 5 is  bug 640072 . Do you think that applying my suggestion from  bug 640072  would fix this issue too?

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

commit f89e04046aaac44ecad27056fb856c3d6e37370f
Author: alexmos <alexmos@chromium.org>
Date: Thu Mar 23 00:46:24 2017

Allow webview guests to skip WAR checks in ShouldAllowOpenURL.

Prior to r429532, the web-accessible resource checks in
ShouldAllowOpenURL were skipped for all schemes other than
HTTP/HTTPS/chrome-extension.  After r429532, we've started enforcing
them for most other schemes, but this breaks navigations from webview
guests to webview-accessible app resources whenever those go through
the OpenURL path, since the webview-accessible resource check is done
separately later, in AllowCrossRendererResourceLoadHelper, and
ShouldAllowOpenURL was killing the navigation before it ever got
there.

This CL will return webview guests to the previous behavior.

BUG= 691941 

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

[modify] https://crrev.com/f89e04046aaac44ecad27056fb856c3d6e37370f/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/f89e04046aaac44ecad27056fb856c3d6e37370f/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[add] https://crrev.com/f89e04046aaac44ecad27056fb856c3d6e37370f/chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/assets/foo.html
[modify] https://crrev.com/f89e04046aaac44ecad27056fb856c3d6e37370f/chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js

#8: Yes, that's exactly the fix I discussed with lfg@, and it would fix this issue too.  I'll go ahead and put up a CL for it.

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

commit a7d08ae5f1e081b95ae6f4e1f163cbd0be6dc075
Author: alexmos <alexmos@chromium.org>
Date: Fri Mar 24 01:23:01 2017

Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

AllowCrossRendererResourceLoadHelper is supposed to check whether a
resource that a webview guest tries to load is webview-accessible [1],
and denies the load if it isn't.  However, it currently has an
exception for all web-triggerable page transitions, blindly allowing
them through.  This allows a webview to happily navigate itself to
non-webview-accessible resources.

The page transition exception seems wrong, and rob@robwu.nl made a
detailed analysis of how we ended up with it in
 https://crbug.com/633963#c14 .  This CL removes it.  The fix was
originally proposed by Rob in  https://crbug.com/640072 .

[1] https://developer.chrome.com/apps/tags/webview#local_resources

BUG= 640072 , 691941 

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

[modify] https://crrev.com/a7d08ae5f1e081b95ae6f4e1f163cbd0be6dc075/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/a7d08ae5f1e081b95ae6f4e1f163cbd0be6dc075/chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js
[modify] https://crrev.com/a7d08ae5f1e081b95ae6f4e1f163cbd0be6dc075/extensions/browser/url_request_util.cc

I've verified this fix in Mac Canary 59.0.3050.0, which has the main fix from r458959, and 59.0.3053.0, which also has the additional fix for webview-accessible resource checks from r459326.  Requesting to merge r458959 to M58.  Not sure about merging r459326 as well, it's an easy fix but also more dangerous, so might benefit from more bake time.

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit 32ec47289bec2eb6327723f491182e9f1c9a6103
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Tue Mar 28 00:33:20 2017

Allow webview guests to skip WAR checks in ShouldAllowOpenURL (Merge to M58)

Prior to r429532, the web-accessible resource checks in
ShouldAllowOpenURL were skipped for all schemes other than
HTTP/HTTPS/chrome-extension.  After r429532, we've started enforcing
them for most other schemes, but this breaks navigations from webview
guests to webview-accessible app resources whenever those go through
the OpenURL path, since the webview-accessible resource check is done
separately later, in AllowCrossRendererResourceLoadHelper, and
ShouldAllowOpenURL was killing the navigation before it ever got
there.

This CL will return webview guests to the previous behavior.

BUG= 691941 

Review-Url: https://codereview.chromium.org/2766313002
Cr-Commit-Position: refs/heads/master@{#458959}
(cherry picked from commit f89e04046aaac44ecad27056fb856c3d6e37370f)

Review-Url: https://codereview.chromium.org/2781783002 .
Cr-Commit-Position: refs/branch-heads/3029@{#447}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/32ec47289bec2eb6327723f491182e9f1c9a6103/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/32ec47289bec2eb6327723f491182e9f1c9a6103/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[add] https://crrev.com/32ec47289bec2eb6327723f491182e9f1c9a6103/chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/assets/foo.html
[modify] https://crrev.com/32ec47289bec2eb6327723f491182e9f1c9a6103/chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js

Tested on windows 7 , mac os 10.12.3 and ubuntu 14.04 using chrome M58 #58.0.3029.41 and followed below steps :

1. Downloaded and launched the extensions provided in comment #1.
2. Clicked on all the links inside the web view and didnt see any blank page or about : blank page.

Attached screencast for reference.

@alexmos-- Could you please check attached screencast and confirm us if this is the expected result or if we had missed any steps in verifyng the issue.

Thanks!

Deleted: 691941.mp4 476 KB

	691941.mp4 476 KB View Download