Closed Bug 989501 Opened 10 years ago Closed 10 years ago

[e10s] Rather than opening a new window, we always open a new tab (pop ups don't work)

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
e10s + ---

People

(Reporter: billm, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 40 obsolete files)

827 bytes, text/html
Details
6.73 KB, patch
mconley
: review+
Details | Diff | Splinter Review
36.25 KB, patch
mconley
: review+
Details | Diff | Splinter Review
11.10 KB, patch
mconley
: review+
Details | Diff | Splinter Review
16.86 KB, patch
mconley
: review+
Details | Diff | Splinter Review
If a page tries to get a new window in e10s, it will always get a new tab instead. This comes down to the fact that TabChild implements a very simple version of ProvideWindow. The full version used outside of e10s is here:

http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#804

I think we'll probably need to move some of that code to a place where it can be shared with TabChild. Does that seem reasonable, Olli?
Flags: needinfo?(bugs)
Blocks: e10s-m1
Summary: [e10s] Rather than opening a new window, we always open a new tab → [e10s] Rather than opening a new window, we always open a new tab (pop ups don't work)
Snag.
Assignee: nobody → mconley
Attached file testcase.html (obsolete) —
This has a link that opens a popup, which will be useful for testing.
OS: Linux → All
IIRC the idea was that parent process would just open a new window if needed.
But looks like http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#1908 got 
b2g-fied a bit.
But couldn't nsContentTreeOwner::ProvideWindow just be called in the parent process, and
then oop-DOMWindow would be returned to child process?
Flags: needinfo?(bugs)
Attached patch WIP Patch 1 (obsolete) — Splinter Review
Status: NEW → ASSIGNED
Attached file testcase.html (obsolete) —
Added target="_blank" test case (requires browser.link.open_newwindow tweak[1] to open a new window with it):

[1]: http://kb.mozillazine.org/Browser.link.open_newwindow
Attachment #8412044 - Attachment is obsolete: true
Attached file testcase.html
More refinement...
Attachment #8418087 - Attachment is obsolete: true
Ok, so I had a long talk with smaug on how to move forward on this, since I've been starting and stopping and fumbling along.

Part of the slowdown here is that we've got some pretty crufy window-opening APIs, as well as a bunch of window opening logic spread around quite a bit (for the single-process case, we've got this logic spread about in nsContentTreeOwner::ProvideWindow, nsWindowWatcher::OpenWindowInternal, and nsBrowserAccess in browser.js).

So our plan for solving this bug goes something like this (and smaug might jump in and correct me if / where I go wrong):

1) The TabChild::ProvideWindow can handle the case where a window-opening link is instead opened in the current tab (thanks to user preferences[1][2]) without ever having to communicate with the TabParent. This is the simple case, and pretty straight-forward. It's unfortunate that TabChild::ProvideWindow will need to duplicate some of the logic from nsContentTreeOwner::ProvideWindow to do this, but perhaps we can file a follow-up bug to somehow share this code somewhere down the line.

2) For the cases where we're opening a new window or a new tab... things get a bit trickier. We need to get the parent process to open a new window with a single remote tab, or just a new remote tab, and get the remote browser sent to the child process for it to send to the link being opened.

The new tab case is pretty straight forward, since this is what we've been doing the whole time - the TabParent can use nsIBrowserDOMWindow's openURIInFrame with OPEN_NEWTAB, and return the remote browser. No problems there, I don't think.

The real problem is the new window case. This is the tricky one.

smaug and I mulled over the best way to approach this, and what we're thinking is that each ContentChild can instantiate an object that implements nsIWindowCreator2, and then calls SetWindowCreator on that processes nsIWindowWatcher with that new object. That new object will know how to forward calls to createChromeWindow2/createChromeWindow to the callers TabChild, which will call the TabParent, which will call the parent-processes nsIWindowCreator2 to actually create a new window. The idea here is that the child-processes nsIWindowWatcher should be able to behave in almost the exact same fashion as in the single-process model, which is nice - no need to special case all over the place.

I'm still a bit fuzzy on how we go from creating that chrome window to passing a remote browser back to the TabChild. I do know that nsIWindowCreator2 hands back an nsIWebBrowserChrome, which TabChild's implement - so somehow, the TabParent, once it's opened the chrome window, needs to give the calling TabChild enough information so that this new window's TabChild can be passed back to nsWindowWatcher.

It's all really quite convoluted. This code is desperately in need of some refactoring. I might take that on once I close this (Ms2ger has been egging me on).

[1]: http://kb.mozillazine.org/Browser.link.open_newwindow
[2]: http://kb.mozillazine.org/Browser.link.open_newwindow.restriction
Slight course-correction to the above plan - billm suggested that instead of communicating from TabChild to TabParent to get the window to open, we communicate from ContentChild to ContentParent to make it happen. He says it seems cleaner that way, and I'll defer to his judgement on that.
Hmm, well, I expect that parent needs to know which tab is trying to open a new window.
At least for the multi display case.
Ok, after some long consultations with smaug (thanks smaug!), I've got a direction to move forward on this.

