As shown by https://codereview.chromium.org/2873503002 this is not a hard blocker for  issue 718489 .  Additionally, I've noticed that adding an opener relationship here will change frame lookup behavior - please see the notes below.

Scenario #1:

1. |extension_contents| calls chrome.windows.create:
   - let's call the new window |new_contents|
   - let's assume the URL of the new window is the same extension

Current behavior:
- |extension_contents| and |new_contents| behave as if they were in the same browsing instance - they can call window.open to find each other's frames by name
- window.opener of |new_contents| is null - this is weird / inconsistent with the browsing instance.


Scenario #2:

1. Step #1 is the same as in scenario #1
2. |new_contents| uses |window.location = 'https://...'| to navigate to a web URL

Current behavior:
- |new_contents| is transferred out of the extension process and so it seems as if in step 2 |new_contents| are leaving the browsing instance of |extension_contents| - after step 2, both windows cannot find each other via window.open.  This is weird -
 I don't think a window can leave a browsing instance via a renderer initiated navigation in any other case (?).


So - if we made chrome.windows.create establish an opener relationship, then the behavior in scenario #2 would change (the web frame from |new_contents| would start being able to lookup the extension frame that opened it).  I am not sure if this is desirable.

After a closer look, it appears that this issue actually *is* blocking  issue 718489 .

In case of the hangouts extension, the following things happen:
1. frame1: contains content from the hangouts extension
2. frame1: chrome.windows.create - creates a "panel" window (frame2)
3. frame2: navigates to web origin X
4. frame1: navigates to web origin X
5. frame2 tries to find frame1 via window.open(..., 'name-of-frame1')

Steps 3 and 4 transfer the frames to another process.  This process (a bit accidentally) happens to be the same process for both frames.  Being in the same process, the frames can find each other via the global Page::OrdinatePages() set.

What is important here, is that this violation of browsing instance boundaries is indistinguishable from the violations we want to stop in  issue 718489 .  In both cases a web frame tries to look another web frame that is not in the same browsing instance.  In both cases the web frames end up in the same process accidentally (in one case because of a quirk in chrome.windows.create + how transfer to a web page happens to transfer both frames to the same process, in another case because of hitting the process limit).


I think to fix this, we want to ensure that chrome.windows.create sets up a proper opener relationship between the newly created contents and the extension frame calling chrome.windows.create.  I am tentatively considering replacing |bool force_new_process_for_new_contents| field in chrome::NavigateParams with |RenderFrameHost* opener|
 field.

Please shout if the plan above is not the right plan.

If we end up with proper openers as a result, my only concern is whether a web page running in a window opened by the extension could just navigate its opener. If that is possible, then I think we might have some issues, as WebContents for the extension that created the new window will have a VIEW_TYPE_EXTENSION_*, yet it could be navigated to a random web URL. This will be confusing and could be cause of bugs, even if we properly navigated cross-process into a web renderer process.

I've just double checked what I wrote in #c2 - if we modify the scenario from #c3 by removing step4 (so that only the new frame (frame2) navigates to a web page, but the old frame (frame1) stays at an extension URL), then step5 is broken - frame2 cannot find frame1, despite being in the same browsing instance according to SiteInstance::IsRelatedSiteInstance.  I am guessing that we don't create RenderFrameProxy for the opener^H^H^Hcreator in this case (I assume we create proxies for real openers instead of iterating over frames in a content::BrowsingInstance).

IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowsCreateAdHocTest2) {
  const extensions::Extension* extension =
      LoadExtension(test_data_dir_.AppendASCII("../simple_with_file"));
  ASSERT_TRUE(extension);

  // Navigate a tab to an extension page.
  GURL extension_url = extension->GetResourceURL("file.html");
  ui_test_utils::NavigateToURL(browser(), extension_url);
  content::WebContents* old_contents =
      browser()->tab_strip_model()->GetActiveWebContents();

  // Execute chrome.windows.create and store the new tab in |new_contents|.
  content::WebContents* new_contents = nullptr;
  {
    content::WebContentsAddedObserver observer;
    ASSERT_TRUE(content::ExecuteScript(old_contents,
                                       "window.name = 'old-contents';\n"
                                       "chrome.windows.create({url: '" +
                                           extension_url.spec() + "'})"));
    new_contents = observer.GetWebContents();
    ASSERT_TRUE(content::WaitForLoadStop(new_contents));
  }

  // Navigate the new tab to a web URL.
  ASSERT_TRUE(StartEmbeddedTestServer());
  GURL web_url1 = embedded_test_server()->GetURL("/title1.html");
  {
    content::TestNavigationObserver nav_observer(new_contents, 1);
    ASSERT_TRUE(content::ExecuteScript(
        new_contents, "window.location = '" + web_url1.spec() + "';"));
    nav_observer.Wait();
  }
  EXPECT_EQ(web_url1, new_contents->GetMainFrame()->GetLastCommittedURL());

  // |old_contents| is at an extension origin,
  // |new_contents| is at a web origin.
  EXPECT_NE(
      old_contents->GetMainFrame()->GetSiteInstance(),
      new_contents->GetMainFrame()->GetSiteInstance());

  // Both frames are in the same browsing instance (according to content::SiteInstance).
  EXPECT_TRUE(
      old_contents->GetMainFrame()->GetSiteInstance()->IsRelatedSiteInstance(
          new_contents->GetMainFrame()->GetSiteInstance()));

  // |new_contents| CANNOT find |old_contents| using window.open/name.
  std::string location_of_other_window;
  EXPECT_TRUE(ExecuteScriptAndExtractString(
      new_contents,
      "var w = window.open('', 'old-contents');\n"
      "window.domAutomationController.send(w.location.href);",
      &location_of_other_window));
  EXPECT_NE(old_contents->GetMainFrame()->GetLastCommittedURL().spec(),
            location_of_other_window);
  EXPECT_EQ("about:blank",
            location_of_other_window);
}

I have a CL that makes chrome.windows.create establish an actual "opener" relationship: https://codereview.chromium.org/2921753002.  I hope this CL can be landed as-is, to unblock fixing of  issue 718489  (via https://crrev.com/2873503002).

In particular:

- Concerns about the new ability of (web) openee navigating the (extension) opener:

    - VIEW_TYPE_EXTENSION_*:

        - This designation can "lie" today (without depending on the new ability).
          For example - the extension can navigate it's main frame to a web url.

        - I only had a cursory look, but it seems to me that this designation
          is more related to the lifetime/view management (i.e. background page
          is never visible and launches with Chrome) rather than to security
          decisions.

    - If an extension wants to avoid the opener relationship, then it can use
      chrome.tabs.create API, rather than chrome.windows.create API.

        - In the future we can consider changing this, but hopefully the future
          changes are not a prerequisite for landing https://crrev.com/2921753002.
        
        - In the future maybe chrome.windows.create should have a noopener-kind-of
          flag?

        - In the future maybe chrome.tabs.create should have a opener-please flag?

    - I think consistent rules are good - I'd rather not add extension-specific
      exceptions to blink::LocalFrame::CanNavigate (which in particular returns
      true if |target_frame == Client()->Opener()| in CanNavigateWithoutFramebusting).

- Overall, I think consistency is good.

  - Today chrome.windows.create creates new frames in a "browsing instance" that is
    almost, but not quite are entirely unlike the real browsing instance established
    by window.open.  This CL makes things more consistent.

  - Just like window.open, chrome.windows.create should be immune to process allocation
    decisions (tracked by  issue 718489 ).  I think making both of these API consistent
    is the best way to accomplish this.


So - with the above out of the way, I plan to send https://codereview.chromium.org/2921753002 out for review.  If there are high-level design concerns, then we can put the review on hold and discuss the design in the bug, here.  Otherwise, let's try to review and polish and land that CL.

Thanks for the detailed investigation and thoughts.  At first, I shared some of Nasko's concern that we'll be making it easier for web pages to navigate extension pages that created them, but we already face that problem in other cases, since we tend to keep extensions and web pages in the same BrowsingInstance:
1) A web iframe inside an extension page (e.g., extension background page) can navigate the main frame to a web page.
2) An extension background page can use window.open to a web URL, and the new window has an opener that it can use to navigate the background page to a web page.

This is admittedly weird and might pose a risk, but we should follow up on that separately, since it already affects these existing cases.

Otherwise I agree that adding an opener makes sense here, making it more like other cases where we stay in the same BrowsingInstance.

The remaining downside is that chrome.windows.create and chrome.tabs.create have different behavior for no obvious reason to me.  I suppose we could make chrome.tabs.create stay in the same BrowsingInstance, but like Nick, I'm hesitant to make this more permissive if it isn't required for compatibility.  It's always easy to open it up later if there's a use case, but much harder to remove it if we decide it was a bad idea.  I suppose we can live with the confusing API difference, unless there are objections.

I seem to be agreeing with lukasza's approach in the CL, so I'll go review it.

