Closed Bug 1042680 (e10s-tree-style-tab) Opened 7 years ago Closed 7 years ago

"Tree Style Tab" add-on does not work with e10s

Categories

(Firefox :: Extension Compatibility, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
e10s + ---
firefox33 --- wontfix
firefox34 --- affected

People

(Reporter: SimonSapin, Assigned: billm)

References

()

Details

(Keywords: addon-compat, dogfood, Whiteboard: [workaround:comment 16])

Attachments

(1 file)

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.
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.
In case that’s relevant, I’m using Gnome 3 on x86_64 Archlinux with Intel CPU and graphics.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1042653
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?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Tree style tabs also seems to break session restore for me, along with the bookmarks bar
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");
Attached patch fix-var-handlingSplinter Review
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.
Attachment #8477669 - Flags: review?(bobbyholley)
Depends on: 1057616
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.
> Nick, that is bug 1057616.

Excellent! Thanks very much.
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.
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
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.
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 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?
Attachment #8477669 - Flags: review?(bobbyholley) → review+
Blocks: 1030420
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?
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/3128a4571fee
Assignee: nobody → wmccloskey
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Shouldn't be resolved yet, since the addon has multiple issues -- see Comment #13 and #15
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The various patches here should really be landing in sub-bugs...
Whiteboard: [workaround:comment 16]
No longer blocks: 1030420
Adding "dogfood" keyword so this bug shows up on the e10s wiki's list of known issues:

https://wiki.mozilla.org/Electrolysis#Known_Issues
Keywords: dogfood
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Bug 1098688 is another e10s + TST problem.
Adding "e10s-tree-style-tab" bug alias to use this bug as a meta bug for Tree Style Tab add-on issues.
Alias: e10s-tree-style-tab
>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.)
(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.
You need to log in before you can comment on or make changes to this bug.