1443221 - chrome.windows.create fails with "chromeWin is null" exception and sometimes "Gah. Your tab just crashed."

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[dw-dev]

Attachment: Tile Tabs WE 7.0.1.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180118215408

Steps to reproduce:

Use Firefox Nightly 60.0a1 (dated before 3 March 2018):

1. Install a modified version (7.0.1) of my Tile Tabs WE add-on (attached).
2. Create two 'about:newtab' tabs and select the first of these tabs.
3. Click on the Tile tabs WE button to create a layout with two tiled windows (each conraining one of the tabs).
4. Right-click on the Tile Tabs WE button and select "Toggle Toolbars".
5. About 50%-75% of the time, this operation will fail with a "chromeWin is null" exception in the console. (see LOG 1 in Comment 1 below)

Use Firefox Nightly 60.0a1 (dated after 4 March 2018):

 - Repeat the above steps 1 to 4.
 - The "chromeWin is null" exception still happens.
 - Sometimes the tabs(s) in one or both tiled windows crash with the message "Gah. Your tab just crashed." (see LOG 2 in Comment 2 below)




Actual results:

 "chromeWin is null" exception is thrown and sometimes tabs crash.


Expected results:

There should be no exception and the tabs should not crash.

Comment 1

[dw-dev]

LOG 1

chromeWin is null                                      PrivateBrowsingUtils.jsm:39
	isBrowserPrivate      resource://gre/modules/PrivateBrowsingUtils.jsm:39:1
	create                      chrome://browser/content/ext-windows.js:127:29
	create                                                  self-hosted:975:17
	call/result<             resource://gre/modules/ExtensionParent.jsm:772:57
	withPendingBrowser       resource://gre/modules/ExtensionParent.jsm:427:26
	next                                                    self-hosted:1211:9
newwin is undefined                                             background.js:2834
Unchecked lastError value: Error: An unexpected error occurred  background.js:2829

Comment 2

[dw-dev]

LOG 2

chromeWin is null                                          PrivateBrowsingUtils.jsm:39
	isBrowserPrivate          resource://gre/modules/PrivateBrowsingUtils.jsm:39:1
	create                          chrome://browser/content/ext-windows.js:127:29
	create                                                      self-hosted:975:17
	call/result<                 resource://gre/modules/ExtensionParent.jsm:772:57
	withPendingBrowser           resource://gre/modules/ExtensionParent.jsm:427:26
	next                                                        self-hosted:1211:9
newwin is undefined                                                 background.js:2944
Unchecked lastError value: Error: An unexpected error occurred      background.js:2939
TypeError: this.tabbrowser is null[Learn More]                    tabbrowser.xml:185:9
	<anonymous>                      chrome://browser/content/tabbrowser.xml:185:9
	tabs_XBL_Constructor         chrome://global/content/bindings/tabbox.xml:241:1
	TabBrowser/<                      chrome://browser/content/tabbrowser.js:38:14
	get                               resource://gre/modules/XPCOMUtils.jsm:194:21
	_updateNewTabVisibility          chrome://browser/content/tabbrowser.js:5001:9
	TabBrowser                        chrome://browser/content/tabbrowser.js:171:5
	tabbrowser_XBL_Constructor       chrome://browser/content/tabbrowser.xml:47:20
TypeError: this.parentNode.tabbrowser is null[Learn More]       tabbrowser.xml:1606:11
	set__visuallySelected          chrome://browser/content/tabbrowser.xml:1606:11
	set__selected                  chrome://browser/content/tabbrowser.xml:1629:13
	set_selectedIndex           chrome://global/content/bindings/tabbox.xml:378:13
	tabs_XBL_Constructor        chrome://global/content/bindings/tabbox.xml:251:13
	TabBrowser/<                      chrome://browser/content/tabbrowser.js:38:14
	get                               resource://gre/modules/XPCOMUtils.jsm:194:21
	_updateNewTabVisibility          chrome://browser/content/tabbrowser.js:5001:9
	TabBrowser                        chrome://browser/content/tabbrowser.js:171:5
	tabbrowser_XBL_Constructor       chrome://browser/content/tabbrowser.xml:47:20
