1401846 - Fix the race condition between tabbrowser and tabbox constructors

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

As described by Kevin in bug 1362774 comment 111, there's a race condition that is reproducible via our test infra and in real life in some edge cases, and bug 1362774 makes it more prominent by making try fail permanently on it.

Let's use the time we have until 58 window opens to fix it to unblock landing of bug 1362774.

Comment 1

[u462496]

I am re-posting bug 1362774 comment 111 for convenience with some of the noise removed from the results and comments re-written for clarity:

******

I set some probes and I assigned windows and tabs unique IDs for tracking.  This is what I observed:

Error free run:

GECKO(16213) | XXX : window.uid : 3301489    tabbrowser : constructor :    initialTab.uid : 3301489
GECKO(16213) | XXX : window.uid : 3301489    tabbox :     constructor :    tab.uid :        3301489
GECKO(16213) | XXX : window.uid : 3325735    tabbrowser : constructor :    initialTab.uid : 3325735
GECKO(16213) | XXX : window.uid : 3325735    tabbox :     constructor :    tab.uid :        3325735
GECKO(16213) | XXX : window.uid : 3325887    tabbrowser : constructor :    initialTab.uid : 3325887
GECKO(16213) | XXX : window.uid : 3325887    tabbox :     constructor :    tab.uid :        3325887
GECKO(16213) | XXX : window.uid : 3301489    tabbrowser : addTab :         tab.uid :        3325985
GECKO(16213) | XXX : window.uid : 3301489    tabbrowser : _insertBrowser : tab.uid :        3325985    tab.linkedBrowser : [object XULElement]
GECKO(16213) | XXX : window.uid : 3301489    tabbrowser : addTab :         tab.uid :        3326318
GECKO(16213) | XXX : window.uid : 3301489    tabbrowser : _insertBrowser : tab.uid :        3326318    tab.linkedBrowser : [object XULElement]

Run that throws error:

GECKO(16279) | XXX : window.uid : 3372543    tabbrowser : constructor :    initialTab.uid : 3372543
GECKO(16279) | XXX : window.uid : 3372543    tabbox :     constructor :    tab.uid :        3372543
GECKO(16279) | XXX : window.uid : 3372543    tabbrowser : addTab :         tab.uid :        3380817 /* addTab runs for the first window before tabbrowser/tabbox constructors run for subsequent windows - by happenstance since opening of subsequent windows is presumably asynchronous */
GECKO(16279) | XXX : window.uid : 3372543    tabbrowser : _insertBrowser : tab.uid :        3380817    tab.linkedBrowser : [object XULElement]
GECKO(16279) | XXX : window.uid : 3380961    tabbox :     constructor :    tab.uid :        undefined  /* tabbrowser constructor for this window has not run yet, thus tab.uid has not been assigned */
GECKO(16279) | XXX : window.uid : 3380961    tabbrowser : _insertBrowser : tab.uid :        undefined        tab.linkedBrowser : undefined  /* ...yet tabbrowser methods have been bound, so tabbrowser code runs which would expect browser to be linked to tab */
GECKO(16279) | XXX : window.uid : 3380961    ERROR : tabbrowser : _insertBrowser : TypeError: invalid 'in' operand browser
GECKO(16279) |     XXX : stack : _insertBrowser@chrome://browser/content/tabbrowser.xml:2397:1
GECKO(16279) |     XXX : stack : getRelatedElement@chrome://browser/content/tabbrowser.xml:6969:11
GECKO(16279) |     XXX : stack : set_selectedIndex@chrome://global/content/bindings/tabbox.xml:408:31
GECKO(16279) |     XXX : stack : tabs_XBL_Constructor@chrome://global/content/bindings/tabbox.xml:277:13
GECKO(16279) |     XXX : stack : @chrome://browser/content/tabbrowser.xml:46:9
GECKO(16279) |     XXX : stack : tabbrowser_XBL_Constructor@chrome://browser/content/tabbrowser.xml:5726:11
GECKO(16279) |     XXX : stack : get@chrome://browser/content/browser.js:248:21
GECKO(16279) |     XXX : stack : getOpenTabsAndWinsCounts@resource:///modules/BrowserUsageTelemetry.jsm:100:5
GECKO(16279) |     XXX : stack : _onTabOpen@resource:///modules/BrowserUsageTelemetry.jsm:615:28
GECKO(16279) |     XXX : stack : handleEvent@resource:///modules/BrowserUsageTelemetry.jsm:365:9
GECKO(16279) |     XXX : stack : addTab@chrome://browser/content/tabbrowser.xml:2713:13
GECKO(16279) |     XXX : stack : loadOneTab@chrome://browser/content/tabbrowser.xml:1713:23
GECKO(16279) |     XXX : stack : _openURIInNewTab@chrome://browser/content/browser.js:5221:15
GECKO(16279) |     XXX : stack : browser_openURIInFrame@chrome://browser/content/browser.js:5362:12
GECKO(16279) |     XXX : stack :
GECKO(16279) | XXX : window.uid : 3380961    tabbrowser : constructor :    initialTab.uid : 3380974  /* tabbrowser constructor for this window finally runs, out of sequence */
GECKO(16279) | XXX : window.uid : 3372543    tabbrowser : addTab :         tab.uid :        3381120
GECKO(16279) | XXX : window.uid : 3372543    tabbrowser : _insertBrowser : tab.uid :        3381120    tab.linkedBrowser : [object XULElement]
GECKO(16279) | XXX : window.uid : 3381711    tabbrowser : constructor :    initialTab.uid : 3381711
GECKO(16279) | XXX : window.uid : 3381711    tabbox :     constructor :    tab.uid :        3381711

Observe the following:

In the error-free run, tabbrowser constructor always runs prior to tabbox constructor for each and every window.

In the error run, for the second window (id=3380961), tabbox constructor runs prior to tabbrowser constructor.  Yet I also observed something else interesting.  tabbrowser code for the second window is available and bound to the tabbox, even though the constructor has not yet run.  Thus `(tabbrowser)getRelatedElement` and `_insertBrowser`[1] get run on a tab which has not had the browser linked to it yet (which would happen in the tabbrowser constructor).  (Observe also that tab.uid in the `_insertBrowser` call is `undefined`, which would have been assigned had the tabbrowser constructor run.)

It would seem to me that the raciness in this case is an XBL issue and how XBL elements get bound and constructors run.  `tabbrowser` is the higher-order binding, yet in some cases, the constructor for `tabbox` - the lower order binding - gets run first.  Yet the higher-order methods bound on the element are in place.

Another observation I made is that:

In all cases where I observed everything ran in the expected order (and no error), `addTab` never ran on the first window until after tabbrowser and tabbox constructors had run for the subsequent windows.

In all cases where I observed everything did not run in the expected order (and had error(s)), `addTab` ran on the first window before bindings were set for the subsequent windows.

I am assuming that the windows were opened asynchronously, and thus the sequence of `addTab` running is not predictable.  It may be possible that the `addTab` call(s), if run before/while the subsequent windows are setting their tabbox/tabbrowser bindings, somehow trigger a glitch in the XBL binding process for those windows.

[1] `insertBrowser` would normally never even run for the initial tab because `insertBrowser` will not run on a tab which has tab.linkedPanel set, which would normally have gotten set in tabbrowser constructor.  But since tabbrowser constructor hasn't run yet, `insertBrowser` attempts to run on a tab which has not been initialized completely.

Comment 2

[Dão Gottwald [:dao]]

Should be fixed now.