Pushing back to P3 (this is less urgent than the other P2s).

67 has branched, moving bugs over to 68.

alexmos: I'm having trouble using the new NotifyPreferencesChanged() function. More specifically, when called, the preferences are not updated for OOPIFs.

When NotifyPreferencesChanged() iterates over the FrameTreeNodes, it retrieves the RenderViewHost from the RenderWidgetHost. But in the case of OOPIFs this returns nullptr and the iframe gets ignored. Retrieving the RenderViewHost from the RenderFrameHost works though.

So what's the difference between the RenderViewHost returned from the RenderFrameHost and the one returned from the RenderWidgetHost? And would it be possible to always retrieve the RVH from the RFH?


Repro instructions:

1. Patch in: https://chromium-review.googlesource.com/c/chromium/src/+/999239
2. Run chrome with --enable-features=DesktopPWAWindowing --site-per-process
3. Visit https://elite-flock.glitch.me/
4. Open the three dot menu and click Install PWA Sandbox
5. In the new app window, click navigate iframe

What is the expected result?
The mixed content inside the iframe should be blocked

What happens instead?
The mixed content inside the iframe is not blocked.

In the logs we can see the following:

[web_contents_impl.cc(1681)] frame url: https://elite-flock.glitch.me/index.html
[web_contents_impl.cc(1683)] Has RVH from Widget
[web_contents_impl.cc(1688)] Has RVH from Frame
[web_contents_impl.cc(1681)] frame url: https://badssl.com/
[web_contents_impl.cc(1685)] Missing RVH from Widget
[web_contents_impl.cc(1688)] Has RVH from Frame

Because the frame with the mixed content is missing the RVH from the Widget, we ignore it when updating the preferences.

Oh, nice find!  That's a bug in WebContentsImpl::NotifyPreferencesChanged(); looks like we've had it for a while, since r386313.

Quick explanation: each FrameTreeNode in the FrameTree (i.e., page) might have a frame living in a different renderer process.  Each process needs a RenderViewHost to track page-level state, such as the webkit prefs.  This RVH is what you get by referencing ftn->current_frame_host()->GetRenderViewHost().  If there are multiple same-site frames in the same frame tree, they all share the same RenderViewHost, hence the std::set deduplication.  So for a page structure like A(B(A)), we would have two RenderViewHosts for A and B, and the top and bottom frame's RVH is the same.  For A(B(C),B(A)), we would have three RenderViewHosts.  These RVHs are stored in FrameTree::render_view_host_map_.

Each frame also needs a RenderWidgetHost to process input events and rendering for that frame.  Top-level frames have RenderWidgetHosts which are also associated with that frame's RenderViewHost, and hence for them RenderViewHost::From(ftn->current_frame_host()->GetRenderWidgetHost()) will match what rfh->current_frame_host()->GetRenderViewHost() returns.  However, for subframes that have a parent in a different process, we create a new RenderWidgetHost, which isn't associated with a RenderViewHost.  This is why for OOPIFs, RenderViewHost::From(ftn->current_frame_host()->GetRenderWidgetHost()) is null.  In a page like A(B,B,B), each subframe will have its own separate RenderWidgetHost, but all subframes will share the same RenderViewHost (for B).  The code as it's currently written will only work for top frame's RVH, though I think its intent is pretty clearly to iterate over all RVHs in the frame tree.  Sorry that I missed that when I filed issue 828697.

As a quick fix, I agree we can collect RenderViewHosts from each FrameTreeNode's RenderFrameHost and not look at RenderWidgetHosts at all.  I think something like this ought to work:

  std::set<RenderViewHost*> render_view_host_set;
  for (FrameTreeNode* node : frame_tree_.Nodes())
    render_view_host_set.insert(node->current_frame_host()->GetRenderViewHost());
  for (RenderViewHost* render_view_host : render_view_host_set)
    render_view_host->OnWebkitPreferencesChanged();

Note also that RenderViewHost is deprecated, and I think the right solution that we'll want eventually is to migrate WCI::NotifyPreferencesChanged() to use WebContentsImpl::SendPageMessage() and switch ViewMsg_UpdateWebPreferences to a PageMsg.  That also takes care of a few corner cases, like a pending cross-process navigation that hasn't committed yet.

Feel free to fix this, or I can take care of it next week (I might be delayed due to BlinkOn).

Friendly ping.  ortuno@ or alexmos@, will you have a chance to try out the suggestion in comment 6?

Also, will this fix help for  issue 829235  as well?  They sound pretty similar, but I'm not sure.

Note that Site Isolation is aiming to launch in M67.  Hopefully this is not severe enough to be a large concern?  If necessary, we can try to get the fix landed and merged.

Sorry for not updating the bug! This got delayed because of BlinkOn, but I'm actively working on fixing it (currently writing a test for the fix in #6).

There is no need to merge. In case mixed content is not blocked, we will show the necessary security indicators to tell the user that the content is not fully secure.

 Issue 829235  has been merged into this issue.

For #8, I was referring to merging the fix for Desktop PWAs. I not know enough to know if we should merge the fix for NotifyPreferencesChanged.

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

commit d231ca32d49e409df44f003f4e414bd592771beb
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Mon May 14 04:20:53 2018

desktop-pwas: Apply strict mixed content to OOPIFs

Before, HostedAppBrowserController would set/unset the strict mixed
content preference on the main RenderViewHost when a WebContents was
attached/detached from a PWA Browser window. This worked when
Site Isolation was not enabled because all frames in the WebContents
shared the same RenderViewHost. Now that Site Isolation is enabled,
cross-origin iframes no longer share the same RenderViewHost resulting
in the strict mixed content preference to not be applied to them.

With Site Isolation enabled we need to handle two cases:

(1) Apply the new preference on existing iframes
(2) Apply the new preference on new iframes

To handle case (1), this patch switches from UpdateWebKitPreferences
to NotifyPreferencesChanged which eventually triggers a preference
update on all RenderViewHosts in the WebContents.
NotifyPreferencesChanged doesn't take any arguments, it just
recomputes the preferences, so this patch modifies
ChromeContentBrowserClient to check if the WebContents is in a PWA
Browser window and set the preference accordingly.

The above changes also handle case (2). When a new iframe
is created, OverrideWebkitPrefs is called for its RenderViewHost
and if the WebContents is in a PWA Browser window then the
preference is set.

Bug:  829688 
Change-Id: Idb11bf979ec76ae8e1dfc905bf68e3c7ad1dbd6f
Reviewed-on: https://chromium-review.googlesource.com/1048885
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558189}
[modify] https://crrev.com/d231ca32d49e409df44f003f4e414bd592771beb/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/d231ca32d49e409df44f003f4e414bd592771beb/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
[modify] https://crrev.com/d231ca32d49e409df44f003f4e414bd592771beb/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[add] https://crrev.com/d231ca32d49e409df44f003f4e414bd592771beb/chrome/test/data/ssl/page_displays_insecure_content_in_iframe.html
[add] https://crrev.com/d231ca32d49e409df44f003f4e414bd592771beb/chrome/test/data/ssl/page_with_cross_site_frame.html