TypeError: this.tabbrowser is null[Learn More]                   tabbrowser.xml:333:15
	_setPositionalAttributes        chrome://browser/content/tabbrowser.xml:333:15
	tabbrowser-tabs_XBL_Constructor chrome://browser/content/tabbrowser.xml:175:11
	TabBrowser/<                      chrome://browser/content/tabbrowser.js:38:14
	get                               resource://gre/modules/XPCOMUtils.jsm:194:21
	_updateNewTabVisibility          chrome://browser/content/tabbrowser.js:5001:9
	TabBrowser                        chrome://browser/content/tabbrowser.js:171:5
	tabbrowser_XBL_Constructor       chrome://browser/content/tabbrowser.xml:47:20

Comment 3

[dw-dev]

CORRECTION to original probem description.

There is a mistake in the second set of steps, using Firefox Nightly 60.0a1 (dated after 4 March 2018).

In step 4, use "Toggle All Toolbars" instead of "Toggle Toolbars"

 - The "chromeWin is null" exception still happens.
 - Sometimes the tabs(s) in one or both tiled windows crash with the message "Gah. Your tab just crashed."

LOG 2, in Comment 2 above, is for the case where the tabs DO NOT crash.

LOG 3, in Comment 4 below, is for the case where the tabs DO crash.

Comment 4

[dw-dev]

LOG 3

chromeWin is null                                                    PrivateBrowsingUtils.jsm:39
	isBrowserPrivate                    resource://gre/modules/PrivateBrowsingUtils.jsm:39:1
	create                                    chrome://browser/content/ext-windows.js:127:29
	create                                                                self-hosted:975:17
	call/result<                           resource://gre/modules/ExtensionParent.jsm:772:57
	withPendingBrowser                     resource://gre/modules/ExtensionParent.jsm:427:26
	next                                                                  self-hosted:1211:9
newwin is undefined                                                           background.js:2944
Unchecked lastError value: Error: An unexpected error occurred                background.js:2939
TypeError: this.tabbrowser is null[Learn More]                              tabbrowser.xml:185:9
	<anonymous>                                chrome://browser/content/tabbrowser.xml:185:9
	tabs_XBL_Constructor                   chrome://global/content/bindings/tabbox.xml:241:1
	TabBrowser/<                                chrome://browser/content/tabbrowser.js:38:14
	get                                         resource://gre/modules/XPCOMUtils.jsm:194:21
	_updateNewTabVisibility                    chrome://browser/content/tabbrowser.js:5001:9
	TabBrowser                                  chrome://browser/content/tabbrowser.js:171:5
	tabbrowser_XBL_Constructor                 chrome://browser/content/tabbrowser.xml:47:20
TypeError: this.parentNode.tabbrowser is null[Learn More]                 tabbrowser.xml:1606:11
	set__visuallySelected                    chrome://browser/content/tabbrowser.xml:1606:11
	set__selected                            chrome://browser/content/tabbrowser.xml:1629:13
	set_selectedIndex                     chrome://global/content/bindings/tabbox.xml:378:13
	tabs_XBL_Constructor                  chrome://global/content/bindings/tabbox.xml:251:13
	TabBrowser/<                                chrome://browser/content/tabbrowser.js:38:14
	get                                         resource://gre/modules/XPCOMUtils.jsm:194:21
	_updateNewTabVisibility                    chrome://browser/content/tabbrowser.js:5001:9
	TabBrowser                                  chrome://browser/content/tabbrowser.js:171:5
	tabbrowser_XBL_Constructor                 chrome://browser/content/tabbrowser.xml:47:20
TypeError: this.tabbrowser is null[Learn More]                             tabbrowser.xml:333:15
	_setPositionalAttributes                  chrome://browser/content/tabbrowser.xml:333:15
	tabbrowser-tabs_XBL_Constructor           chrome://browser/content/tabbrowser.xml:175:11
	TabBrowser/<                                chrome://browser/content/tabbrowser.js:38:14
	get                                         resource://gre/modules/XPCOMUtils.jsm:194:21
	_updateNewTabVisibility                    chrome://browser/content/tabbrowser.js:5001:9
	TabBrowser                                  chrome://browser/content/tabbrowser.js:171:5
	tabbrowser_XBL_Constructor                 chrome://browser/content/tabbrowser.xml:47:20
