1042680 - (e10s-tree-style-tab) "Tree Style Tab" add-on does not work with e10s

Status: VERIFIED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Simon Sapin (:SimonSapin)]

Steps to reproduce:

* Enable e10s (http://arewee10syet.com/)
* Install Tree Style Tab
* Click on a link with the "middle" mouse button

Expected results:

Tabs are on the left, and form a tree where each subtree is indented and collapsible. The new tab should be a "child" of the current tab in the tree.


Actual result, on Nightly 34.0a1 (2014-07-23):

Tabs are on the left, but the new tab gets added at the end of the top-level of the tree, not a child of the current tab.

Comment 1

[Chris Peterson [:cpeterson]]

Thanks, Simon. I'll update list of addons on arewee10syet.com. Interestingly, in bug 1042653, someone reported that Tree Style Tabs crashed on their (Windows) machine.

I'm setting the "tracking-e10s" tracking flag to '?' so the e10s team will review the bug at our next triage meeting.

Comment 2

[Simon Sapin (:SimonSapin)]

In case that’s relevant, I’m using Gnome 3 on x86_64 Archlinux with Intel CPU and graphics.

Comment 4

[Chris Peterson [:cpeterson]]

Jim: this bug is about the tab being added to the wrong end of the list, but bug 1042653 is a crash. Do these two bugs have the same root cause?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Chris Peterson (:cpeterson) from comment #4)
> Jim: this bug is about the tab being added to the wrong end of the list, but
> bug 1042653 is a crash. Do these two bugs have the same root cause?

sounds similar though I'm fine with keeping both open just to be safe.

Comment 6

[Simon Sapin (:SimonSapin)]

In Nigthly 34.0a1 (2014-08-06), the crash described bug 1042653 is now resolved for me, but the problem initially described here still exists. Re-opening.

When middle-clicking on a link, or left-clicking while holding the CTRL keyboard key, the new tab incorrectly goes to the end of the top-level list of tabs instead of becoming a "child" of the current tab.

However, right-clicking and choosing "Open link in a new tab" in the context menu does create a "child" tab as expected. I don’t remember if I had tried this before.

Comment 7

[John Schoenick [:johns]]

Tree style tabs also seems to break session restore for me, along with the bookmarks bar

Comment 8

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

John tracked this down to an issue with compartment_per_addon. Tree style tabs is rewriting browser code, and so the browser code gets pulled into the addon compartment. The browser code is assigning to gBidiUI which is declared as |var gBidiUI| in a XUL overlay. I tracked that down to a problem in the writeToGlobalPrototype code. The xpcshell test case is something like:

var x;
make new sandbox with writeToGlobalPrototype
evalInSandbox("x = 1");

Comment 9

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

Attachment: fix-var-handling

This should fix the problem John was seeing. There's a very simple testcase that's part of the patch. The problem is that the proto global already has a binding |x| which is non-configurable. When we assign to that property from the sandbox, we call the sandbox addprop hook, which tries to copy the property from the sandbox to the proto global. The property on the sandbox global is configurable, so the copy call throws an exception because we're trying to change the property on the proto global from non-configurable to configurable.

So I added a check to detect if the property is already there. If it is, we use JS_SetProperty rather than JS_CopyProperty. However, I also had to add a special case to that to handle our bizzarro const semantics. I added a comment about that.

The problem where control-clicking doesn't open a child tab seems to affect current nightly without e10s.

Comment 10

[Nicholas Nethercote [inactive]]

Hmm... I briefly turned on e10s last week, but turned it off again shortly afterwards because I hit a couple of annoying problems (all of which have bugs filed).

Just today I noticed Tree Style Tabs giving me exactly the behaviour mentioned in comment 0. I still have e10s off. I'm not sure how long I've been getting this behaviour; I'd be surprised if it was the whole time since I turned e10s off. I've restarted Firefox several times since then.

I'm not sure if this data point is helpful or just confusing at the moment, but I thought I'd mention it.

Comment 11

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

Nick, that is bug 1057616.

Comment 12

[Nicholas Nethercote [inactive]]

> Nick, that is bug 1057616.

Excellent! Thanks very much.

Comment 13

[John Schoenick [:johns]]

Yet another issue has appeared in recently nightlies: the 'auto-hide' functionality will never trigger, probably because some mouseover event for content is no longer happening on the parent. I haven't bisected yet.

So the separate issues are:

1) The compartment-per-addon global issue causes the addon to break badly. This has been worked around in upstream addon, and attached patch fixes root cause.

