Closed Bug 1576492 Opened 5 years ago Closed 5 years ago

this.tabmail is null error with browser.windows.getCurrent()

Categories

(Thunderbird :: Add-Ons: Extensions API, defect)

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6869+ fixed, thunderbird70 fixed, thunderbird71 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 71.0
Tracking Flags:
Tracking Status
thunderbird_esr68 69+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: alta88, Assigned: darktrojan)

Details

Attachments

(1 file, 1 obsolete file)

1576492-windows-incomplete-2.diff
(deleted), patch
mkmelin
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review

background.js contains:

function getCurrentWindow() {
  return browser.windows.getCurrent();
}
getCurrentWindow().then((currentWindow) => {
  console.dir(currentWindow);
});

and the result is:

11:06:37.927 this.tabmail is null ext-mail.js:967
    get activeTab chrome://messenger/content/parent/ext-mail.js:967
    get title chrome://messenger/content/parent/ext-mail.js:824
    convert chrome://extensions/content/parent/ext-tabs-base.js:915
    convert chrome://extensions/content/parent/ext-tabs-base.js:2053
    getCurrent chrome://messenger/content/parent/ext-windows.js:70
    getCurrent self-hosted:1019
    result resource://gre/modules/ExtensionParent.jsm:1160
    withPendingBrowser resource://gre/modules/ExtensionParent.jsm:771
    result resource://gre/modules/ExtensionParent.jsm:1160
    callAndLog resource://gre/modules/ExtensionParent.jsm:1119
    call resource://gre/modules/ExtensionParent.jsm:1159
    InterpretGeneratorResume self-hosted:1300
    AsyncFunctionNext self-hosted:839
Flags: needinfo?(geoff)

Looks like we're in a state of "extension ready, window not ready". Somehow Firefox avoids this happening.

I think the right thing to do is return null, or we could throw an error.

I'm working on some other window API stuff, so I'll look at this again once that's done.

Attached patch 1576492-windows-incomplete-1.diff (obsolete) (deleted) — — Splinter Review

Yes, I think we should return null if the window isn't completely loaded.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Flags: needinfo?(geoff)
Attachment #9090271 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9090271 [details] [diff] [review] 1576492-windows-incomplete-1.diff Review of attachment 9090271 [details] [diff] [review]: ----------------------------------------------------------------- It would be nice if we could just wait for it to be ready and return it then. And do you want to return null, and not a Promise resolving null? I suppose the api handles that wrapping normally?

I hadn't considered waiting, but I suppose it is acceptable, as I can't see any use case where returning immediately with null is better than returning soon with a result.

And yeah, the WE magic code handles just returning the result instead of a Promise.

Do you like this one better?

Attachment #9090271 - Attachment is obsolete: true
Attachment #9090271 - Flags: review?(mkmelin+mozilla)
Attachment #9090329 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9090329 [details] [diff] [review] 1576492-windows-incomplete-2.diff Review of attachment 9090329 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thx! r=mkmelin
Attachment #9090329 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d7405123c5b2
Wait for windows to load before returning from windows.getCurrent and .getLastFocused. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 9090329 [details] [diff] [review] 1576492-windows-incomplete-2.diff Do we care about 68 ESR uplift? I guess it's OK to ship in beta now.
Attachment #9090329 - Flags: approval-comm-esr68?
Attachment #9090329 - Flags: approval-comm-beta+
Target Milestone: --- → Thunderbird 71.0

It's important the api resolves for the load otherwise it would be useless. And background.js runs very early at startup, and also when there is an install/undo disable or remove, which made for the extension having to figure these things out. Uplift to 68 would be great.

Will be pushed to beta today, then TB 68.x with x > 1.

Attachment #9090329 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: