Closed Bug 1133077 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: billm, Assigned: mconley)

References

Details

Attachments

(2 files)

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.
Flags: needinfo?(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
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.
Flags: needinfo?(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
Attachment #8565986 - Flags: review?(wmccloskey)
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?
(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 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.
Attachment #8565986 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/2cf0e5bc1473
Assignee: nobody → mconley
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.