Sorry for the delay here - it's been a crazy week.  I'm a little concerned about setting an opener relationship by default for chrome.windows.open.  In the average case, it makes me a little worried (since an opened window could determine the extension that opened it through window.opener), and in a few specific cases it degrades rapidly.  One area in particular I'd be worried about is session managers - if you use an extension to restore your session, the result could be that your entire browser ends up in a single browsing instance, right?

Nasko and I bounced some ideas of each other yesterday, and I think what we decided would be the simplest approach is simply making this optional by introducing a new property to the chrome.windows.create params - something like setSelfAsOpener, which can default to false.  That way, the majority of cases still default to the safest approach (separate browsing instance, no openers), but the extension could opt-in to sharing a browsing instance.  This also, I think, makes the default chrome.windows.create behavior a little more consistent with the tabs behavior.

What do you think?  Would this satisfy the requirements?

Thanks for the feedback Devlin and pointing out the session manager scenario - I agree that the problem in this scenario should be considered a blocker.  I think that the only answer I could give for the approach from https://crrev.com/2921753002 is that hopefully the session managers use chrome.tabs.create rather than chrome.windows.create, but this answer doesn't seem very compelling to me.


RE: an opened window could determine the extension that opened it through window.opener

Can you help me understand how the opened window would achieve this?  FWIW, I've just double-checked that accessing window.opener.location.href is blocked if the opener is cross-site.


RE: setSelfAsOpener param

I like the idea of the caller explicitly controlling the behavior here.  I agree that opt-in approach (i.e. defaulting to no-opener/no-shared-browsing-instance) seems best in the long-term (as compared to defaulting to current behavior and opting out via something like noOpener param).

Still - the opt-in approach can be a breaking change for existing extensions.  Can you please offer your thoughts on this?

- I assume that if an extension adds not-yet-unrecognized things (like setSelfAsOpener) to the chrome.windows.create property bag, then Chrome will degrade gracefully (just ignoring the unrecognized property).

- I can try to try driving a change in hangouts (which is the only extension we know for sure depends on the current behavior and would be broken).  Any thoughts on whether this would be sufficient for landing setSelfAsOpener on trunk?  

    - I am not sure if we can count on finding out if any extensions got broken before the change hits Beta and/or Stable channel.  Maybe at this point it would be okay to ask the extension authors to add setSelfAsOpener (since this seems like a fairly simple fix)?

    - I am not sure if a base::Feature-based Finch kill-switch is feasible here.  It is certainly possible, but might complicate the code quite a bit (since chrome::Navigate would have to keep the old and the new intertwined paths).

    - I don't think UMA would help here (i.e. I don't see how to distinguish this particular flavour of frame lookup VS other, non-extension-related lookups).

> RE: an opened window could determine the extension that opened it through window.opener

> Can you help me understand how the opened window would achieve this?  FWIW, I've just double-checked that accessing window.opener.location.href is blocked if the opener is cross-site.

Ah, okay, that makes me feel a bit better.  I was worried that they would be able to access some details about the opener since they were in the same browsing instance.  I still feel uneasy about it, but this is better.  That said, I think the session manager case and preferring to default to separate browsing instances is still significant enough reason.

> I assume that if an extension adds not-yet-unrecognized things (like setSelfAsOpener) to the chrome.windows.create property bag, then Chrome will degrade gracefully (just ignoring the unrecognized property).

Very unfortunately - no.  Chrome will throw an error.  This means that extensions would have to compensate by either version-checking or doing something like:
try {
  chrome.windows.create({setSelfAsOpener: true}, ...);
} catch (e) {
  chrome.windows.create({}, ...);
}

It's not great, but I think it's better than silently opting in other extensions for what I think is pretty unusual behavior.

> I can try to try driving a change in hangouts (which is the only extension we know for sure depends on the current behavior and would be broken).  Any thoughts on whether this would be sufficient for landing setSelfAsOpener on trunk?  
> I am not sure if we can count on finding out if any extensions got broken before the change hits Beta and/or Stable channel.  Maybe at this point it would be okay to ask the extension authors to add setSelfAsOpener (since this seems like a fairly simple fix)?

AFAIK, this is completely undocumented behavior that no one should be relying on (given how strange it is).  I'm fine with having extension developers that rely on this be responsible for fixing it (though the burden of fixing hangouts may fall to us).

> I am not sure if a base::Feature-based Finch kill-switch is feasible here.  It is certainly possible, but might complicate the code quite a bit (since chrome::Navigate would have to keep the old and the new intertwined paths).
> I don't think UMA would help here (i.e. I don't see how to distinguish this particular flavour of frame lookup VS other, non-extension-related lookups).

