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.

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

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?

> 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.

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?

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.

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.

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

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

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

This turned out to not be too bad

https://hg.mozilla.org/integration/mozilla-inbound/rev/be78e31dbc20 Thanks for the review, and the prodding to get this right!