2) Tab nesting no longer works for control-clicking items, because the content-click-handler pathway the addon hooks is different in e10s mode. This probably needs to be fixed upstream.

3) Auto-collapse/hide doesn't work, for some reason.

Comment 14

[John Schoenick [:johns]]

And for issue #3, the bisect:

> Bug 1051017 - Add browser.contentWindowAsCPOW and browser.contentDocumentAsCPOW (r=mconley,mrbkap)
> http://hg.mozilla.org/mozilla-central/rev/d6bdd3272ccb

Searching for contentWindow in TST code that is doing:

> if (this.findPluginArea(this.browser.contentWindow) ...) { ... show/hide }

But the debugger shows contentWindow set but null and oddly no contentWindowAsCPOW. Without bug 1051017 contentWindow is accessible as expected

Comment 15

[John Schoenick [:johns]]

For the autohide issue: this.browser.mCurrentBrowser.contentWindow and this.browser.mCurrentBrowser.contentWindowAsCPOW are both visible, so maybe we're not interposing the addon-only content window in some contexts? I don't understand the browser hierarchy well enough to know why this makes a difference.

Comment 16

[John Schoenick [:johns]]

I created a PR against tree-style-tabs to fix or work around the issues reported so far:
https://github.com/piroor/treestyletab/pull/760

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8477669 [details] [diff] [review]
fix-var-handling

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

r=me with those things addressed.

::: js/xpconnect/src/Sandbox.cpp
@@ +403,5 @@
>      if (!JS_GetPrototype(cx, obj, &proto))
>          return false;
>  
>      // After bug 1015790 is fixed, we should be able to remove this unwrapping.
>      RootedObject unwrappedProto(cx, js::UncheckedUnwrap(proto, /* stopAtOuter = */ false));

Can we remove this now and just use |proto| directly in both the cases below?

::: js/xpconnect/tests/unit/test_writeToGlobalPrototype.js
@@ +1,1 @@
> +var G = 3;

Why are we removing the const test? Shouldn't we be testing both?

Comment 18

[Bobby Holley (:bholley)]

Looks like this fixes some of the orange in bug 1030420. Bill, do you have a moment today to address review comments and get it landed?

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3128a4571fee

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3128a4571fee

Comment 21

[John Schoenick [:johns]]

Shouldn't be resolved yet, since the addon has multiple issues -- see Comment #13 and #15

Comment 22

[Bobby Holley (:bholley)]

The various patches here should really be landing in sub-bugs...

Comment 23

[Chris Peterson [:cpeterson]]

Adding "dogfood" keyword so this bug shows up on the e10s wiki's list of known issues:

https://wiki.mozilla.org/Electrolysis#Known_Issues

Comment 24

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

A new version of TST was released today that incorporates John's changes. I think we should close this. If someone sees any new problems, please report in a new bug.

Comment 25

[Nicholas Nethercote [inactive]]

I can confirm that Tree Style Tab v0.15.2014111300a022851 (which I got from http://piro.sakura.ne.jp/xul/xpi/nightly/) works fine with e10s. Thank you for the fixes.

Comment 26

[Nicholas Nethercote [inactive]]

Bug 1098688 is another e10s + TST problem.

Comment 27

[Chris Peterson [:cpeterson]]

Adding "e10s-tree-style-tab" bug alias to use this bug as a meta bug for Tree Style Tab add-on issues.

Comment 28

[Russell Haley]

>I can confirm that Tree Style Tab v0.15.2014111300a022851 (which I got from http://piro.sakura.ne.jp/xul/xpi/nightly/) works fine with e10s. Thank you for the fixes.

I can disconfirm that.  I still get middle-clicked links appearing at the end of the tree with the current nightly and TST 0.15.20141202something, or whatever the current version is at that link. (None of the places where the version number appears in about:addons can be highlighted and copied, for some reason.)

Comment 29

[Nicholas Nethercote [inactive]]

(In reply to Russell Haley from comment #28)
> >I can confirm that Tree Style Tab v0.15.2014111300a022851 (which I got from http://piro.sakura.ne.jp/xul/xpi/nightly/) works fine with e10s. Thank you for the fixes.
> 
> I can disconfirm that.  I still get middle-clicked links appearing at the
> end of the tree with the current nightly and TST 0.15.20141202something, or
> whatever the current version is at that link.

It got broken again by a different change. See https://github.com/piroor/treestyletab/pull/816, which includes a link to a non-official build that fixes the new problem.