I agree that this isn't great for finch and UMA wouldn't be very useful.


-----


Overall, I'm fine with landing the patch to provide a mechanism for doing this and having it be default-off.  Given we aren't aware of extensions other than hangouts relying on this (and given it's totally undocumented), I think this should be okay, and existing extensions can update if they do rely on this behavior.

Modifying the Hangouts extension will be tracked in b/62491623 (internal to Google).

I've updated https://crrev.com/2921753002 to introduce setSelfAsOpener parameter (OTOH, a detailed CR of this CL can probably wait until we are able to modify the Hangouts extension).

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

commit a44664786d0d3235f9f050bd68e58a362d571f9a
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Nov 13 22:32:59 2017

Making chrome.windows.create establish an actual "opener" relationship.

Before this CL, windows (or tabs, panels, etc.) created by
chrome.windows.create API form a relationship that is almost,
but not quite, like an opener relationship:

1. Unlike in a true opener relationship, the 2 frames (opener and
   openee) can find each other, but only if they are still in the same
   process.  For example, if only 1 frame navigates cross-process,
   then the frames won't be able to find each other (for a specific
   example please see  https://crbug.com/713888#c5 ).

2. Unlike in a true opener relationship, window.opener is not set
   (and consequently cannot be navigated by the openee).


After this CL, chrome.windows.create API forms a true opener
relationship *iff* the new parameter - setSelfAsOpener is used.
Otherwise, the newly opened window is created in a separate browsing
instance.  This is required for later restricting named frame lookup
to the current browsing instance (see  https://crbug.com/718489 ).

The CL establishes a true opener relationship, by having
extensions::WindowsCreateFunction::Run pass an opener (a
RenderFrameHost*) into chrome::NavigateParams, rather than using
|force_new_process_for_new_contents| field which this CL removes.


This CL tweaks ExtensionApiTest.WindowsCreateVsBrowsingInstance test
so that:

- The test is closer to what the hangouts extension does
  (it navigates both windows to the same web origin and *then* attempts
  to lookup one window from the other).  Without this modification, the
  test would NOT have caught the hangouts extension regression that was
  almost introduced by https://crrev.com/2873503002).

- The test verifies the opener relationship (i.e. |window.opener|,
  findability via |window.open|, SiteInstance::IsRelatedSiteInstance)
  rather than accidental, indirect manifestations of having an opener
  relationship (i.e. being in the same process or site instance).


Relevant automated tests:
- Tweaked test:
  - ExtensionApiTest.WindowsCreateVsSiteInstance
    (the window.open part passes before and after this CL; the
     window.opener part + the IsRelatedSiteInstance part only pass after
     this CL)
- Other tests:
  - AppBackgroundPageApiTest.Basic
    (hosted app -> background page can violate browsing instance; tests
     handling of mapping of web urls [full url, not just origin] to
     extensions)
  - CtrlClickShouldEndUpInNewProcessTest.* tests
    (tests for behavior that used to depend on the removed
     |force_new_process_for_new_contents| field).

I have also verified that the Hangouts extension (current prod version
- 2017.1019.418.1) works fine before and after this CL (i.e. I have
verified that this CL doesn't regress  https://crbug.com/597750 ).

Bug:  713888 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Test: See above.
Change-Id: If23ac7b72256bfcd695fc4bc3759a5bd73f4812f
Reviewed-on: https://chromium-review.googlesource.com/758801
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516081}
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/chrome/browser/extensions/api/tabs/tabs_test.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/chrome/browser/ui/browser_navigator_params.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/chrome/browser/ui/browser_navigator_params.h
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/chrome/common/extensions/api/windows.json
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/content/browser/frame_host/navigator.h
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/content/browser/frame_host/navigator_impl.h
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/content/public/browser/page_navigator.cc
[modify] https://crrev.com/a44664786d0d3235f9f050bd68e58a362d571f9a/content/public/browser/page_navigator.h

I think we can mark this as fixed, because:

- It does seem fixed - after #c12 we have the following behavior:

    - both chrome.windows.create and chrome.tabs.create default to creating the new WebContents in a new/separate BrowsingInstance.

    - if chrome.windows.create explicitly sets setSelfAsOpener, then in addition to reusing the BrowsingInstance we will also set window.opener

- So far #c12 seems to be sticking and soon we will start landing dependent CLs that will make it difficult to revert (like https://crrev.com/c/764487)