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

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jruderman, Assigned: ttaubert)

Tracking

(Blocks 1 bug, {regression})

Trunk
Firefox 27
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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? :-)
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert) → needinfo?(dao)
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.
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #809421 - Flags: review?(dao)
Flags: needinfo?(dao)
Duplicate of this bug: 919998
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.
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.
Attachment #809421 - Attachment is obsolete: true
Attachment #809421 - Flags: review?(dao)
Attachment #809788 - Flags: review?(dao)
Attachment #809788 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/2aa142b4b128
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.