1634240 - can't access property "isAdoptingTab", this.window.gBrowserInit is undefined

Status: NEW

(WebExtensions :: General, defect, P5)

Description

[kernp25]

Attachment: firefox_5ShyKY8KuU.png

https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/browser/components/extensions/parent/ext-browser.js#1130

I'm seeing this error every time i start Nightly.
My version: 77.0a1 (2020-04-28) (64-Bit)

Comment 1

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

uhm... it may be a race, gBrowserInit should be defined pretty early in the new windows creation.

On which platform are you able to trigger this?
also, would be possible for you to put together an STR to reproduce this consistently starting from a new profile?
(based on the logs I suspect the extension may be ublock, because I see a vapi-background.js file in the stacktrace, and the condition seems to be related to browser.tabs.query called from a browser.windows API event listener)

The fix may be as simple as changing that line into something like if (this.window.gBrowserInit?.isAdoptingTab()) {...}, but I would like to understand how to trigger it in case there is some other wrong assumption that we may need to take care of (and being able to reproduce it consistently may allow us to also bisect the commit that introduced a change that may be related to that).

Thanks for reporting this issue (and for the help on investigating and triaging it properly).

Comment 2

[kernp25]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #1)

On which platform are you able to trigger this?

Windows 10.

also, would be possible for you to put together an STR to reproduce this consistently starting from a new profile?
(based on the logs I suspect the extension may be ublock, because I see a vapi-background.js file in the stacktrace, and the condition seems to be related to browser.tabs.query called from a browser.windows API event listener)

Yes! It has something to do with uBlock Origin.

STR:

1. Install https://addons.mozilla.org/firefox/addon/ublock-origin/
2. Restart Firefox
3. Open browser console

AR:
In the browser console there is this error: can't access property "isAdoptingTab", this.window.gBrowserInit is undefined

The error does only show, if you have uBlock Origin installed and enabled.

Comment 3

[kernp25]

Can you reproduce the bug?

Comment 4

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi,

I am not able to replicate the issue following the STR mentioned in comment #2 . I've tried on: windows 10 pro

On the following versions:
Firefox Nightly version 78.0a1 (2020-05-06) (64-bit)
Beta 77.0b2 (64-bit)
Release 76.0 (64-bit)

Best regards, Clara.

Comment 5

[kernp25]

Full STR:

1. Open Firefox Nightly
2. Install https://addons.mozilla.org/firefox/addon/ublock-origin/
3. Restart Firefox
4. Open browser console

Comment 6

[kernp25]

Attachment: ICSAtZ75pd.mp4

Comment 7

[kernp25]

Doing Cmd/Ctrl-Alt-R to restart the browser seems not to trigger the error.

You need to close the browser first and then start it again manually.

Comment 8

[Alex Cornestean]

Hello,

I have managed to reproduce the issue based on the STR from Comment 6 on the latest Nightly (78.0a1/20200511214706) and possibly on the latets Beta (77.0b4/20200511090718) and Release (76.0.1/20200507114007) under Windows 10 Pro 64-bit and macOS Catalina 10.15.

Please note that I can reproduce the issue only once per fresh profile. Any subsequent restarts will not trigger the error.

I’ve specified that I MIGHT have reproduced the issue on Beta and Release because even though I get an error in the console after doing the STR, it’s not the same as the one I get in Nightly. See the attached screenshots for more details.

For the moment I will set the issue as affecting only Nightly and if the error obtained in Beta and Release turn out to be the same one, I will proceed to make the proper changes.

I have also performed a regression range and narrowed it down to https://bugzilla.mozilla.org/show_bug.cgi?id=1626575 (Bug 1626575 - Remove Loader.jsm exceptions from Android allowed-dupes.mn. r=ntim) with the following differential revision: https://phabricator.services.mozilla.com/D69248 .

The corresponding pushlog is: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=011ad9ff26088e341b0aebecd4424523b7759e88&tochange=5bea15a5406508db54281cf72f4c3511c6e88f11 .

If the regression range is by any chance incorrect, please let me know and I will do it again.

Comment 9

[Alex Cornestean]

Attachment: Error Nightly.png

Comment 10

[Alex Cornestean]

Attachment: Error Beta.png

Comment 11

[Alex Cornestean]

Attachment: Error Release.png

Comment 12

[kernp25]

I also have seen this error on Nightly, after i installed an update (after the browser restarted).

Comment 13

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

Hi Alex,
I took a look to the push log linked in comment 8, but none of those patches looks something that may be able to introduce this error,
what good and bad releases have you used in mozregression? would you mind to give another try to bisect it? Thanks!

About the different stack traces in beta and release, they looks related to the same error (the differences are likely related to other changes in Nightly, one of the differences in nightly that are likely affecting the stack trace is the "javascript.options.asyncstack" pref,that is set to true by default on nightly and devedition but not on release and beta), and so 77 and 76 are also likely affected as well.

Comment 14

[Alex Cornestean]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #13)

Hi Alex,
I took a look to the push log linked in comment 8, but none of those patches looks something that may be able to introduce this error,
what good and bad releases have you used in mozregression? would you mind to give another try to bisect it? Thanks!

About the different stack traces in beta and release, they looks related to the same error (the differences are likely related to other changes in Nightly, one of the differences in nightly that are likely affecting the stack trace is the "javascript.options.asyncstack" pref,that is set to true by default on nightly and devedition but not on release and beta), and so 77 and 76 are also likely affected as well.

Hi Luca,

Sure, I'll give it another try with bisecting the issue and based on your comment I'll also set 76 and 77 as affected. Thank you !

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 17