this is undefined                                                        UTEventReporting.jsm:51
	sendSessionEndEvent             resource://activity-stream/lib/UTEventReporting.jsm:51:1
	sendUTEvent                       resource://activity-stream/lib/TelemetryFeed.jsm:364:7
	endSession                        resource://activity-stream/lib/TelemetryFeed.jsm:240:5
	onAction                          resource://activity-stream/lib/TelemetryFeed.jsm:391:9
	_middleware/</<                           resource://activity-stream/lib/Store.jsm:49:11
	Store/this[method]                        resource://activity-stream/lib/Store.jsm:28:55
	onActionFromContent resource://activity-stream/lib/ActivityStreamMessageChannel.jsm:87:5
	onNewTabUnload     resource://activity-stream/lib/ActivityStreamMessageChannel.jsm:232:5
	onNewTabUnload                                                        self-hosted:975:17
	callListeners                          resource://gre/modules/RemotePageManager.jsm:33:9
	portMessageReceived                   resource://gre/modules/RemotePageManager.jsm:123:5
	portMessageReceived                                                   self-hosted:975:17
	callListeners                          resource://gre/modules/RemotePageManager.jsm:33:9
	ChromeMessagePort.prototype.observe   resource://gre/modules/RemotePageManager.jsm:361:3

Comment 5

[dw-dev]

One other important piece of information.

I have tested with both Firefox 58.0 and Firefox Developer Edition 59.0b12 - and none of these problems happen.

Comment 6

[David Olah]

I reproduced the issue on Nightly 60.0a1 (2018-03-07), Windows 10.0, x64 and it is not reproducing on 59.0 (64-bit).

The crash signature is: @ mozilla::ipc::MessageChannel::CxxStackFrame::CxxStackFrame | mozilla::ipc::MessageChannel::Send | mozilla::layers::PAPZCTreeManagerChild::SendUpdateZoomConstraints

Please see also: https://bugzilla.mozilla.org/show_bug.cgi?id=1392621 because the crash signature is related to it.

Comment 7

[dw-dev]

One thing I forgot to mention, which may be important, is that for me the chromeWin and tab crashed exceptions only happened when the original Firefiox window was "maximized" (on Windows 10).

Comment 8

[dw-dev]

This bug appears to have been fixed in Firefox Nightly 60.0a1 (2018-03-09) (64-bit).

Comment 9

[David Olah]

Hi dw-dev,

I retried today on Nightly 60.0a1 (2018-03-09) and I confirm that it is fixed now. 

Thank you for your feedback. I will close the bug and anytime you have problems with this issue please fill free to reopen it.

Comment 10

[dw-dev]

Hi David,

I have discovered that this problem still happens under certain circumstances with Firefox Nightly 61.0a1, when using the currently released version of Tile Tabs WE (9.0).

I have found two ways to reproduce the problem:

FIRST WAY

1. Install Tile Tabs WE 9.0.
2. Open the Browser Toolbox (Tools > Web Developer > Browser Toolbox).
3. Click on the Tile Tabs WE button to create a layout with 2 tiled windows.
4. Right-click on the Tile Tabs WE button and select Toggle Toolbars a few times, or use the Toggle Toolbars keyboard shortcut
   (Alt+Shift+RightArrow on Windows/Linux or MacCtrl+Shift+RightArrow on Mac)

It seems that enabling the Browser Toolbox exacerbates the problem and makes it happen almost immediately.  This implies there may be a timing issue.

SECOND WAY

1. Install Tile Tabs WE 9.0.
2. Click on the Tile Tabs WE button to create a layout with 2 tiled windows.
3. Use the Toggle Toolbars keyboard shortcut many times and very rapidly
   (Alt+Shift+RightArrow on Windows/Linux or MacCtrl+Shift+RightArrow on Mac)

The fact that Toggle Toolbars has to be done many times and very rapidly implies that there is a timing issue.

In the Tile Tabs WE code, I have tried throttling the Toggle Toolbars operation to a maximum of once every 200ms (which nearly eliminates the problem) and to a maximum of once every  300ms (which completely eliminates the problem).


Using either the FIRST WAY or the SECOND WAY, the console log looks like:

> NS_ERROR_NOT_INITIALIZED: ext-windows.js:19
> Key event not available on some keyboard layouts: key=“i” modifiers=“accel,alt,shift” id=“key_browserToolbox” browser.xul
> [Show/hide message details.] chromeWin is null PrivateBrowsingUtils.jsm:39

For some reason, it is no longer possible to copy and paste the show/hide message details from the browser console, but I have transcribed them by hand below:

