I agree.  It's odd right now that window.name lookups work for any frame in the renderer process, even when we start sharing the process randomly when Chrome goes over the process limit.

There's currently no representation of BrowsingInstance in the renderer process, and the class itself was intentionally hidden as a bit of an implementation detail within SiteInstance.  (We generally talk about whether two SiteInstances are "related" to say whether they belong to the same BrowsingInstance.)

That said, we should have some kind of namespace corresponding to it in the renderer process, whether we introduce some kind of BrowsingInstance identifier or manage it another way.

+1 to plumbing this through to blink. Another use case that's blocked on it: https://codereview.chromium.org/2860263002

I am trying to tackle this by

1. Adding a |browsing_instance_id_| field to blink::Page:
   - The new field is used to implement a new instance method: const PageSet& blink::Page::RelatedPages()
   - The new field is populated via a new, mandatory argument of Page::CreateOrdinary
     This argument is plumbed from a new, optional argument of WebView::Create
     (optional so that print preview and tests default to being in a browsing instance
      not related to any other pages).

2. Adding a |browsing_instance_id_| field to content::RenderViewImpl:
   - The new field is used to propagate opener's browsing_instance_id_
     when creating a new view in RenderViewImpl's implementation of
     WebViewClient::CreateView.

3. Adding an id to content::BrowsingInstance and exposing it via
   content::SiteInstanceImpl::GetBrowsingInstanceId.

