1133077 - [e10s] window.open broken with vertical tabs add-on

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

STR:
1. Install Vertical Tabs add-on: https://github.com/darrinhenein/VerticalTabs/blob/master/VerticalTabs.xpi
2. Visit a page that uses window.open. I visit https://crash-stats.mozilla.org and click the "Sign In" button at the bottom of the page.

Actual result: Content process crash.
Expected result: Window opens.

I used mozregression to find this regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a793dd926e90&tochange=ed3e996b3805

It looks like RecvCreateWindow is returning false. I've tracked that down to here:
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp?rev=73e42f528ee6#1804
For some reason, mPrimaryContentShell is null.

Mike, do you have any idea why this might be happening? I realize not a lot people are likely to have this add-on installed, but it seems like something that might affect more than just this add-on.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

If mPrimaryContentWindow is null, that means there is no non-remote browser in the newly opened window. That's very strange, because we removed the code that caused the initial browser to _be_ remote, so the initial browser should default to being non-remote.

RecvCreateWindow is then able to force that initial browser to become remote after it has finished loading, and pass that back to the caller[1].

Let me take a poke at this.

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#689

Comment 2

[Mike Conley (:mconley) (:⚙️)]

So this is a sad tale, but I think I have a fix.

The first thing to know is that VerticalTabs rearranges the XUL of every window that opens - including pop up windows.

When it does this, it removes the tabbrowser-tabs element from the DOM, and re-inserts it elsewhere, causing the tabs XBL binding to be re-attached.

When the tabs binding is reattached, it re-runs the constructor, and runs this code:

if (this.tabbox && this.tabbox.hasAttribute("selectedIndex")) {
  let selectedIndex = parseInt(this.tabbox.getAttribute("selectedIndex"));
  this.selectedIndex = selectedIndex > 0 ? selectedIndex : 0;
  return;
}
 
Setting the selectedIndex also sets the selectedIndex on our special async tabbrowser-tabpanels binding that was added in bug 1009628. Unfortunately, our tabbrowser-tabpanels binding has a bug in it, where if the requested selectedIndex matches the current selectedIndex, we still do all of the work to remove the content-type="primary" attribute from the browser tab, fire down the MozAfterRemotePaint message, wait for the message to return, and then set content-type="primary" back.

The problem is, however, that this VerticalTabs XUL re-arrangement happens on the window load event, and once the load events are handled (and we're still waiting for the content process to tell us that the selected tab is ready to paint), we're left in this half-switched state where there is no browser with type="content-primary", thus, no mPrimaryContentShell, and thus, we fail the assertion, and we crash.

I think the easy fix is to make it so that our async binding will not attempt to switch the tabs if the requested index is the same as the current index.

Long term, this would also be fixed once gw280 moves us to async tabbrowser-tabs instead of the async tabbrowser-tabpanels.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: tabbrowser-tabpanels should not attempt to switch tabs if the requested index matches the current index. r=?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8565986 [details] [diff] [review]
tabbrowser-tabpanels should not attempt to switch tabs if the requested index matches the current index. r=?

How do you feel reviewing this, billm?

Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=804f18abea5d

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks for looking into this Mike.

I still don't quite understand everything though. If the tabs are the same, then I would expect shouldWait to be false here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3405
So _prepare should return a resolved promise. Consequently, _finalizeTabSwitch should run immediately and set content-primary for the new tab:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3494
Which of these inferences is wrong?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bill McCloskey (:billm) from comment #5)
> Thanks for looking into this Mike.
> 
> I still don't quite understand everything though. If the tabs are the same,
> then I would expect shouldWait to be false here:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3405
> So _prepare should return a resolved promise. Consequently,
> _finalizeTabSwitch should run immediately and set content-primary for the
> new tab:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#3494
> Which of these inferences is wrong?

None are wrong - the problem is that in finalizeTabSwitch, the toTab equals the fromTab, so in the last lines of _finalizeTabSwitch, we set the type of the tab to content-targetable, which clears out the mPrimaryContentShell.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8565986 [details] [diff] [review]
tabbrowser-tabpanels should not attempt to switch tabs if the requested index matches the current index. r=?

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

Oh, right. Thanks again.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

No problem - thanks for the quick review!

remote:   https://hg.mozilla.org/integration/fx-team/rev/2cf0e5bc1473

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2cf0e5bc1473

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1133077.html