> NS_ERROR_NOT_INITIALIZED: ext-windows.js:19
> Key event not available on some keyboard layouts: key=“i” modifiers=“accel,alt,shift” id=“key_browserToolbox” browser.xul
> chromeWin is null    PrivateBrowsingUtils.jsm:39
>    isBrowserPrivate      resource://gre/modules/PrivateBrowsingUtils.jsm:39:1
>    create                chrome://browser/content/ext-windows.js:127:29
>    create                self-hosted:982:17
>    call/result<          resource://gre/modules/ExtensionParent.jsm:818:57
>    withPendingBrowser    resource://gre/modules/ExtensionParent.jsm:473:26
>    next                  self-hosted:1218:9


Just to note, this problem dose not happen when using Tile Tabs WE on Chrome, no matter how many times or how fast Toggle Toolbars is used.

Comment 11

[David Olah]

dw-dev, thank you for the reply!

I managed to reproduce the steps. Based on comment 10, I will reopen the bug.

Comment 12

[Jim Mathies [:jimm]]

Looking for a prioritization.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

FYI, this appears to be doing IPC on the wrong thread while destroying a tab/presshell -> destroying zoomconstraints

Comment 14

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

I've been looking a bit into the "chromeWin is null" part of this issue, which seems an additional issues to look into besides the crash which is currently investigated as part of Bug 1392621.

By investigating a bit more deeply what the extension was going and what was happening, I noticed that, while the exception is being triggered later (when the extension is recreating the existent windows to add/remote the toolbars), the actual underlying bug did already happen at that point (to be precise: when the two tabs from the initial window are being turned into two "tiled" windows with a single tab each).

To turn the two tabs into two tiled windows, the extension is calling `browser.windows.create` with a tabId to be adopted in the new window, while it has also subscribed the webNavigation.onCompleted event.