The above would:
- leak the concept of a browsing instance out of content::SiteInstanceImpl,
  (but still keeping it out of //content's public API)
- expose the concept of a browsing instance to blink
  - naked browsing instance id would only be present as
    an argument of WebView::Create and Page::CreateOrdinary
  - otherwise browsing instance ids are kind encapsulated within Page
    (i.e. hidden behind via Page::RelatedPages())

I would very much like to use IdType32<T> rather than adding yet another int-based id value, but this is difficult because 1) blink cannot depend on IdType32 (currently in //gpu/command_buffer/common/id_type.h) and 2) there is no common T type that would be visible across //content (i.e. content::BrowsingInstance), blink/web (i.e. blink::WebView) and blink/core (i.e. blink::Page).

I'm personally a bit wary of plumbing integer IDs through.

The key observation here is that browsing contexts can only become related by calls to window.open() and targeted link navigations. These (should) both go through RenderViewImpl::CreateView().

Similarly, when replicating this for OOPIF, we'll only get new related views when creating a new view. My overall feeling is that we should have a Blink API to create a related WebView, and have the WebViews joined in a circularly-linked list.

This does have the issue of having to ensure that the browser and renderer are in-sync with what belongs in the same unit of related browsing contexts, but this strikes me as pretty similar to other issues with navigation, etc where the browser-renderer have to agree on state or bad things will happen.

Thanks for writing up #c5 - this does indeed seem nicer than what we've discussed earlier and what I've put up in #c4.

One thing to note is that there are things that are effectively sharing window namespace *without* going through window.open and/or content::BrowsingInstance.  One known example if chrome.windows.create API (the hangouts extension depends on the window namespace behavior) - the test that I am adding in https://codereview.chromium.org/2873693003 would be broken by either #c4 (verified breakage) or #c5 (untested).  I hope that chrome.windows.create can create a proper opener relationship (this would also help with  issue 713888 ).

I still need to look at tryjob results (quite a few failures in WIP for #c4 @ https://codereview.chromium.org/2873503002) to see if I've made a mistake in my CL or if there are other things that would be broken by restricting window namespace to windows having opener relationship.

There is another thing that would be broken by restricting window lookup to the current browsing instance - lookup of background contents of hosted apps.  In particular, the AppBackgroundPageApiTest.Basic in browser_tests would be broken.  This particular test performs the following steps:

1. basic/test.js: window.onload: chrome.tabs.create({ 'url': a_url }, ...
2. common/a.html: window.onload: backgroundWindow = window.open('bg.html', 'bg', 'background');
3. basic/test.js: onBackgroundPageLoaded: chrome.tabs.remove(pageA.id, ...
4. basic/test.js: onBackgroundPageLoaded: chrome.tabs.create({ url: b_url },
5. common/b.html: window.onload: backgroundWindow = window.open('bg.html', 'bg');

Since a.html and b.html are created in steps 1 and 4 via chrome.tabs.create, they end up in separate browsing instances (see also  issue 713888 ).  Since b.html is in a separate browsing instance, it won't be able to find the background page created/opened by a.html (at least after WIP CL @ https://codereview.chromium.org/2873503002).

WIP CL @ https://codereview.chromium.org/2873503002 is based on taking a "browsing instance id" maintained by the browser-side in content::BrowsingInstance object and propagating it to the renderer-side and associating blink::Page objects with a particular browsing instance (i.e. the WIP CL is currently based on #c4 above).  An alternative, cleaner approach from #c5 would just require the browser to tell the renderer that the newly created WebView should be in the same browsing instance as the other, related WebView.  The alternative approach is doable when the browser has an opener (which can act as "the other, related WebView"), but there might be scenarios where the browser only has a SiteInstance (rather then a related RenderFrameHost or RenderViewHost) - one example from nasko@ is a hangouts app which creates two webview tags in the same storage partition (and so - in the same SiteInstance) so they can find each other and communicate.

Btw, I'm not sure we need to tie this to the opener necessarily: the reply to the renderer-initiated IPC to create a new view (and the IPC from browser to renderer to create a new view) could simply pass along the routing ID of the webview that a newly created webview should be associated with.

Status update for https://codereview.chromium.org/2873503002:
- Got rid of browsing_instance_id - instead having blink::Page maintain a circular, double-linked list of pages it is related to
- Using (newly added) extensions::ExtensionFrameHelper::FindFrame as a fallback if blink cannot find a frame within the browsing instance

The next hurdle is limiting the applicability of extensions::ExtensionFrameHelper::FindFrame, so that at it doesn't pierce browsing instances for web pages, while still allowing the piercing for hosted apps.  I am currently investigating whether the approach below makes sense.  Open questions:
- Is frame->Opener() always a *local* frame if frame->GetSecurityOrigin().IsUnique()
- RenderFrameMatches and ExtensionWebContentsObserver::GetExtensionFromFrame don't match hosted apps today.  Should they?
- Should other callers of extensions::RendererExtensionRegistry::GetExtensionOrAppByURL (i.e. CrossesExtensionExtents) call into the shared code below?
- Need to check if we have test coverage for: look up frame by name, outside of browsing instance, from an extension frame at initial url or at about:blank (i.e. it looking at openers required).

// Gets an extension (if any) associated with the local |frame|.
const Extension* GetExtensionOrAppFromFrame(blink::WebLocalFrame* frame) {
  // TODO(lukasza): Try to share the code below with 1) other callers of
  // extensions::RendererExtensionRegistry::GetExtensionOrAppByURL and 2)
  // RenderFrameMatches below and from
  // ExtensionWebContentsObserver::GetExtensionFromFrame.

  // We cannot look at the frame's origin - this won't work for hosted apps,
  // which can declare web extents at path granularity.  To work around this, we
  // look at the opener's URL.  This is similar to what CrossesExtensionExtents
  // does.
  while (frame->GetSecurityOrigin().IsUnique() && frame->Opener()) {
    DCHECK(frame->Opener()->IsWebLocalFrame());
    frame = frame->Opener()->ToWebLocalFrame();
  }

  return extensions::RendererExtensionRegistry::Get()->GetExtensionOrAppByURL(
      frame->GetDocument().Url());
}

...

content::RenderFrame* ExtensionFrameHelper::FindFrame(
    content::RenderFrame* relative_to_frame,
    const std::string& name) {
  // TODO(lukasza):  https://crbug.com/718489 : ExtensionFrameHelper::FindFrame
  // method violates browsing instance boundaries - we should investigate how to
  // restrict its applicability (hopefully to background contents only).

  // Only extensions should pierce browsing instance boundaries.
  if (!GetExtensionOrAppFromFrame(relative_to_frame->GetWebFrame()))
    return nullptr;

  for (const ExtensionFrameHelper* helper : g_frame_helpers.Get()) {
    if (helper->render_frame()->GetWebFrame()->AssignedName().Utf8() == name)
      return helper->render_frame();
  }

  return nullptr;
}

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

commit 2fbbe131dc69bab2acc38c7289999cdd5e5716c8
Author: lukasza <lukasza@chromium.org>
Date: Wed May 31 21:01:11 2017

FrameTree::Find only searches relative to local frames.

This CL reinforces that FrameTree::Find should only ever need to search
relative to *local* frames.  To accomplish this, this CL:

- Adds a DCHECK(this_frame_->IsLocalFrame()) to FrameTree::Find method.

- Moves FindFrameForNavigation method from Frame to LocalFrame class.

- Moves FindFrameByName method from WebView to WebLocalFrame
  and uses |this| rather than an optional |relative_to_frame| parameter.

All the existing callers of FrameTree::Find are already doing the lookup
relative to *local* frames, except test code.  This CL preserves the old
behavior in the test code, by 1) doing the lookup relative to the main
frame and 2) failing safely if the main frame is remote or if the result
is not local (both 1 and 2 were the old behaviors of
WebView::FindFrameByName).

The move of the FindFrameByName method (including tweaks of the doc
comment) also highlights that the lookup (as implemented via
FrameTree::Find) should only search within a browsing instance, not just
within the frame tree of a given WebView.  OTOH, this comes with some
disclaimers: 1) until  https://crbug.com/718489  is fixed the lookup is
process-wide, and 2) some extension features require ignoring the
browsing instance boundaries.

BUG= 718489 
TBR=gene@chromium.org (got lgtm via gene1...)

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

[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/components/printing/test/print_web_view_helper_browsertest.cc
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/content/renderer/pepper/pepper_plugin_instance_impl.cc
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/content/shell/test_runner/test_runner_for_specific_view.cc
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/core/frame/Frame.h
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/web/WebLocalFrameImpl.h
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/public/web/WebLocalFrame.h
[modify] https://crrev.com/2fbbe131dc69bab2acc38c7289999cdd5e5716c8/third_party/WebKit/public/web/WebView.h

I have a fix that I think is almost ready for review at: https://codereview.chromium.org/2873503002.  The only think that makes me hesitant to send the CR request to dcheng@ is the dependency on https://codereview.chromium.org/2921753002 (for  issue 713888 ).  Hopefully there is no controversy with the other CL/bug :-).

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

commit 1c950906dd11eb51e4995e079f87fdbba9eb72cb
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Dec 06 00:38:17 2017

Improve granularity of window namespaces in Blink.

This CL ensures that blink::FrameTree::Find(const AtomicString& name)
only looks for a match in the current set of related browsing contexts
(represented in the browser by content::BrowsingInstance).  This CL
means that window.open's behavior won't change just because a renderer
process happens to host multiple unrelated browsing contexts (possible
for example after reusing a renderer process because of hitting the
process limit).

This CL consists of 3 parts:

- New browser tests.
  - RenderFrameHostManagerTest.ProcessReuseVsBrowsingInstance
    for verifying browsing instance boundaries when the
    renderer processes get reused.
  - ExtensionFunctionalTest.FindingUnrelatedExtensionFramesFromAboutBlank
    for verifying that extensions can still lookup unrelated frames
    from the same extension.

- Having blink::Page maintain a set of related pages.
  - Page::next_related_page_ and Page::prev_related_page_ form a
    circular, double-linked list of related pages.
  - Page::CreateOrdinary takes a new |opener| parameter and treats the
    new page and the |opener| as related and puts them on the same list.
  - |opener| is propagated from content::RenderViewImpl, through
    blink::WebViewImpl into blink::Page.

- Falling back to blink embedder when blink::FrameTree::Find finds no frame
  with the given name.
  - The fallback is needed to preserve the old behavior for extensions.
  - The fallback goes through blink::LocalFrameClient,
    blink::WebFrameClient / content::RenderFrameImpl,
    content::ContentRendererClient / ::ChromeContentRendererClient,
    ::ChromeExtensionsRendererClient and finally is implemented
    by extensions::ExtensionFrameHelper.
  - Currently the fallback iterates through all same-origin frames in
    the given process, but requires that the |relative_to_frame| is an
    extension frame.  In the future we might want to restrict piercing
    of browsing instances to specific scenarios where it is needed
    (e.g. restrict it to background pages / contents only?).

I've tested this CL via:
(all tests below pass before and after the CL, except for
ProcessReuseVsBrowsingInstance which is fixed by this CL)

- New tests:
    - RenderFrameHostManagerTest.ProcessReuseVsBrowsingInstance
      (web -> web shouldn't violate browsing instance)
    - ExtensionFunctionalTest.FindingUnrelatedExtensionFramesFromAboutBlank
      (extension/about:blank -> extension can violate browsing instance)

- Existing tests:
    - ExtensionApiTest.WindowsCreate_WithOpener and _NoOpener
      (chrome.windows.create stays in the same browsing instance depending
       on the setSelfAsOpener parameter)
    - AppBackgroundPageApiTest.Basic
      (hosted app -> background page can violate browsing instance;
       tests handling of mapping of web urls [full url, not just origin]
       to extensions)

- Manual testing:
  - Hangouts Chrome *extension* continues to work (sufficient to
    validate that sign-in works).  Tested with version 2017.1019.418.1.

Bug:  718489 
Test: See above.
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Tbr: tommycli@chromium.org
Change-Id: Icdc9ec7bef0e35b59e04fb12385045f22db80c3a
Reviewed-on: https://chromium-review.googlesource.com/764487
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521917}
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/chrome/browser/extensions/api/tabs/tabs_test.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/chrome/browser/extensions/extension_functional_browsertest.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/chrome/renderer/extensions/chrome_extensions_renderer_client.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/chrome/renderer/extensions/chrome_extensions_renderer_client.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/components/plugins/renderer/webview_plugin.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/components/printing/renderer/print_render_frame_helper.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/browser/browsing_instance.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/browser/renderer_host/render_process_host_browsertest.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/renderer/render_frame_impl.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/content/renderer/render_view_impl.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/extensions/renderer/scoped_web_frame.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/extensions/renderer/script_context_set.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/extensions/renderer/script_context_set.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/exported/WebViewImpl.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/exported/WebViewTest.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/exported/WorkerShadowPage.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/loader/EmptyClients.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/1c950906dd11eb51e4995e079f87fdbba9eb72cb/third_party/WebKit/public/web/WebView.h

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

commit e4c10ef6c4009f1df94fe36e0585ce59414e0b5a
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Dec 12 23:51:44 2017

Revert "Improve granularity of window namespaces in Blink."

This reverts commit 1c950906dd11eb51e4995e079f87fdbba9eb72cb.

Reason for revert: Broke Hangouts Chrome App - see  https://crbug.com/794079 

Original change's description:
> Improve granularity of window namespaces in Blink.
>
> This CL ensures that blink::FrameTree::Find(const AtomicString& name)
> only looks for a match in the current set of related browsing contexts
> (represented in the browser by content::BrowsingInstance).  This CL
> means that window.open's behavior won't change just because a renderer
> process happens to host multiple unrelated browsing contexts (possible
> for example after reusing a renderer process because of hitting the
> process limit).
>
> This CL consists of 3 parts:
>
> - New browser tests.
>   - RenderFrameHostManagerTest.ProcessReuseVsBrowsingInstance
>     for verifying browsing instance boundaries when the
>     renderer processes get reused.
>   - ExtensionFunctionalTest.FindingUnrelatedExtensionFramesFromAboutBlank
>     for verifying that extensions can still lookup unrelated frames
>     from the same extension.
>
> - Having blink::Page maintain a set of related pages.
>   - Page::next_related_page_ and Page::prev_related_page_ form a
>     circular, double-linked list of related pages.
>   - Page::CreateOrdinary takes a new |opener| parameter and treats the
>     new page and the |opener| as related and puts them on the same list.
>   - |opener| is propagated from content::RenderViewImpl, through
>     blink::WebViewImpl into blink::Page.
>
> - Falling back to blink embedder when blink::FrameTree::Find finds no frame
>   with the given name.
>   - The fallback is needed to preserve the old behavior for extensions.
>   - The fallback goes through blink::LocalFrameClient,
>     blink::WebFrameClient / content::RenderFrameImpl,
>     content::ContentRendererClient / ::ChromeContentRendererClient,
>     ::ChromeExtensionsRendererClient and finally is implemented
>     by extensions::ExtensionFrameHelper.
>   - Currently the fallback iterates through all same-origin frames in
>     the given process, but requires that the |relative_to_frame| is an
>     extension frame.  In the future we might want to restrict piercing
>     of browsing instances to specific scenarios where it is needed
>     (e.g. restrict it to background pages / contents only?).
>
> I've tested this CL via:
> (all tests below pass before and after the CL, except for
> ProcessReuseVsBrowsingInstance which is fixed by this CL)
>
> - New tests:
>     - RenderFrameHostManagerTest.ProcessReuseVsBrowsingInstance
>       (web -> web shouldn't violate browsing instance)
>     - ExtensionFunctionalTest.FindingUnrelatedExtensionFramesFromAboutBlank
>       (extension/about:blank -> extension can violate browsing instance)
>
> - Existing tests:
>     - ExtensionApiTest.WindowsCreate_WithOpener and _NoOpener
>       (chrome.windows.create stays in the same browsing instance depending
>        on the setSelfAsOpener parameter)
>     - AppBackgroundPageApiTest.Basic
>       (hosted app -> background page can violate browsing instance;
>        tests handling of mapping of web urls [full url, not just origin]
>        to extensions)
>
> - Manual testing:
>   - Hangouts Chrome *extension* continues to work (sufficient to
>     validate that sign-in works).  Tested with version 2017.1019.418.1.
>
> Bug:  718489 
> Test: See above.
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
> Tbr: tommycli@chromium.org
> Change-Id: Icdc9ec7bef0e35b59e04fb12385045f22db80c3a
> Reviewed-on: https://chromium-review.googlesource.com/764487
> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
> Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#521917}

TBR=dcheng@chromium.org,creis@chromium.org,thestig@chromium.org,wolenetz@chromium.org,tommycli@chromium.org,rdevlin.cronin@chromium.org,lukasza@chromium.org,rbpotter@chromium.org


Bug:  718489 ,  794079 
Change-Id: I7ee06ae1b8341044377d5e0ae975888c99d8f700
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Reviewed-on: https://chromium-review.googlesource.com/822756
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523609}
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/chrome/browser/extensions/api/tabs/tabs_test.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/chrome/browser/extensions/extension_functional_browsertest.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/chrome/renderer/extensions/chrome_extensions_renderer_client.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/chrome/renderer/extensions/chrome_extensions_renderer_client.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/components/plugins/renderer/webview_plugin.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/components/printing/renderer/print_render_frame_helper.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/browser/browsing_instance.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/browser/renderer_host/render_process_host_browsertest.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/renderer/render_frame_impl.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/content/renderer/render_view_impl.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/extensions/renderer/scoped_web_frame.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/exported/WebViewImpl.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/exported/WebViewTest.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/exported/WorkerShadowPage.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/loader/EmptyClients.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/e4c10ef6c4009f1df94fe36e0585ce59414e0b5a/third_party/WebKit/public/web/WebView.h

The current (not yet confirmed hypothesis) is that things broke, because there is an expectation that if a Chrome app has 2 webviews in the same storage partition, then they should be hosted in the same process and should be able to find and script each other.  I assume that the reverted CL broke the findability aspect.

extensions/browser/guest_view/web_view/web_view_guest.cc:

  // If we already have a webview tag in the same app using the same storage
  // partition, we should use the same SiteInstance so the existing tag and
  // the new tag can script each other.
  auto* guest_view_manager = GuestViewManager::FromBrowserContext(
      owner_render_process_host->GetBrowserContext());
  scoped_refptr<content::SiteInstance> guest_site_instance =
      guest_view_manager->GetGuestSiteInstance(guest_site);
  if (!guest_site_instance) {
    // Create the SiteInstance in a new BrowsingInstance, which will ensure
    // that webview tags are also not allowed to send messages across
    // different partitions.
    guest_site_instance = content::SiteInstance::CreateForURL(
        owner_render_process_host->GetBrowserContext(), guest_site);
  }

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

commit adc89c9b3a88435206c990d5d4a072ea3fdd6d83
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Dec 15 00:10:48 2017

Rename WebViewHelper::WebView accessor method to GetWebView.

This helps avoid using |class WebView*| syntax to disambiguate
between referring to the WebView class or to the WebViewHelper's
accessor method.

See also the "New Blink Style Guide" at https://goo.gl/s2FC7t which says
"use bare words for getters", but then adds "Where a getter's name
collides with a type name, prefix it with "Get".

Bug:  718489 
Change-Id: Ia3ace60b727d6de28e77c77690ddc561fa9b03d6
Tbr: pfeldman@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/773103
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524246}
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/bindings/core/v8/ActivityLoggerTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/events/WebInputEventConversionTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/LocalFrameClientImplTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/PrerenderingTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/WebAssociatedURLLoaderImplTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/WebDocumentTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/WebFrameSerializerSanitizationTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/WebHelperPluginTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/exported/WebViewTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/frame/BrowserControlsTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/frame/FrameSerializerTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/frame/MHTMLTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/frame/VisualViewportTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/html/forms/ExternalPopupMenuTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/input/ImeOnFocusTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/page/ContextMenuControllerTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/page/PageOverlayTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/page/ViewportTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/page/scrolling/RootScrollerTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinatorTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/core/testing/sim/SimTest.cpp
[modify] https://crrev.com/adc89c9b3a88435206c990d5d4a072ea3fdd6d83/third_party/WebKit/Source/modules/wake_lock/ScreenWakeLockTest.cpp

lfg@ - is there an easy way for renderer code to tell whether that renderer hosts a guest view?  In this situation, normal browsing-instance-granularity rules have to be broken (e.g. frames in 2 separate guestviews should be able to find each other via window.open even though there is no opener relationship between the frames/pages - see also  issue 794079  and a new regression test being added in https://crrev.com/c/833385/1..2).

Note that we know that a similar browsing-instance-granularity violation has to be introduced for extensions.  In the reverted r521917 this was handled in ChromeContentRendererClient::FindFrame [1] by asking ChromeExtensionsRendererClient::FindFrame to find the frame (potentially violating browsing instance boundaries).

So - is there a way to detect from ChromeContentRendererClient::FindFrame that the renderer hosts a guest view?  If yes, then finding a frame from the other guest view should be possible - one would just have to go through the list of all the frames hosted in the renderer process (e.g. from the g_routing_id_frame_map or g_frame_map in content/renderer/render_frame_impl.cc).

[1] https://chromium-review.googlesource.com/c/chromium/src/+/764487/19/chrome/renderer/chrome_content_renderer_client.cc

BTW: My current plan is to implement GuestViewContainer::AreGuestViewContainersPresent based on g_guest_view_container_map, but I am open to other suggestions.

So, you are interested in knowing if a parent process owns a guest? I don't think there's an easy way to do this, you'll need to add it. Adding it to GuestViewContainer sounds reasonable, just keep in mind you may have false positives -- the guest view is in the process of being destroyed but the container will still be on the map.

In theory we could tweak r521917 and modify extensions::WebViewGuest::CreateWebContents so that when it calls WebContents::Create (from //content's public API) it sets a new WebContents::CreateParams::ignore_browsing_instance_boundaries field/bool which then eventually gets propagated to RenderView on the renderer side.  This seems rather icky.

An alternative would be to propagate browsing_instance_id to the renderer side.  I think I'll try to dust off an old patch that tried doing this at https://codereview.chromium.org/2873503002/#ps60001

What does that bool do? I don't understand what it means to ignore browsing instance boundaries: it would be ignored on lookup I guess?

I would actually prefer that to plumbing IDs around...

RE: #c26

The bool could be consulted by RenderFrameImpl::FindFrame.  If present, it would do a global lookup (by looking at names of all frames from g_routing_id_frame_map).

Q: Will |BrowserPluginGuestDelegate* guest_delegate| field in WebContents::CreateParams be something that lives for a long time (i.e. something that survices replacement of Browser *Plugin* with OOPIFs)?

If yes, then presence of |guest_delegate| might be a sufficient signal from //content embedder (and we wouldn't need a new |ignore_browsing_instance_boundaries| proposed in #c25).

I've made some progress on this and have a patch that makes almost all my browsing instance tests pass locally (with modification only within internal //content code):

    RenderViewHostImpl::CreateRenderView:
    +  params->ignore_browsing_instance_boundaries =
    +      GetSiteInstance()->GetSiteURL().SchemeIs(kGuestScheme);

    ... propagating the value from RenderViewHostImpl into RenderViewImpl ...

    RenderFrameImpl::FindFrame:
    +  if (render_view_->ignore_browsing_instance_boundaries()) {
    +    for (const auto& it : g_routing_id_frame_map.Get()) {
    +      WebLocalFrame* frame = it.second->GetWebFrame();
    +      if (frame->AssignedName() == name)
    +        return frame;
    +    }
    +  }
    +

OTOH, there is still one test that is failing after my changes - I need to investigate why some tests from the WebViewTest tests suite fail after my changes with: FATAL:profile_destroyer.cc(57)] Check failed: hosts.empty() || profile->IsOffTheRecord() || profile->HasOffTheRecordProfile() || content::RenderProcessHost::run_renderer_in_process(). Profile still has 1 hosts

QUESTION: When will features::kGuestViewCrossProcessFrames ship to 100%?  (in other words, when can we stop running WebViewTest tests in */1 flavour that disables the kGuestViewCrossProcessFrames feature?)

Following up on the DCHECK mentioned in #c30 - it turns out that the new test triggers the DCHECK 1) even without my changes and 2) only when running the new test with kGuestViewCrossProcessFrames disabled.  The findability aspect of my change seems to work fine.  Since kGuestViewCrossProcessFrames will soon become the default, maybe it might be okay to just disable the new test in the old mode (AFAIK if the fix lands on Tuesday, then it will make it into the next Chrome Dev channel release).

FWIW, the DCHECK seems to happen, because the following route/listener is added and never removed from the leaked RenderProcessHostImpl:

  1 RenderProcessHostImpl::AddRoute(...) ../../content/browser/renderer_host/render_process_host_impl.cc:2207:33
  2 RenderWidgetHostImpl::RenderWidgetHostImpl(...) ../../content/browser/renderer_host/render_widget_host_impl.cc:371:13
  3 RenderWidgetHostFactory::Create(...) ../../content/browser/renderer_host/render_widget_host_factory.cc:25:14
  4 RenderViewHostFactory::Create(...) ../../content/browser/renderer_host/render_view_host_factory.cc:53:24
  5 FrameTree::CreateRenderViewHost(...) ../../content/browser/frame_host/frame_tree.cc:360:40
  6 RenderFrameHostManager::CreateRenderFrameProxy(...) ../../content/browser/frame_host/render_frame_host_manager.cc:1670:56
  7 FrameTree::CreateProxiesForSiteInstance(...) ../../content/browser/frame_host/frame_tree.cc:253:33
  8 RenderFrameHostManager::CreateOpenerProxies(...) ../../content/browser/frame_host/render_frame_host_manager.cc:2226:0
  9 RenderFrameProxyHost::UpdateOpener() ../../content/browser/frame_host/render_frame_proxy_host.cc:216:51
 10 RenderFrameHostManager::DidChangeOpener(...) ../../content/browser/frame_host/render_frame_host_manager.cc:299:18

RFHM::CreateRenderFrameProxy creates a new RVHI, but doesn’t associate it with a RFPH.  This leads to leaking of the RVHI responsible for the IPC routing mentioned in #c31 above (normally an RVHI is potentially destroyed when its refcount is decremented by RFHI or RFPH, but here the RVHI is not associated with RFHI or RFPH and so its refcount stays at 0 refcount for the whole time).

I wonder if it is ever okay to have a situation where RFHM::GetRenderFrameProxyHost(instance) would find an already existing, live RFPH, but frame_tree_node_->frame_tree()->GetRenderViewHost(instance) wouldn’t find a RVHI?  If such situation is okay, then the DCHECK from above can be fixed by simply doing the check for an old RFPH and returning with it *before* considering creating RVHI.

Re c#28, I think a lot of the functionality for the BrowserPluginGuestDelegate is still relevant for OOPIF-based guests, so it seems likely that it will stick around (although will likely need renaming at some point).

Re c#30/31 - I *think* it should be possible to ignore any WebView */0 tests, but I don't even know what it means to run a */1 test with kGuestViewCrossProcessFrames disabled, since the '1' says it should be enabled, by definition.

wjmaclean@ - thanks for pointing this out - I think I was indeed confused wrt the meaning of /0 vs /1 tests.

FWIW, based on my reading of the constructor RenderFrameProxyHost (which explicitly checks whether the |render_view_host| argument is null or not), I think it is okay/legitimate to have RenderFrameProxyHost without a corresponding RenderViewHost.  An example of a callstack that creates such RenderFrameProxyHost is below:

  1 content::RenderFrameProxyHost::RenderFrameProxyHost(...) ../../content/browser/frame_host/render_frame_proxy_host.cc:68:35
  2 content::RenderFrameHostManager::CreateRenderFrameProxyHost(...) ../../content/browser/frame_host/render_frame_host_manager.cc:817:11
  3 content::RenderFrameHostManager::CreateOuterDelegateProxy(...) ../../content/browser/frame_host/render_frame_host_manager.cc:1744:7
  4 content::WebContentsImpl::AttachToOuterWebContentsFrame(...) ../../content/browser/web_contents/web_contents_impl.cc:1628:19
  5 guest_view::GuestViewMessageFilter::OnAttachToEmbedderFrame(...) ../../components/guest_view/browser/guest_view_message_filter.cc:174:23 

Based on this, I think I'll go forward with the memory leak fix proposed in #c32.  I'll upload a new patchset and kick off tryjobs later today.

Re#30-32: While web_view_browsertests aren't needed anymore without GuestViewCrossProcessFrames, this particular code is relevant for PDFs, so we should try to find a reasonable solution that we can live with for at least a few months/quarters.

So I have some questions:
1. Is the precondition for hitting the DCHECK embedding multiple webviews in the same process (i.e. storage partition)?
2. Why don't we hit any DCHECKs with pages embedding multiple PDFs?
3. Can you test loading PDFs after the change to not create RVHs?

+lazyboy@, +nasko@ (author and //content reviewer of r336565)

AFAICT, RenderFrameProxyHost's constructor used to have a CHECK that |render_view_host| argument is never null, but this has changed in r336565 (e.g. see https://codereview.chromium.org/972313002/patch/1060001/1070020).

Really +lazyboy@, +nasko@ :-)

1. Is the precondition for hitting the DCHECK embedding multiple webviews in the same process (i.e. storage partition)?

Yes.  To hit the leak, we need to have multiple guestviews in the same storage partition, and one of the guestviews needs to be set (via window.open finding an already existing guestview) as an opener of another guestview.

2. Why don't we hit any DCHECKs with pages embedding multiple PDFs?

Probably because window.opener is not set for PDFs.

3. Can you test loading PDFs after the change to not create RVHs?

It is not clear how to construct a PDF-based scenario that would trigger the code affected by the leak fix proposed above.  We've discussed this over chat and we think there might not be a need to try a PDF-based scenario (beyond what is already covered by automatic tests).

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

commit af2f3357c278d99e9efb4cf3948f6576e47266ec
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Jan 17 14:05:08 2018

[reland] Improve granularity of window namespaces in Blink.

This is a reland of r521917 that got reverted in r523609 because of
 https://crbug.com/794079 .

This CL ensures that blink::FrameTree::Find(const AtomicString& name)
only looks for a match in the current set of related browsing contexts
(represented in the browser by content::BrowsingInstance).  This CL
means that window.open's behavior won't change just because a renderer
process happens to host multiple unrelated browsing contexts (possible
for example after reusing a renderer process because of hitting the
process limit).

This CL consists of 3 parts:

- New browser tests.
  - RenderFrameHostManagerTest.ProcessReuseVsBrowsingInstance
    for verifying browsing instance boundaries when the
    renderer processes get reused.
  - ExtensionFunctionalTest.FindingUnrelatedExtensionFramesFromAboutBlank
    for verifying that extensions can still lookup unrelated frames
    from the same extension.

- Having blink::Page maintain a set of related pages.
  - Page::next_related_page_ and Page::prev_related_page_ form a
    circular, double-linked list of related pages.
  - Page::CreateOrdinary takes a new |opener| parameter and treats the
    new page and the |opener| as related and puts them on the same list.
  - |opener| is propagated from content::RenderViewImpl, through
    blink::WebViewImpl into blink::Page.

- Falling back to blink embedder when blink::FrameTree::Find finds no frame
  with the given name.
  - The fallback is needed to preserve the old behavior for extensions
    and guestviews.
  - For extensions the fallback goes through blink::LocalFrameClient,
    blink::WebFrameClient / content::RenderFrameImpl,
    content::ContentRendererClient / ::ChromeContentRendererClient,
    ::ChromeExtensionsRendererClient and finally is implemented
    by extensions::ExtensionFrameHelper.
  - Currently extensions::ExtensionFrameHelper iterates through all
    same-origin frames in the given process, but requires that the
    |relative_to_frame| is an extension frame.  In the future we might
    want to restrict piercing of browsing instances to specific
    scenarios where it is needed (e.g. restrict it to background pages /
    contents only?).
  - Additionally RenderFrameImpl::FindFrame checks all frames within the
    given process if |render_view_->renderer_wide_named_frame_lookup()|
    is set (which is the case for RenderView constructed for guestviews,
    which need to be able to find each other if they are in the same
    storage partision = if they are in the same renderer process).

I've tested this CL via:
(all tests below pass before and after the CL, except for
ProcessReuseVsBrowsingInstance which is fixed by this CL)

- New tests:
    - RenderFrameHostManagerTest.ProcessReuseVsBrowsingInstance
      (web -> web shouldn't violate browsing instance)
    - ExtensionFunctionalTest.FindingUnrelatedExtensionFramesFromAboutBlank
      (extension/about:blank -> extension can violate browsing instance)

- Existing tests:
    - ExtensionApiTest.WindowsCreate_WithOpener and _NoOpener
      (chrome.windows.create stays in the same browsing instance depending
       on the setSelfAsOpener parameter)
    - AppBackgroundPageApiTest.Basic
      (hosted app -> background page can violate browsing instance;
       tests handling of mapping of web urls [full url, not just origin]
       to extensions)
    - WebViewTest.FindabilityIsolation
      (guestviews in the same storage partition should be able to find
       each other - regression test for  https://crbug.com/794079 ;
       added in a recent https://crrev.com/c/868631)

- Manual testing:
  - Hangouts Chrome extension
    (nckgahadagoaajjgafhacjanaoiihapd - 2017.1019.418.1).
    - Opens fine (i.e. no impact from r516081)
    - Opens fine in presence of gmail (i.e. b/71647499 is fixed)
  - Hangouts Chrome app
    (knipolnnllmklapflnccelgolnpehhpl - 2017.420.419.1)
    - Opens fine (i.e. no regression of  https://crbug.com/794079 )

Original CL was reviewed on https://crrev.com/c/764487.
The reland (this) CL was reviewed on https://crrev.com/868313.

Bug:  718489 ,  794079 
Test: See above.
Change-Id: I7f6ce7c71a3f995d52797dd187c6c18dee766e4e
Tbr: tommycli@chromium.org
Tbr: Lei Zhang <thestig@chromium.org>
Tbr: Matthew Wolenetz <wolenetz@chromium.org>
Tbr: Rebekah Potter <rbpotter@chromium.org>
Tbr: Charlie Reis <creis@chromium.org>
Tbr: Devlin <rdevlin.cronin@chromium.org>
Tbr: Daniel Cheng <dcheng@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/868313
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529731}
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/chrome/browser/extensions/api/tabs/tabs_test.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/chrome/browser/extensions/extension_functional_browsertest.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/chrome/renderer/extensions/chrome_extensions_renderer_client.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/chrome/renderer/extensions/chrome_extensions_renderer_client.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/components/plugins/renderer/webview_plugin.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/components/printing/renderer/print_render_frame_helper.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/browser/browsing_instance.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/browser/renderer_host/render_process_host_browsertest.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/common/renderer.mojom
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/renderer/render_frame_impl.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/renderer/render_view_impl.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/content/renderer/render_view_impl.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/extensions/renderer/scoped_web_frame.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/exported/WebViewImpl.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/exported/WebViewTest.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/exported/WorkerShadowPage.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/loader/EmptyClients.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/public/web/WebFrameClient.h
[modify] https://crrev.com/af2f3357c278d99e9efb4cf3948f6576e47266ec/third_party/WebKit/public/web/WebView.h

Commit af2f3357... initially landed in 65.0.3324.0.  I've verified that the issue is fixed in Chrome Canary on Windows (e.g. Hangouts extension [nckgahadagoaajjgafhacjanaoiihapd - 2017.1019.418.1] continues to work even if some frames from unrelated BrowsingInstances [e.g. from mail.google.com] share a process).

Marking the issue as fixed.  Let's hope the fix sticks this time.

+govind@ / TPM for M65 release

Would it be okay / possible / desirable to merge r529731 into the latest Dev release (to help mitigate b/71647499 for people willing to use the Dev channel)?  I assume I would merge into branch 3322.  Is there any approval necessary?  Would a respin happen automatically (or do I have to go through some extra steps to kick off a build and release process)?

Per offline chat with  lukasza@, this fix will be included in next week dev release. No merge is needed.

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

commit 9554691983eea9f397987ef4811cf3cb8017868b
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Jan 30 16:15:22 2018

UMA recording the kind of target frame when extensions pierce browsing
instance.

Extensions violate browsing instance boundaries.  The UMA metrics added
in this CL should help confirm or deny the theory that violating
browsing instance boundaries is only needed for looking up background
contents / pages (of VIEW_TYPE_BACKGROUND_CONTENTS type).

Bug:  718489 
Change-Id: I69246c7d38962c85c55b2e4d271b19eaae1dae1e
Reviewed-on: https://chromium-review.googlesource.com/766469
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532899}
[modify] https://crrev.com/9554691983eea9f397987ef4811cf3cb8017868b/base/test/histogram_tester.h
[modify] https://crrev.com/9554691983eea9f397987ef4811cf3cb8017868b/chrome/browser/extensions/extension_functional_browsertest.cc
[modify] https://crrev.com/9554691983eea9f397987ef4811cf3cb8017868b/extensions/common/manifest.h
[modify] https://crrev.com/9554691983eea9f397987ef4811cf3cb8017868b/extensions/common/view_type.h
[modify] https://crrev.com/9554691983eea9f397987ef4811cf3cb8017868b/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/9554691983eea9f397987ef4811cf3cb8017868b/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/9554691983eea9f397987ef4811cf3cb8017868b/tools/metrics/histograms/histograms.xml

Is cl listed at #45 need to be merged to M65? If yes, pls request a merge.

Also is M64 merge is needed due to b/71647499?

My appologies - the CL at #45 should have been associated with issue 786411.  No need to do a merge.

No worries at all, thank you for confirmation.