For one, because of smaug's concern in comment 10, I'm sticking with the original plan of having the IPC communication take place between the TabChild / TabParent for opening new windows.

The challenge here is that the TabChild wants us to hand over a PBrowserParent** after the window opens. Unfortunately, there's no nice way of getting to the remote browser from an nsIWebBrowserChrome.

So, the plan goes something like this:

We will have the TabParent call directly into the window creator's CreateChromeWindow2 to create the new window. It will need to pass itself in as a parameter.

CreateChromeWindow2 will have to pass the TabParent along to nsXULWindow::CreateNewWindow...which passes it along to CreateNewContentWindow... which passes it along to nsAppShellService::CreateTopLevelWindow, which passes it to nsAppShellService::JustCreateTopWindow, which passes it to nsWebShellWindow::Initialize, which will stash the TabParent (or its frameloader) into the newly created docshell.

Eventually, once the XUL document has finished loading, the frameloader for the new window is going to have ::TryRemoteBrowser called on it. This will call the static function ContentParent::CreateBrowserOrApp. Somehow, this function has to get a handle on the original TabParent, and use its ContentParent to create the new browser, stash its frameloader into the docshell, and then unwind the stack.

Eventually, we get back to the TabParent who called CreateChromeWindow2 in the first place. This function needs to grab the newly created frameloader that was stashed in the docshell, and after some conversion, pass back something compatible with PBrowserParent**.

It's still a little hand-wavey, but I think I'm starting to get a better sense of how all of these pieces fit together.
Comment on attachment 8423514 [details] [diff] [review]
Part 5: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it.

Whoops, please ignore.
Attachment #8423514 - Attachment is obsolete: true
Attached patch Parts 1-5 (folded) (obsolete) — Splinter Review
Attachment #8416180 - Attachment is obsolete: true
Comment on attachment 8423515 [details] [diff] [review]
Part 1: Make it possible to stash opener and newly created TabParent's inside DocShell. r=?

This, like all of the other patches, are WIP - but it's large enough that I felt it necessary to break it into chunks.

So here's chunk 1.
Attachment #8423515 - Flags: feedback?(bugs)
Comment on attachment 8423517 [details] [diff] [review]
Part 2: Make it possible to pass in an nsITabParent when opening windows, and then stash that nsITabParent in the newly created chrome docshell. r=?

Chunk 2...
Attachment #8423517 - Flags: feedback?(bugs)
Comment on attachment 8423518 [details] [diff] [review]
Part 3: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser. r=?

Chunk 3
Attachment #8423518 - Flags: feedback?(bugs)
Comment on attachment 8423519 [details] [diff] [review]
Part 4: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=?

etc...
Attachment #8423519 - Flags: feedback?(bugs)
Comment on attachment 8423520 [details] [diff] [review]
Part 5: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it.

Hey felipe - here's my hack to make newly created windows set their initial browser to remote="true" if the CHROME_REMOTE_WINDOW flag is set. We can probably do something neater / cleaner, and I have no idea what the performance impact of this might be (since it's in the onLoad, it could affect ts_paint / tpaint).

Still, wanted to get your feedback.
Attachment #8423520 - Flags: feedback?(felipc)
One last thing to emphasize here, if I haven't already done it enough, is that this is still WIP - like, I wouldn't want to land this as-is for the following reasons:

1) The windows that get opened don't have toolbars
2) The user preference to open links in new tabs is currently not being respected

Anyhow, this is where we're at. Have at it.
Comment on attachment 8423517 [details] [diff] [review]
Part 2: Make it possible to pass in an nsITabParent when opening windows, and then stash that nsITabParent in the newly created chrome docshell. r=?

This has an addition to nsXULWindow's GetInterface that turned out not to be necessary. I'll update this and the folded patch.
Attachment #8423517 - Attachment is obsolete: true
Attachment #8423517 - Flags: feedback?(bugs)
Attachment #8423530 - Flags: feedback?(bugs)
Attached patch Parts 1-5 (folded) (obsolete) — Splinter Review
Attachment #8423521 - Attachment is obsolete: true
Comment on attachment 8423520 [details] [diff] [review]
Part 5: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it.

Review of attachment 8423520 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +754,5 @@
>      gMultiProcessBrowser =
>        window.QueryInterface(Ci.nsIInterfaceRequestor)
>        .getInterface(Ci.nsIWebNavigation)
>        .QueryInterface(Ci.nsILoadContext)
> +      .useRemoteTabs || (chromeFlags & Ci.nsIWebBrowserChrome.CHROME_REMOTE_WINDOW);

this looks fine. Good to know that the chromeFlags are already properly set here.
If this was the last thing blocking you, you could even spin this to a separate bug, "nsILoadContext.useRemoteTabs is set too late in the window open process".