A new window is being created from https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/browser/components/extensions/parent/ext-windows.js#170-171
with the tab to be adopted as the first of the window.arguments array, but unfortunately when the tabTracker internal utility is going to recognize the tab in the new window as the "adopting tab" (from https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/browser/components/extensions/parent/ext-browser.js#408) there is a pretty high chance that the webNavigation API has already been triggered by the new window initializing its first tab to "about:blank", and that tab is wrongly getting assigned a new tabId by tabTracker.

Later tabTracker is going to update the tabIds (as it can be seen by looking at the code linked above), but the wrongly assigned tabId is going to still be part of the `tabTracker._tabIds` map and so even if that tabId is never going to be in a `browser.tabs.query` result, it is (wrongly) considered as a valid id by other API calls (basically because it can still be retrieved by the `tabTracker._tabIds` map), but the related API call is likely to fail because that tab has been removed after adopting the original existent tab and so its `ownerGlobal` is null (and besides the more "visible" issues, that tab is going to be kept active by the `tabTracker._tabIds` map and it will be free to be garbage collected only once/if that extension is shutting down).

Based on the above analysis, I'm going to attach a patch with a proposed fix for the underlying issue:
adding to ext-webNavigation.js an additional check to recognize this scenario (webNavigation event triggered by a tab created in a new window just to adopt an existent tab) so that it can avoid to fire a webNavigation event (because the tab is not really navigating or loading, it is just a tab created internally specifically to adopt the existent one) and avoid that tab to get wrongly assigned a new tabId at all.

Comment 15

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

Attachment: Bug 1443221 - Do not send a webNavigation API event for a browser which is adopting an existent tab in a new window.

Review commit: https://reviewboard.mozilla.org/r/235022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235022/

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8966295 [details]
Bug 1443221 - Do not send a webNavigation API event for a browser which is adopting an existent tab in a new window.

https://reviewboard.mozilla.org/r/235022/#review240762

This also sounds like bug 1423705.  Assuming my question below identifies where the invalid tabid is getting pushed into tabTracker._tabIds, r+.

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:182
(Diff revision 1)
> +    // Wait the tabs load to be completed, then listen for the webNavigation events
> +    // and move the tabs into new windows.

lots of stuff here that seems like it could simply be done using BrowserTestUtils.  I had to decipher what was actually part of the test vs. boilerplate to get ready to test.

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:201
(Diff revision 1)
> +    // Wait the tabs to have been attached to the new window and then assert that no
> +    // webNavigation.onCompleted event has been received.
> +    browser.test.log("Waiting tabs move to new window to be attached");
> +    await Promise.all(onceTabsAttached);
> +
> +    browser.test.assertEq("[]", JSON.stringify(webNavEvents),

Couldn't you just test that webNavEvents.length == 0?

::: toolkit/components/extensions/parent/ext-webNavigation.js:130
(Diff revision 1)
> +      if (chromeWin && chromeWin.arguments && chromeWin.arguments[0] instanceof chromeWin.XULElement &&
> +          chromeWin.gBrowser.selectedBrowser === data.browser) {
> +        return;
> +      }
> +
>        // Fills in tabId typically.

Is this where the invalid tabid is being created?

Comment 17

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

Comment on attachment 8966295 [details]
Bug 1443221 - Do not send a webNavigation API event for a browser which is adopting an existent tab in a new window.

https://reviewboard.mozilla.org/r/235022/#review240762

ah! thanks for mentioning its bugzilla issue number, I was sure that we had another issue filed related to tabs being assigned a different tabId when moved between windows, I wanted to take a look to that issue and see if it is related to this issue or if it is another similar issue.

> lots of stuff here that seems like it could simply be done using BrowserTestUtils.  I had to decipher what was actually part of the test vs. boilerplate to get ready to test.

sounds good to me, I'm going to update the patch with the test sightly reworked as suggested.

> Couldn't you just test that webNavEvents.length == 0?

I'm ok if we want to turn this into `browser.test.assertEq(0, webNavEvents.length, "...");`, but the main reason why I opted for the above assertion is that if this assertion fails on try, we would get a much more helpful assertion error (because it will actually list which unexpected webNavigation events have been received, otherwise we will only know that we have received some, but no clues on what they were triggered from).

> Is this where the invalid tabid is being created?

Yes, for the webNavigation scenarios at least, `tabTracker.getBrowserData(data.browser)` is where the tabId is being created for an unknown `browser` (and its tab) (https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/browser/components/extensions/parent/ext-browser.js#250-263)

But it is not the only way an "still unknown" tab could get a tabId, `tabTracker.getId(nativeTab)` will assign a new tabId if the nativeTab doesn't have one yet: https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/browser/components/extensions/parent/ext-browser.js#237-248

And so, there could be potentially other ways to get a tabId assigned to a tab (and its "browser" element) that should not have one, especially if the API that is retrieving the tabId is potentially intercepting the "browser" element or its tab while the tab is still being adopted (e.g. I've not yet tried, but webRequest is using `tabTracker.getBrowserData` for the same purpose of the webNavigation API here: https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/toolkit/components/extensions/parent/ext-webRequest.js#19 and so using the tabs API combined with the webRequest API listeners and moving tabs between windows could lead to a similar issue).

Comment 18

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

Comment on attachment 8966295 [details]
Bug 1443221 - Do not send a webNavigation API event for a browser which is adopting an existent tab in a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235022/diff/1-2/

Comment 19

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '23936' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc'
remote: abort: working directory of /repo/hg/mozilla/try: timed out waiting for lock held by 'hgssh4.dmz.scl3.mozilla.com/effffffc:23936'
abort: stream ended unexpectedly (got 0 bytes, expected 4)

Comment 20

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bc480b2e7d89dc91f7a91d78832d864e42ab702e -d 7a26b1da04e5: rebasing 457572:bc480b2e7d89 "Bug 1443221 - Do not send a webNavigation API event for a browser which is adopting an existent tab in a new window. r=mixedpuppy" (tip)
merging toolkit/components/extensions/parent/ext-webNavigation.js
warning: conflicts while merging toolkit/components/extensions/parent/ext-webNavigation.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 21

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

Comment on attachment 8966295 [details]
Bug 1443221 - Do not send a webNavigation API event for a browser which is adopting an existent tab in a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235022/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bdc8b924470
Do not send a webNavigation API event for a browser which is adopting an existent tab in a new window. r=mixedpuppy

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4bdc8b924470

Comment 24

[CosminB]

Attachment: Bug1443221.gif

I was able to reproduce this issue on Firefox 59.0.3 (20180427210249) under Win 7 64-bit and Mac OS X 10.13.3.

The errors from Comment1 are displayed in the browser console.

This issue is verified as fixed Firefox 61.0a1 (20180503220110) under Win 7 64-bit and Mac OS X 10.13.3.

Please have a look at the attached video.

I also want to mention that Comment10 still reproduces the issue on Firefox 61.0a1 (20180503220110) under Win 7 64-bit and Mac OS X 10.13.3.

Luca has filed an issue for it Bug1458918