914438 - tabPreviews_capture will throw if aTab.linkedBrowser is null

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't try to capture previews on tabs without a linked browser.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't try to capture previews on tabs without a linked browser.

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

Comment 3

[Dão Gottwald [:dao]]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Attachment: This seems to fix things nicely too

Comment 6

[Dão Gottwald [:dao]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Dão Gottwald [:dao]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Dão Gottwald [:dao]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Prevent propagation of SSTabRestored events in tests when we close the relevant tab.

This turned out to not be too bad

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

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

Comment 14

[Ed Morley [:emorley]]

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