@@ +764,5 @@
>        Cc["@mozilla.org/eventlistenerservice;1"]
>          .getService(Ci.nsIEventListenerService)
>          .addSystemEventListener(gBrowser, "click", contentAreaClick, true);
> +    } else {
> +      gBrowser.updateBrowserRemoteness(gBrowser.mCurrentBrowser, "about:blank");

this looks more WIP material :) I imagine that instead of doing this here you'll want to put this somewhere in the path of the "uriToLoad" code.. or even better, make sure that path hits the loadURI that would call updateBrowserRemoteness by itself
Attachment #8423520 - Flags: feedback?(felipc) → feedback+
Comment on attachment 8423515 [details] [diff] [review]
Part 1: Make it possible to stash opener and newly created TabParent's inside DocShell. r=?

In order to prevent leaks I think mOpener should be nsWeakPtr, which
means TabParent should inherit nsSupportsWeakReference, 
and you need to add nsISupportsWeakReference to its QI implementation.


mRemote is quite vague name. Maybe mOpenedRemote or some such?
Attachment #8423515 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8423530 [details] [diff] [review]
Part 2: Make it possible to pass in an nsITabParent when opening windows, and then stash that nsITabParent in the newly created chrome docshell. r=?

> interface nsPIWindowWatcher : nsISupports
> {
>   /** A window has been created. Add it to our list.
>       @param aWindow the window to add
>       @param aChrome the corresponding chrome window. The DOM window
>                      and chrome will be mapped together, and the corresponding
>                      chrome can be retrieved using the (not private)
>@@ -46,31 +47,34 @@ interface nsPIWindowWatcher : nsISupport
>       @param aName window name from JS window.open. can be null.  If a window
>              with this name already exists, the openWindow call may just load
>              aUrl in it (if aUrl is not null) and return it.
>       @param aFeatures window features from JS window.open. can be null.
>       @param aCalledFromScript true if we were called from script.
>       @param aDialog use dialog defaults (see nsIDOMWindow::openDialog)
>       @param aNavigate true if we should navigate the new window to the
>              specified URL.
>+      @param aOpeningTab the nsITabParent that is opening this window. Can be
>+                         null.
>       @param aArgs Window argument
>       @return the new window
> 
>       @note This method may examine the JS context stack for purposes of
>             determining the security context to use for the search for a given
>             window named aName.
>       @note This method should try to set the default charset for the new
>             window to the default charset of the document in the calling window
>             (which is determined based on the JS stack and the value of
>             aParent).  This is not guaranteed, however.
>   */
>   nsIDOMWindow openWindow2(in nsIDOMWindow aParent, in string aUrl,
>                            in string aName, in string aFeatures,
>                            in boolean aCalledFromScript, in boolean aDialog,
>-                           in boolean aNavigate, in nsISupports aArgs);
>+                           in boolean aNavigate, in nsITabParent aOpeningTab,
>+                           in nsISupports aArgs);
Might be good to document what aParent is when aOpeningTab is non-null.

>@@ -1794,17 +1799,19 @@ NS_IMETHODIMP nsXULWindow::CreateNewCont
>     AutoNoJSAPI nojsapi;
>     nsIThread *thread = NS_GetCurrentThread();
>     while (xulWin->IsLocked()) {
>       if (!NS_ProcessNextEvent(thread))
>         break;
>     }
>  }
> 
>-  NS_ENSURE_STATE(xulWin->mPrimaryContentShell);
>+  if (!aOpeningTab) {
>+    NS_ENSURE_STATE(xulWin->mPrimaryContentShell);
>+  }
Perhaps worth to add a comment why we can't check the existence of primary docshell in
case of aOpeningTab is non-null.
And maybe we could check (NS_ENSURE_FOO) that the created chrome docshell has now newly created TabParent set.
Attachment #8423530 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8423518 [details] [diff] [review]
Part 3: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser. r=?


>   nsCOMPtr<nsIDocShellTreeItem> parentAsItem(do_QueryInterface(parentAsWebNav));
>+  nsCOMPtr<nsIDocShell> parentDocShell(do_QueryInterface(parentAsWebNav));
>+  NS_ENSURE_TRUE(parentDocShell, false);
>+
>+  // XXXmconley: is this a sketchy cast?
No

>+  TabParent *openingTab = static_cast<TabParent*>(parentDocShell->GetOpener());
>+  ContentParent* openerContentParent = nullptr;
>+
>+  if (openingTab) {
>+    openerContentParent = openingTab->Manager();
>+  }
So this doesn't deal with the case where we end up having tabs from different processes in one window.
But I guess, this is fine. we should end up using that child process for this window by default.