[Pascal Chevrel:pascalc]

unassigned carry-over with no priority/severity, too late for an uplift in 77.

Comment 18

[Shane Caraveo (:mixedpuppy)]

The traceback from comment 9 points to a onFocusChanged listener. In uBlock, there is one which calls tabs.query for the activetab, and that is where the failure to get gBrowserInit happens. gBrowserInit is defined as a var on browser.js, so I am not sure how it would be undefined on a window.

The first thing I notice is that the only place onFocusChanged happens is in ublock platform/chromium code, I'm uncertain how the platform files interact on various platforms.

https://github.com/gorhill/uBlock/blob/7f999b759fe540e457e297363f55b25d9860dd3e/platform/chromium/vapi-background.js#L299

See if Raymond has any thoughts on this.

Comment 19

[[e-mail]]

This is now far in the past so I can't be sure anymore (I should have commented in the code), but I think I needed to add a window.onFocusChanged listener because the tabs.onActivated listener was not being called in certain scenarios, leading to the following issue: https://github.com/uBlockOrigin/uBlock-issues/issues/151.

happens is in ublock platform/chromium code

The chromium/ part is just a historical artifact, I meant to rename this to webext/ since forever now.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 23

[:Gijs (he/him)]

Can we work around this by having the relevant API in ext-browser check if the window is in BrowserWindowTracker.windows, and if not, wait for it to be (probably using registerOpeningWindow or similar) before trying to figure out the active tab there?

Comment 24

[Rob Wu [:robwu]]

uBlock Origin calls tabs.query({ active: true, windowId }) from a windows.onFocusChanged listener. This was already the case four years ago (comment 18), and is still happening now (stack trace from bug 1891520, same as this bug). The logic exists to make sure that when a browser window is focused, and uBlock Origin has not detected its active tab before, that it identifies the active tab (uBlock Origins' vapi-background.js source).

Although the error is raised from get activeTab, the issue is not there. The logic at that point expects the input to be an initialized browser window. The list of windows is from a call to windowManager.query in tabManager.query's implementation. This windowManager is specific to the extension framework and independent of BrowserWindowTracker.

windowManager's query implementation looks up the windows at: https://searchfox.org/mozilla-central/rev/b220e40ff2ee3d10ce68e07d8a8a577d5558e2a2/toolkit/components/extensions/parent/ext-tabs-base.js#2252-2254,2259-2261,2266

There are three branches:

1. When windowId is set, getWindow is used, which may return browser windows that are still loading (and even non-browser windows that are still loading).
2. When lastFocusedWindow:true is used, in which case getTopWindow is used, which is Services.wm.getMostRecentWindow("navigator:browser"). In theory this window could still be initializing.
3. In all other cases, windowManager.getAll() is called, which excludes incomplete windows by default (source).

uBlock Origin is affected by the first case. I imagine that it is theoretically possible for a newly opened window to become focused and/or lose focus, which triggers this logic.

I see the following ways to fix this bug (all of them make sense, any of them will get rid of the error):

• Modify getWindow to NOT return incomplete windows, at least not by default. This is not super trivial because all callers have to be audited, to check whether they should expect an incomplete window or not.
  • A weaker version of this is to patch the specific use in windowManager.query() method only (instead of all), to ignore windows that are incomplete.
• Modify windows.getLastFocused's implementation to delay the dispatch of the the event when the new window has fully been initialized if it has not yet. The initialization of the browser window should be relatively quick (hopefully!), so the impact of waiting a little bit should be relatively minor.
  • This fixes the issue by ensuring that the extension is only notified of the browser window when it has fully been loaded, so that when the extension calls tabs.query(), that the error cannot be triggered (and something useful is returned instead).
  • Even if the browser window itself is initialized quickly, I imagine that the result of tabs.query() is likely going to be an empty array in the scenarios where this bug would be trigger, because populating the tabs takes longer than just the readiness of the browser window.

Since the analysis of this bug takes more time than writing a patch, I'll attach a patch so that this bug can be closed soon.

Comment 25

[Rob Wu [:robwu]]

Attachment: Bug 1634240 - Stop exposing non-browser windows in windows.onFocusChanged

The extension APIs cannot do anything meaningful with windowId for
non-browser windows. Stop exposing them in windows.onFocusChanged, which
is a known way to leak non-browser windowIds.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b56107c9c8f
Stop exposing non-browser windows in windows.onFocusChanged r=zombie

Comment 27

[Norisz Fay [:noriszfay]]

Backed out for causing bc failures on browser_ext_windows_onFocusChanged.js

Backout link

Push with failures

Failure log

Comment 28

[Aaron Train [:aaronmt]]

Hello,

Prior to back-out, (2 back-outs at the same time: either this or https://bugzilla.mozilla.org/show_bug.cgi?id=1891486) is suspected to have broken all Android UI tests that attempt to load either legitimate URLs and PDF files mainly. On video review, the browser(s) stall (progress bar hang) on attempting to load a URL and PDFs mainly.

Focus

• Matrix: link

Fenix

• Matrix: link

STR

• Download and install https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/GpR3X2N9SsO6E1YqN1u1Xw/artifacts/public/build/target.arm64-v8a.apk on an android ARM64-v8a device/emulator

• Attempt to load mozilla.org / google.com

Expected:

• Site loads

Actual:

• Stalls loading with the progress bar hung

Comment 29

[Rob Wu [:robwu]]

The patch here only touches extension code, specifically the windows.onFocusChanged implementation. This is a desktop-only feature that cannot have any impact on Android builds.

I'm surprised at the test failures, and have created a try push with extra logging to see if it provides any pointers.