897794 - "this.previewFromTab(...) is undefined" when closing last tab shows about:newtab

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jesse Ruderman]

This seems to be Windows-only. 

1. Set user_pref("browser.tabs.closeWindowWithLastTab", false);
2. Press Ctrl+W to "close" your last tab (actually loads about:newtab)
3. Check for JS errors

[19:02:13.515] this.previewFromTab(...) is undefined @ resource://app/modules/WindowsPreviewPerTab.jsm:540

[Exception... "'[JavaScript Error: "this.previewFromTab(...) is undefined" {file: "resource://app/modules/WindowsPreviewPerTab.jsm" line: 540}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/tabbrowser.xml :: updateCurrentBrowser :: line 1062"  data: yes]

Regression from bug 870100?  (My fuzzer started reporting this last night, and those patches made major changes to tab previews last night.)

Dao once fixed something similar in bug 563337.

Comment 1

[:Gijs (he/him)]

I'm seeing this happening in mochitest-browser CustomizableUI tests (Australis) as well. What seems to happen in the case Jesse outlined in comment #0 (this was easier to attach the JS debugger to) is this:

1. We create the new tab in position 1 (the single closing tab having position 0), and this gets a preview
2. We remove the old tab. The new tab is now in position 0, and is the only tab in the list.
3. We blur the closing tab, causing the newly created tab to be selected. However, this tab still has position 1, not position 0. Because we already updated the list of previews, and it doesn't have item 1, the code breaks.
4. We only update the tab's positional index properties (._tPos) after we've blurred the closing tab.

I'm not entirely sure what the best way of fixing this would be. Maybe we should watch for closing tabs in another way than we do now and/or have a separate event for it? This is essentially a race condition; given that normally, there's animation involved in opening a new tab, if you created a new tab inbetween closing one and the closing animation being finished, perhaps you could trigger the same case? That would be pretty bad...

Tim, can you (or someone else who knows this code still better) sanity-check my diagnosis? :-)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

WindowsPreviewPerTab is another part of the code base that does thumbnails, yay. I unfortunately have no knowledge of this code. Dão might be a better fit.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Gijs is right, as TabClose is dispatched before _tPos changes we'll get inconsistencies between tabbrowser.tabs and the .previews array managed in AeroPeek's TabWindow. I have an idea how to solve this, will post something soon.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Use a WeakMap to manage AeroPeek previews

If we use a WeakMap to store previews for tabs we don't actually need to fiddle around with _tPos. If we iterate over .previews we can also just iterate over the tabbrowser's list of tabs.

This seems to pass browser/modules/test/ and browser/components/tabview/test/. I don't see the error anymore, which occurred very frequently when running tabview tests.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 809421 [details] [diff] [review]
Use a WeakMap to manage AeroPeek previews

Do you need a WeakMap or would a Map work as well? Maps are iterable, so you wouldn't need to iterate over tabs and exclude closing tabs all the time.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

I started with a WeakMap because it felt like the right thing when using tabs as keys but in hindsight using a Map would indeed make things a little easier. We listen for the TabClose event anyway so there's no risk of leaking anything. Will update the patch.

Comment 8

[Carsten Book [:Tomcat]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28335970&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28336004&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28336021&tree=Mozilla-Inbound

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Use a Map to manage AeroPeek previews

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/2aa142b4b128

Comment 11

[Carsten Book [:Tomcat]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28344688&tree=Mozilla-Inbound

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2aa142b4b128