>+++ b/dom/ipc/ContentParent.cpp
>@@ -750,24 +750,27 @@ ContentParent::RunAfterPreallocatedProce
> {
> #ifdef MOZ_NUWA_PROCESS
>     PreallocatedProcessManager::RunAfterPreallocatedProcessReady(aRequest);
> #endif
> }
> 
> /*static*/ TabParent*
> ContentParent::CreateBrowserOrApp(const TabContext& aContext,
>-                                  Element* aFrameElement)
>+                                  Element* aFrameElement,
>+                                  ContentParent* aOpenerContentParent)
> {
>     if (!sCanLaunchSubprocesses) {
>         return nullptr;
>     }
> 
>     if (aContext.IsBrowserElement() || !aContext.HasOwnApp()) {
>-        if (nsRefPtr<ContentParent> cp = GetNewOrUsed(aContext.IsBrowserElement())) {
>+        nsRefPtr<ContentParent> cp = aOpenerContentParent ? aOpenerContentParent
>+                                                          : GetNewOrUsed(aContext.IsBrowserElement());

I wonder if this will need some tweaking - but that depends on what b2g will want to do here, if anything new.
Attachment #8423518 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8423519 [details] [diff] [review]
Part 4: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=?

> 
>-    // Otherwise, create a new top-level window.
>+    // The browser.link.open_newwindow and browser.link.open_newwindow.restriction
>+    // user preferences might cause us to open the link in the current window
>+    // instead of a new window or tab, which means we can skip communicating with
>+    // TabParent, so we should figure that out first and bail early if that's what
>+    // we're doing.
>+    bool openInCurrent = false;
>+
>+    int32_t containerPref =
>+      Preferences::GetInt("browser.link.open_newwindow",
>+                          nsIBrowserDOMWindow::OPEN_NEWTAB);
>+
>+    if (!aCalledFromJS) {
>+      if (containerPref == nsIBrowserDOMWindow::OPEN_CURRENTWINDOW) {
>+        openInCurrent = true;
>+      }
>+    } else {
>+      // We've been opened via window.open. It's possible that
>+      // browser.link.open_newwindow.restriction will change our window opening
>+      // behaviour here.
>+      if (containerPref == nsIBrowserDOMWindow::OPEN_CURRENTWINDOW) {
>+        int32_t restrictionPref =
>+          Preferences::GetInt("browser.link.open_newwindow.restriction", 2);
>+
>+        if (restrictionPref == 0) {
>+          openInCurrent = true;
>+        }
>+        if (restrictionPref == 2 &&
>+            (aChromeFlags != nsIWebBrowserChrome::CHROME_ALL ||
>+             aPositionSpecified || aSizeSpecified)) {
>+          openInCurrent = true;
>+        }
>+      }
>+    }
Would it be possible to make TabChild and nsContentTreeOwner.cpp to use same code for this all?
Otherwise it is guaranteed that one will be updated at some point without the other getting updated

>-    virtual bool AnswerCreateWindow(PBrowserParent** retval) MOZ_OVERRIDE;
>+    virtual bool AnswerCreateWindow(const uint32_t& aChromeFlags,
>+                                    const bool& aCalledFromJS,
>+                                    const bool& aPositionSpecified,
>+                                    const bool& aSizeSpecified,
>+                                    const nsString& aURI,
>+                                    const nsString& aName,
>+                                    const nsString& aFeatures,
>+                                    bool* windowIsNew,
>+                                    PBrowserParent** retval) MOZ_OVERRIDE;
aWindowIsNew
aRetVal
Attachment #8423519 - Flags: feedback?(bugs) → feedback+
some tests would be good.
Ok, so, I've addressed most of smaug's feedback locally, but some things still remain to do:

1) The windows that get opened don't have toolbars
2) The user preference to open links in new tabs is currently not being respected
3) Abstract out the code that determines once and for all whether or not a new window will be opened, or a new tab, or if a link will be opened in the current tab, etc. That logic is currently duplicated and should be centralized.
4) Write tests
Attached patch WIP Tests (obsolete) — Splinter Review
So here's what I'm thinking for tests - I'm testing the three mechanisms for opening windows from content (window.open with defaults, window.open with non-defaults, target="_blank"), against the two prefs that fiddle with the behaviour: browser.link.open_newwindow and browser.link.open_newwindow.restriction.

Please see the header of the test file I added for a more clear breakdown.

I ended up needing a browser chrome mochitest because I needed to do things like detect when windows were being opened, as well as detecting when we were "opening a new window" in the same browser that had the link in it. I'm open to suggestions though.

I'm not sure if embedding/test/ was the right place to put this. I'm not sure if browser chrome mochitests really belong there, or most places that aren't browser/. I'm also not sure if I really need to guard against Android and b2g in the browser.ini.

All feedback welcome!
Attachment #8429728 - Flags: feedback?(bugs)
Ok, something else I just noticed about my patch - because we're forwarding this call from content through PBrowser, nsWindowWatcher is convinced that the caller is chrome (which, technically it is I guess), even though the message source is content.

That sounds bad, security-wise - and is also allowing content to sidestep the dom.disable_window_open_feature protections. This explains why with my patch, the location bar can be missing in popup windows.
Attachment #8423515 - Attachment is obsolete: true
Attachment #8423530 - Attachment is obsolete: true
Attachment #8423518 - Attachment is obsolete: true
Attachment #8423519 - Attachment is obsolete: true
Attachment #8423520 - Attachment is obsolete: true
Attachment #8423533 - Attachment is obsolete: true
Attachment #8430135 - Attachment is obsolete: true
Attachment #8430336 - Flags: review?(bugs)
Comment on attachment 8430336 [details] [diff] [review]
Part 1: Make it possible to stash opener and newly created TabParent's inside DocShell. r=?

Bah, this isn't as atomic as I had hoped.
Attachment #8430336 - Flags: review?(bugs)
Attachment #8430336 - Attachment is obsolete: true
Comment on attachment 8430341 [details] [diff] [review]
Part 1: Make it possible to stash opener and newly created TabParent's inside DocShell. r=?

This should be more atomic.
Attachment #8430341 - Flags: review?(bugs)
Attachment #8430136 - Attachment is obsolete: true
Attachment #8430351 - Flags: review?(bugs)
Reviews coming hopefully tomorrow.
Comment on attachment 8430341 [details] [diff] [review]
Part 1: Make it possible to stash opener and newly created TabParent's inside DocShell. r=?

> [scriptable, builtinclass, uuid(e46d924d-c20f-4add-8cf5-1e1c817b2181)]
> interface nsIDocShell : nsIDocShellTreeItem
update uuid of nsIDocShell, since you change the interface.


> {
>   /**
>    * Loads a given URI.  This will give priority to loading the requested URI
>@@ -953,9 +954,15 @@ interface nsIDocShell : nsIDocShellTreeI
>    *
>    * Used by the Responsive Design View and B2G Simulator.
>    *
>    * Default is False.
>    * Default value can be overriden with
>    * docshell.device_size_is_page_size pref.
>    */
>   [infallible] attribute boolean deviceSizeIsPageSize;
>+
>+  [noscript,notxpcom,nostdcall] void setOpener(in nsITabParent aOpener);
>+  [noscript,notxpcom,nostdcall] nsITabParent getOpener();
Couldn't this be just
[noscript,notxpcom,nostdcall] attribute nsITabParent opener;



>+
>+  [noscript,notxpcom,nostdcall] void setOpenedRemote(in nsITabParent aOpenedRemote);
>+  [noscript,notxpcom,nostdcall] nsITabParent getOpenedRemote();
Same here
Attachment #8430341 - Flags: review?(bugs) → review+
Attachment #8430351 - Flags: review?(bugs) → review+
Depends on: 1021466
Attachment #8430137 - Attachment is obsolete: true
Attachment #8430341 - Flags: checkin+
Comment on attachment 8430351 [details] [diff] [review]
Part 2: Make it possible to pass in an nsITabParent when opening windows, and then stash that nsITabParent in the newly created chrome docshell. r=?

Parts 1 and 2 were checked in via bug 1021466.
Attachment #8430351 - Flags: checkin+
Comment on attachment 8435918 [details] [diff] [review]
Part 3: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser. r=?

Ok, part 3 is ready for review now.
Attachment #8435918 - Flags: review?(bugs)
Comment on attachment 8435918 [details] [diff] [review]
Part 3: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser. r=?

>-  nsCOMPtr<nsIDocShellTreeItem> parentAsItem(parentWin->GetDocShell());
>+  nsCOMPtr<nsIDocShell> parentDocShell = parentWin->GetDocShell();
>+  if (!parentDocShell) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIDocShellTreeItem> parentAsItem(parentDocShell);
>   if (!parentAsItem) {
>     return false;
>   }
nsIDocShell inherits nsIDocShellTreeItem, so the QI and null check are useless


>       unused << mRemoteBrowser->SendSetUpdateHitRegion(true);
>     }
>+    nsCOMPtr<nsITabParent> remoteTab = static_cast<nsITabParent*>(mRemoteBrowser);
>+    parentDocShell->SetOpenedRemote(remoteTab);
Why can't you just pass mRemoteBrowser to SetOpenedRemote?
Attachment #8435918 - Flags: review?(bugs) → review+
Depends on: 1022412
Attachment #8430138 - Attachment is obsolete: true
Attachment #8430139 - Attachment is obsolete: true
Attachment #8429728 - Attachment is obsolete: true
Attachment #8429728 - Flags: feedback?(bugs)
A try push with my latest patches and my new tests (which now work for e10s as well!):

remote:   https://tbpl.mozilla.org/?tree=Try&rev=75d27eb4aae5
Attachment #8435918 - Attachment is obsolete: true
Comment on attachment 8439358 [details] [diff] [review]
Part 3: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser

This was r+'d by smaug already, just addressing feedback and de-bitrotting.
Attachment #8439358 - Flags: review+
Attachment #8439304 - Attachment is obsolete: true
Comment on attachment 8439366 [details] [diff] [review]
Part 4: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=?

This is the patch that does most of the heavy lifting. I ended up extracting out the window opening rule stuff from nsContentTreeOwner, and putting it into nsWindowWatcher::GetWindowOpenLocation, for easy re-use.

Let me know if you have any questions.
Attachment #8439366 - Flags: review?(bugs)
Comment on attachment 8439305 [details] [diff] [review]
Part 5: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it. try: -b do -p all -u none -t none

Hey felipe,

So just a reminder - the reason why I can't just go and set remote on the initial browser in the tabbrowser.xml constructor is because I need to make sure the browserDOMWindow is set before that occurs. That's why I have to call into updateBrowserRemoteness from onLoad.

I also made updateBrowserRemoteness not care about URLs, and added a new updateBrowserRemotenessByURL for the original callers to use.
Attachment #8439305 - Flags: review?(felipc)
Attachment #8439306 - Attachment is obsolete: true
Comment on attachment 8439366 [details] [diff] [review]
Part 4: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=?

Bah, still failing a test.
Attachment #8439366 - Flags: review?(bugs)
Attachment #8439366 - Attachment is obsolete: true
Try is currently busted, so waiting on that before I push and re-request review on part 4.
Attachment #8439358 - Attachment is obsolete: true
Attachment #8439439 - Attachment is obsolete: true
Attachment #8439305 - Attachment is obsolete: true
Attachment #8439305 - Flags: review?(felipc)
Attachment #8439376 - Attachment is obsolete: true
Attachment #8440164 - Attachment is obsolete: true
Attachment #8440165 - Attachment is obsolete: true
Attachment #8440166 - Attachment is obsolete: true
Attachment #8440345 - Attachment is obsolete: true
Comment on attachment 8440351 [details] [diff] [review]
Part 3: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser

This was already r+'d by smaug a few versions back.
Attachment #8440351 - Flags: review+
Attachment #8440346 - Attachment is obsolete: true
Comment on attachment 8440352 [details] [diff] [review]
Part 4: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=?

Ok, this is the hefty patch of the bunch, and the one that'll likely need the closest inspection.

Along with hooking up the CreateWindow functions between the TabChild and TabParent, this does the baseURI construction on the TabParent side as we had talked about, and moves the calculation of the window opening behaviour out of nsContentTreeOwner into a static method on nsWindowWatcher.

Let me know if you have any questions.
Attachment #8440352 - Flags: review?(bugs)
Comment on attachment 8440352 [details] [diff] [review]
Part 4: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=?

>   if (openingTab) {
>-    openerContentParent = openingTab->Manager();
>+    openerContentParent = static_cast<ContentParent*>(openingTab->Manager());
>   }
I don't understand this change

>+    // XXXmconley: I'm pretty sure we can assume that if content is requesting
>+    // that we open a window from an e10s tab, that we want to enforce that the
>+    // new window is also out-of-process... right?
>+    features.Assign(aFeatures);
>+    features.Append(",remote");
Right. But do we have somewhere checks that JS can't force ",remote" in case of non-e10s web page is opening
a new window? (Not really about this bug though, but could you check and possibly file a new bug)


>+  TabParent* pointer = static_cast<TabParent*>(newRemoteTab.get());
>+  *aRetVal = static_cast<PBrowserParent*>(pointer);
Wouldn't return static_cast<TabParent*>(newRemoteTab.get()); work?




