Boris Zbarsky [:bzbarsky] Assignee
	Description • 11 years ago

I am _guessing_ this happens due to the timeout set in tabPreviews_handleEvent firing after the linkedBrowser has gone away...

This causes test failures once xpconnect no longer eats those exceptions.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 11 years ago

Unfortunately, looks like we have other callsites that pass in tabs without a linkedBrowser too.

	Dão Gottwald [:dao]
	Comment 3 • 11 years ago

Comment on attachment 801956 [details] [diff] [review]
Don't try to capture previews on tabs without a linked browser.

>--- a/browser/base/content/browser-tabPreviews.js
>+++ b/browser/base/content/browser-tabPreviews.js
>@@ -85,17 +85,18 @@ var tabPreviews = {
>           // The timeout keeps the UI snappy and prevents us from generating thumbnails
>           // for tabs that will be closed. During that timeout, don't generate other
>           // thumbnails in case multiple TabSelect events occur fast in succession.
>           this._pendingUpdate = true;
>           setTimeout(function (self, aTab) {
>             self._pendingUpdate = false;
>             if (aTab.parentNode &&
>                 !aTab.hasAttribute("busy") &&
>-                !aTab.hasAttribute("pending"))
>+                !aTab.hasAttribute("pending") &&
>+                aTab.linkedBrowser)

If linedBrowser is gone, I'd also expect parentNode to be null. Both of these things are done in tabbrowser's _endRemoveTab method. In other words, we should already be protected against this bug...

(In reply to Boris Zbarsky [:bz] from comment #2)
> Unfortunately, looks like we have other callsites that pass in tabs without
> a linkedBrowser too.

Where is that?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 11 years ago

> Where is that?

Ah, good question question.  https://tbpl.mozilla.org/php/getParsedLog.php?id=27632379&tree=Mozilla-Inbound&full=1 (or any BC test run log from a vanilla tree) and has tons of:

    INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
00:54:55     INFO -  [Exception... "'[JavaScript Error: "aTab.linkedBrowser is null" {file: "chrome://browser/content/browser.js" line: 6086}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource:///modules/sessionstore/SessionStore.jsm :: ssi_sendTabRestoredNotification :: line 4058"  data: yes]

ssi_sendTabRestoredNotification sends a "SSTabRestored" event, and in that branch of handleEvent we don't check for a parentNode or a linkedBrowser.  I was looking at builds in which the exception is reported before, which didn't have that full stack....

I'll try doing a parentNode check in the SSTabRestored case, I guess, though it seems odd to have tabs going through there that have a null parentNode.

	Dão Gottwald [:dao]
	Comment 6 • 11 years ago

Comment on attachment 802432 [details] [diff] [review]
This seems to fix things nicely too

The SSTabRestored event listener is added on gBrowser.tabContainer (the tab's parentNode). How can we even receive that event if the tab isn't in the tabContainer anymore?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 11 years ago

Here's a simple example.  browser/components/sessionstore/test/browser_491168.js (one of the tests hitting this) has code like this:

27     tab.addEventListener("SSTabRestored", function() {
...
31       gBrowser.removeTab(tab);

Since this listener is directly on the tab, it fires before the tabbrowser listener.  And it doesn't stopPropagation on the event, so the tabbrowser listener fires afterward, on a now-removed tab.  And that happens because the event target chain in the DOM is built before firing any event listeners.

I guess the other option is to go through all the tests doing this and have them stopPropagation or something.

	Dão Gottwald [:dao]
	Comment 8 • 11 years ago

Ok, so this could theoretically happen in non-test code, but I'm not sure why anyone would do this outside of a test. If we tried to handle this pattern in every event listener, I imagine this could get out of hand. So I'm leaning towards changing the test.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 11 years ago

Test_s_.  Plural. I guess I'll see what I can do...

	Dão Gottwald [:dao]
	Comment 10 • 11 years ago

Yeah, if the number of tests is overwhelming, I'm fine with the current fix as well, with a comment explaining this.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 11 years ago

Looks like it might be just a few tests, just tons of failures.  Try run going.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 11 years ago

This turned out to not be too bad

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/be78e31dbc20

Thanks for the review, and the prodding to get this right!

	Ed Morley [:emorley]
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/be78e31dbc20