:shell escalante Reporter
	Description • 7 years ago

OS: Mac
Regression window: worked 4/24, broke 4/25
Issue: Tree Style Tabs breaks when opening links in new tabs 

Andreas took a very quick glance at the code and the developer handles some e10s stuff around where the exception occurs

	:shell escalante Reporter
	Comment 1 • 7 years ago

Hi Bob,

I asked in a general meeting if anyone knew of something that might have landed really recently and impacted links (so fishing).  Jimm mentioned you were doing ongoing work on file content process handling for sandboxing edge cases.  And potentially that might impact this area.

Did you land anything that would have shown up in todays nightly that could be related?

Andreas is in Germany - theone on IRC for more details

	Andreas Wagner [:TheOne] [use NI]
	Comment 2 • 7 years ago

So, the reproduce this, just install the latest Nightly along with the Tree Style Tabs add-on and then click on a link in an email on gmail.

The error I see in the browser console is:
E10SUtils.jsm:117:1: TypeError: aURI is null

Oddly, setting a breakpoint at that line using the browser debugger does not ever show `aURI` being null-ish.

	Andreas Wagner [:TheOne] [use NI]
	Comment 3 • 7 years ago

Looks like devtools links to the wrong file/line.

The error appears inside the add-on: chrome://treestyletab/content/windowHelper.js#42

The relevant code is:

nsBrowserAccess.prototype.__treestyletab__openURIInFrame = nsBrowserAccess.prototype.openURIInFrame;
nsBrowserAccess.prototype.openURIInFrame = function(aURI, aParams, aWhere, aContext) {
    if (aWhere === Ci.nsIBrowserDOMWindow.OPEN_NEWTAB)
        TreeStyleTabService.onBeforeBrowserAccessOpenURI(aParams, aWhere, aContext);
    return nsBrowserAccess.prototype.__treestyletab__openURIInFrame.call(this, aURI, aParams, aWhere, aContext);
};

(line 42 is the return statement)

I assume `openURIInFrame` changed?

	Andreas Wagner [:TheOne] [use NI]
	Comment 4 • 7 years ago

Wild guess: Bug 902695 touched `openURIInFrame` on April 24.

Bill, could the patch in that bug break add-ons using this method?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 5 • 7 years ago

That bug landed four years ago, so I doubt it :-). Did you mean a different bug?

In any case, this sounds like a perfect opportunity to use mozregression. It can find exactly which change broke Tree Style Tabs.

	Andreas Wagner [:TheOne] [use NI]
	Comment 6 • 7 years ago

Hm, yeah, not sure what happened there. What I meant was bug 1356922.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 7 • 7 years ago

Oh yeah, we added a new argument to that function. Maybe they just need to do that too and pass it along.

	YUKI "Piro" Hiroshi
	Comment 8 • 7 years ago

Thank you for the mention! This has been fixed by TST's this commit:
https://github.com/piroor/treestyletab/commit/c7009f5c878bfc04e056f91db2d5236d223e17ae

	Bob Owen (:bobowen)
	Comment 9 • 7 years ago

(In reply to :shell escalante from comment #1)
> Hi Bob,
> 
> I asked in a general meeting if anyone knew of something that might have
> landed really recently and impacted links (so fishing).  Jimm mentioned you
> were doing ongoing work on file content process handling for sandboxing edge
> cases.  And potentially that might impact this area.

Hi shell, I have been changing things in this sort of area recently, but not in the last few days.
Anyway, looks like we found the change.

I'll be making some more changes soon, but nothing that I think will break addons. (/me crosses fingers)

	(no longer active)
	Comment 10 • 7 years ago

(In reply to YUKI "Piro" Hiroshi from comment #8)
> Thank you for the mention! This has been fixed by TST's this commit:
> https://github.com/piroor/treestyletab/commit/
> c7009f5c878bfc04e056f91db2d5236d223e17ae

Thanks a lot for the quick fix!

Any reason why we should keep this bug open now?

	Andreas Wagner [:TheOne] [use NI]
	Comment 11 • 7 years ago

I was waiting for a new version to be released, but it's probably better close this.