>@@ -433,17 +434,20 @@ nsWindowWatcher::OpenWindowInternal(nsID
>   nsresult                        rv = NS_OK;
>   bool                            nameSpecified,
>                                   featuresSpecified,
>                                   isNewToplevelWindow = false,
>                                   windowIsNew = false,
>                                   windowNeedsName = false,
>                                   windowIsModal = false,
>                                   uriToLoadIsChrome = false,
>-                                  windowIsModalContentDialog = false;
>+                                  windowIsModalContentDialog = false,
>+  // Opening tabs are only ever passed to OpenWindowInternal if we're opening
>+  // a window from a remote tab.
>+                                  openedFromRemoteTab = !!aOpeningTab;
This could use MOZ_ASSERT which check that if we have aOpeningTab, we're in parent process.
Perhaps add similar assertions elsewhere too



>+  // This is because when using child processes, the parent process shouldn't
>+  // know or care about names - unless we're opening named windows from chrome.
>+  if (!aOpeningTab) {
>+    // try to find an extant window with the given name
>+    nsCOMPtr<nsIDOMWindow> foundWindow = SafeGetWindowByName(name, aParent);
>+    GetWindowTreeItem(foundWindow, getter_AddRefs(newDocShellItem));
>+  }




>+/* static */
>+int32_t nsWindowWatcher::GetWindowOpenLocation(nsIDOMWindow *aParent,
>+                                               uint32_t aChromeFlags,
>+                                               bool aCalledFromJS,
>+                                               bool aPositionSpecified,
>+                                               bool aSizeSpecified)
>+{
>+  // If all else fails, we'll default to opening a new tab.
>+  NS_ENSURE_TRUE(aParent, nsIBrowserDOMWindow::OPEN_NEWTAB);
This changes the behavior of nsContentTreeOwner::ProvideWindow.


>+
>+  bool isFullScreen = false;
>+  aParent->GetFullScreen(&isFullScreen);
so only this part should be inside if (aParent) {}


This needs tryserver run, and some tests (with different browser.link.open_newwindow values)
Attachment #8440352 - Flags: review?(bugs) → review-
Comment on attachment 8440167 [details] [diff] [review]
Tests - try: -b do -p all -u all[Ubuntu,-x64] -t other

I used mochitest-browser because I needed to make sure I could detect and control the windows and tabs being opened. If there's a better way of doing this with a plain mochitest, let me know and I'll try to convert this.
Attachment #8440167 - Flags: review?(bugs)
Comment on attachment 8440347 [details] [diff] [review]
Part 5: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it. try: -b do -p all -u none -t none

Hey felipe, please see comment 65 for an explanation for this patch.
Attachment #8440347 - Flags: review?(felipc)
Attachment #8440351 - Attachment is obsolete: true
Attachment #8440352 - Attachment is obsolete: true
Comment on attachment 8442165 [details] [diff] [review]
Part 3: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser

This was already given an r+ from smaug. Part 4 used to undo an error in this patch (the setting of the openingTab->Manager() to openerContentParent, which have incompatible types), which I've since corrected.

I've also updated it to handle the case where the openingTab manager is not in fact a ContentParent.
Attachment #8442165 - Flags: review+
Comment on attachment 8442166 [details] [diff] [review]
Part 4: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=?

(In reply to Olli Pettay [:smaug] from comment #83)
> Comment on attachment 8440352 [details] [diff] [review]
> Part 4: When content in an e10s tab requests a new window be opened, forward
> that call to the TabParent, and have the TabParent pass itself in when
> creating the new window. r=?
> 
> >   if (openingTab) {
> >-    openerContentParent = openingTab->Manager();
> >+    openerContentParent = static_cast<ContentParent*>(openingTab->Manager());
> >   }
> I don't understand this change
> 

Yeah, this was bogus. This was correcting an error that Part 3 had in it. This has been reverted - see my patch for Part 3 to see what I'm doing in here now.

> >+    // XXXmconley: I'm pretty sure we can assume that if content is requesting
> >+    // that we open a window from an e10s tab, that we want to enforce that the
> >+    // new window is also out-of-process... right?
> >+    features.Assign(aFeatures);
> >+    features.Append(",remote");
> Right. But do we have somewhere checks that JS can't force ",remote" in case
> of non-e10s web page is opening
> a new window? (Not really about this bug though, but could you check and
> possibly file a new bug)
> 

In nsWindowWatcher::CalculateChromeFlags, it looks like we ignore "remote" unless the caller is chrome. In the case that you mentioned, isCallerChrome would be false, and aOpenedFromRemoteTab would be false, so it'd be ignored.

> 
> >+  TabParent* pointer = static_cast<TabParent*>(newRemoteTab.get());
> >+  *aRetVal = static_cast<PBrowserParent*>(pointer);
> Wouldn't return static_cast<TabParent*>(newRemoteTab.get()); work?
> 

It did! Thanks.

> 
> 
> 
> >@@ -433,17 +434,20 @@ nsWindowWatcher::OpenWindowInternal(nsID
> >   nsresult                        rv = NS_OK;
> >   bool                            nameSpecified,
> >                                   featuresSpecified,
> >                                   isNewToplevelWindow = false,
> >                                   windowIsNew = false,
> >                                   windowNeedsName = false,
> >                                   windowIsModal = false,
> >                                   uriToLoadIsChrome = false,
> >-                                  windowIsModalContentDialog = false;
> >+                                  windowIsModalContentDialog = false,
> >+  // Opening tabs are only ever passed to OpenWindowInternal if we're opening
> >+  // a window from a remote tab.
> >+                                  openedFromRemoteTab = !!aOpeningTab;
> This could use MOZ_ASSERT which check that if we have aOpeningTab, we're in
> parent process.
> Perhaps add similar assertions elsewhere too
> 

I've added a MOZ_ASSERT_IF, and another further down in the function that ensures that the calculated chrome flags have nsIWebBrowserChrome::CHROME_REMOTE_WINDOW if we've been opened from a remote tab.

> 
> 
> >+/* static */
> >+int32_t nsWindowWatcher::GetWindowOpenLocation(nsIDOMWindow *aParent,
> >+                                               uint32_t aChromeFlags,
> >+                                               bool aCalledFromJS,
> >+                                               bool aPositionSpecified,
> >+                                               bool aSizeSpecified)
> >+{
> >+  // If all else fails, we'll default to opening a new tab.
> >+  NS_ENSURE_TRUE(aParent, nsIBrowserDOMWindow::OPEN_NEWTAB);
> This changes the behavior of nsContentTreeOwner::ProvideWindow.
> 

Good eyes. Removed.

> 
> >+
> >+  bool isFullScreen = false;
> >+  aParent->GetFullScreen(&isFullScreen);
> so only this part should be inside if (aParent) {}
> 

Done.

> 
> This needs tryserver run, and some tests (with different
> browser.link.open_newwindow values)

Ok, I'll push the updated patches to try with my tests in a minute and post the link.
Attachment #8442166 - Flags: review?(bugs)
Try push with latest patches and tests: https://tbpl.mozilla.org/?tree=Try&rev=5f911b48bcd7
Comment on attachment 8440167 [details] [diff] [review]
Tests - try: -b do -p all -u all[Ubuntu,-x64] -t other

Mostly rs+.
Overly Promise-y and yield-y for my eyes :)

(I'm surprised to see we still use Promise.jsm, and not the native Promise object.)
Attachment #8440167 - Flags: review?(bugs) → review+
We should have some tests also for the case when window.open opens a named window foo and then
<a href="somepage.html" target="foo"> is clicked.
Comment on attachment 8440167 [details] [diff] [review]
Tests - try: -b do -p all -u all[Ubuntu,-x64] -t other

>diff --git a/embedding/test/browser.ini b/embedding/test/browser.ini
>new file mode 100644
>--- /dev/null
>+++ b/embedding/test/browser.ini
>@@ -0,0 +1,8 @@
>+[DEFAULT]
>+skip-if = buildapp == 'b2g'
>+
>+[browser_test_new_window_from_content.js]
>+skip-if = toolkit == 'android'
>+support-files =
>+  test_new_window_from_content_child.html
>+  test_new_window_from_content_child.js
>\ No newline at end of file
>diff --git a/embedding/test/browser_test_new_window_from_content.js b/embedding/test/browser_test_new_window_from_content.js
>new file mode 100644
>--- /dev/null
>+++ b/embedding/test/browser_test_new_window_from_content.js

a bit surprising place to put these files.
Put them to dom/tests/browser/
Attachment #8442166 - Flags: review?(bugs) → review+
Comment on attachment 8440347 [details] [diff] [review]
Part 5: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it. try: -b do -p all -u none -t none

Review of attachment 8440347 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +757,5 @@
>      gMultiProcessBrowser =
>        window.QueryInterface(Ci.nsIInterfaceRequestor)
>        .getInterface(Ci.nsIWebNavigation)
>        .QueryInterface(Ci.nsILoadContext)
> +      .useRemoteTabs || (chromeFlags & Ci.nsIWebBrowserChrome.CHROME_REMOTE_WINDOW);

can we drop the loadContext.useRemoteTabs thing entirely and always go by the chromeFlags?  And file a bug saying that .useRemoteTabs is not set early enough for when we need it.

@@ +799,5 @@
>            .XULBrowserWindow = window.XULBrowserWindow;
>      window.QueryInterface(Ci.nsIDOMChromeWindow).browserDOMWindow =
>        new nsBrowserAccess();
>  
> +    gBrowser.updateBrowserRemoteness(gBrowser.mCurrentBrowser, gMultiProcessBrowser);

can you put this inside as an else clause below, just to avoid the call for the common non-e10s case?
Attachment #8440347 - Flags: review?(felipc) → review+
Attachment #8430341 - Attachment is obsolete: true
Attachment #8430351 - Attachment is obsolete: true
Attachment #8440167 - Attachment is obsolete: true
Attachment #8440347 - Attachment is obsolete: true
Attachment #8442165 - Attachment is obsolete: true
Attachment #8442166 - Attachment is obsolete: true
Ok, I've kinda botched the numbering of patches here, especially since the first two patches landed in a separate bug. So I'm going to start over a bit.

The original "Parts 1 and 2" landed in bug 1021466.

I'm going to re-number the remaining patches 1-4 and upload their final versions now.
Attachment #8443619 - Attachment description: Part 1: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser → Part 1: If an opening nsITabParent is found when creating a new content window, use that opener's ContentParent to make the new remote browser. r=smaug.
Attachment #8443619 - Flags: review+
Attachment #8443621 - Attachment description: Part 2: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window → Part 2: When content in an e10s tab requests a new window be opened, forward that call to the TabParent, and have the TabParent pass itself in when creating the new window. r=smaug.
Attachment #8443621 - Flags: review+
Attachment #8443623 - Attachment description: Part 3: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it → Part 3: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW chromeflag set, make sure the initial browser has remote="true" set on it. r=felipe.
Attachment #8443623 - Flags: review+
Attachment #8443625 - Flags: review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/42245901f967
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68ef4eeae5de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2be95d9f6b69
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea31eeb8fa0b


(In reply to :Felipe Gomes from comment #95)
> Comment on attachment 8440347 [details] [diff] [review]
> Part 5: If a newly created browser.xul window has the CHROME_REMOTE_WINDOW
> chromeflag set, make sure the initial browser has remote="true" set on it.
> try: -b do -p all -u none -t none
> 
> Review of attachment 8440347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +757,5 @@
> >      gMultiProcessBrowser =
> >        window.QueryInterface(Ci.nsIInterfaceRequestor)
> >        .getInterface(Ci.nsIWebNavigation)
> >        .QueryInterface(Ci.nsILoadContext)
> > +      .useRemoteTabs || (chromeFlags & Ci.nsIWebBrowserChrome.CHROME_REMOTE_WINDOW);
> 
> can we drop the loadContext.useRemoteTabs thing entirely and always go by
> the chromeFlags?  And file a bug saying that .useRemoteTabs is not set early
> enough for when we need it.
> 

felipe and I talked about this in IRC - it turns out that one of my patches already sets useRemoteTabs early enough in order to render this part of the patch unnecessary, so I've removed it.

(In reply to Olli Pettay [:smaug] from comment #93)
> We should have some tests also for the case when window.open opens a named
> window foo and then
> <a href="somepage.html" target="foo"> is clicked.

I've filed bug 1028338 for that.

Thanks felipe and smaug for the reviews! And thanks especially to smaug for your patience and guidance walking me through this stuff.
Depends on: 1030414
Depends on: 1035197
Depends on: 1116616
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.