1654243 - WindowTrackerBase's topNonPBWindow may throw NS_ERROR_FAILURE on macOS when no window is available

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

:Gijs pointed us this logged error in Bug 1647059 comment 5 (see BrowserConsole screenshot from Bug 1647059 comment 4).

The exception is raised by Services.wm.getMostRecentNonPBWindow when there is no browser window open, which can happen on macOS when all browser window have been closed (also confirmed by trying to call it manually from the BrowserToolbox after I closed all browser windows for that Firefox instance).

From a quick look to nsWindowMediator.cpp I guess it does throw that error from here.

On the contrary Services.wm.getMostRecentWindow does just return null in that case.

This error is unrelated to Bug 1647059, but it seems to still be a legit bug from a WebExtensions API perspective, e.g. uBlock does trigger this error by calling browser.tabs.query when the window has been closed and so the API call fails with the generic "An unexpected error occurred" error instead of just returning an empty list of tabs as result of the tabs.query API call.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

I'm not sure if there is an actual reason why these two similar methods are behaving differently in that condition.
An easy way to fix it would be to try...catch in ext-tabs-base.js the call to Services.wm.getMostRecentNonPBWindow, another one would be to double-check if there is any reason to not just make Services.wm.getMostRecentNonPBWindow to return null in that case as Services.wm.getMostRecentWindow does.

Comment 2

[Rob Wu [:robwu]]

Relevant code on desktop:
https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/toolkit/components/extensions/parent/ext-tabs-base.js#1519-1520

On mobile the property can also be null:
https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/mobile/android/components/extensions/ext-utils.js#151-152
https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/mobile/android/modules/geckoview/GeckoViewWebExtension.jsm#513,517

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Rob Wu [:robwu] from comment #2)

On mobile the property can also be null:
https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/mobile/android/components/extensions/ext-utils.js#151-152
https://searchfox.org/mozilla-central/rev/227f22acef5c4865503bde9f835452bf38332c8e/mobile/android/modules/geckoview/GeckoViewWebExtension.jsm#513,517

Well, the issue is not that topNonPBWindow may returns null, it is that in ext-tabs-base.js we don't expect Services.wm.getMostRecentNonPBWindow to throw when there is no window around.

But I see that on mobile we are not calling Service.wm.getMostRecentNonPBWindow, and so this should be a desktop-only issue.

Thanks Rob for finding and linking here the mobile code, that was really helpful!

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Olli,
do you remember if there are specific reasons for Services.wm.getMostRecentNonPBWindow to throw an NS_ERROR_FAILURE error if it is called when there is no window open?
(it looks that we introduced this method long time ago in Firefox 45, Bug 1221992, and this has always been the behavior of this function, on the contrary Services.wm.getMostRecentWindow seems to be just returning null in that case).

I'm mainly interested to assess if we should fix this issue on the WebExtensions framework side (by wrapping the call to Services.wm.getMostRecentNonPBWindow into a try...catch), or if a more reasonable fix may be to make getMostRecentNonPBWindow to return null instead of throwing an error under this condition (as getMostRecentWindow seems to be already doing).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Returning null should be fine, but need to audit all the callers.
Luckily there aren't many.

Comment 6

[Dão Gottwald [:dao]]

Can you just use BrowserWindowTracker.getTopWindow instead of Services.wm.getMostRecent[NonPB]Window?

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Dão Gottwald [::dao] from comment #6)

Can you just use BrowserWindowTracker.getTopWindow instead of Services.wm.getMostRecent[NonPB]Window?

I just took a loot and yes! It seems that we could, or even better it seems that we should:

The WindowTracker class which we already define at browser/ level, which inherits the WindowTrackerBase defined at toolkit level, is already using BrowserWindowTracker in the definition of getTopNormalWindow](https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-browser.js#320-326).

We should just redefine also the topWindow and topNonPBWindow getters and use BrowserWindowTracker for both.

Thanks a lot for bringing it up!