Closed Bug 1443849 Opened 3 years ago Closed 3 years ago

Make gBrowser.init more predictable by calling it from gBrowserInit, prevent early access to gBrowser

Categories

(Firefox :: Tabbed Browser, enhancement, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: perf)

Attachments

(4 files)

Bug 1443849 - Part 1: Don't dispatch the TabAttrModified event for the visuallyselected attribute when it hasn't changed.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1443849 - Part 2: Don't touch gBrowser before gBrowserInit.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1443849 - Part 3: Get rid of the tabbrowser property, use gBrowser directly.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1443849 - Part 4: Call gBrowser.init from gBrowserInit and prevent early access to gBrowser.
59 bytes, text/x-review-board-request
Gijs
: review+
jcristau
: approval-mozilla-beta+
Details 
gBrowser will likely be accessed before we really need to construct these bindings, so if possibly we should avoid touching them in init() and make the gBrowser properties pointing to these elements lazy again.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
No longer blocks: 1444165
Blocks: 1401846
(In reply to Dão Gottwald [::dao] from comment #2)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4a5532aef3865fc9c92af626ea8583fef4ca5e29

*Sigh* This intermittent failure is back if it was even fixed in the first place:

browser_check_identity_state.js | Uncaught exception - at chrome://global/content/bindings/tabbox.xml:384 - TypeError: this.tabbox is undefined

I can't seem to make sense of this, and at this point I think I'd just use document.getElementById("tabbrowser-tabbox") here: https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/tabbrowser.xml#144

What do you think?

Here's a new try run with that change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a24acd2ca5d42b8d59dfd1b2ffac80de79b986b
In the failing trypush, there are some earlier errors:


[task 2018-03-09T10:08:43.495Z] 10:08:43     INFO - Console message: [JavaScript Error: "TypeError: can't convert undefined to object" {file: "chrome://browser/content/tabbrowser.js" line: 255}]
[task 2018-03-09T10:08:43.496Z] 10:08:43     INFO - get visibleTabs@chrome://browser/content/tabbrowser.js:255:27
[task 2018-03-09T10:08:43.497Z] 10:08:43     INFO - _setPositionalAttributes@chrome://browser/content/tabbrowser.xml:308:15
[task 2018-03-09T10:08:43.497Z] 10:08:43     INFO - tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml:137:11
[task 2018-03-09T10:08:43.498Z] 10:08:43     INFO - init@chrome://browser/content/tabbrowser.js:143:24
[task 2018-03-09T10:08:43.499Z] 10:08:43     INFO - get@chrome://browser/content/browser.js:157:5
[task 2018-03-09T10:08:43.499Z] 10:08:43     INFO - getOpenTabsAndWinsCounts@resource:///modules/BrowserUsageTelemetry.jsm:100:5
[task 2018-03-09T10:08:43.500Z] 10:08:43     INFO - _onTabOpen@resource:///modules/BrowserUsageTelemetry.jsm:634:28
[task 2018-03-09T10:08:43.501Z] 10:08:43     INFO - handleEvent@resource:///modules/BrowserUsageTelemetry.jsm:369:9
[task 2018-03-09T10:08:43.501Z] 10:08:43     INFO - addTab@chrome://browser/content/tabbrowser.js:2414:5
[task 2018-03-09T10:08:43.502Z] 10:08:43     INFO - openNewForegroundTab/promises<@resource://testing-common/BrowserTestUtils.jsm:196:44
[task 2018-03-09T10:08:43.503Z] 10:08:43     INFO - switchTab@resource://testing-common/BrowserTestUtils.jsm:237:7
[task 2018-03-09T10:08:43.503Z] 10:08:43     INFO - openNewForegroundTab@resource://testing-common/BrowserTestUtils.jsm:190:9
[task 2018-03-09T10:08:43.504Z] 10:08:43     INFO - loadNewTab@chrome://mochitests/content/browser/browser/base/content/test/siteIdentity/browser_check_identity_state.js:13:10
[task 2018-03-09T10:08:43.505Z] 10:08:43     INFO - dataUriTest@chrome://mochitests/content/browser/browser/base/content/test/siteIdentity/browser_check_identity_state.js:357:22
[task 2018-03-09T10:08:43.506Z] 10:08:43     INFO - async*test_data_uri@chrome://mochitests/content/browser/browser/base/content/test/siteIdentity

This is implying that actually, https://hg.mozilla.org/try/file/4a5532aef3865fc9c92af626ea8583fef4ca5e29/browser/base/content/tabbrowser.js#l143 is triggering XBL construction of the tabcontainer etc. early:

this.mCurrentTab = document.getElementById("tabbrowser-initialTab");

This kind of makes sense because the initial tab will be a (grand*)kid of the tabcontainer and tabbox.

Does that help, or do you think we should still just go with your suggestion in comment #3?
Flags: needinfo?(dao+bmo)
Comment on attachment 8957495 [details]
Bug 1443849 - Part 1: Don't dispatch the TabAttrModified event for the visuallyselected attribute when it hasn't changed.

(In reply to :Gijs (under the weather; responses will be slow) from comment #4)
> This is implying that actually,
> https://hg.mozilla.org/try/file/4a5532aef3865fc9c92af626ea8583fef4ca5e29/
> browser/base/content/tabbrowser.js#l143 is triggering XBL construction of
> the tabcontainer etc. early:
> 
> this.mCurrentTab = document.getElementById("tabbrowser-initialTab");
> 
> This kind of makes sense because the initial tab will be a (grand*)kid of
> the tabcontainer and tabbox.
> 
> Does that help, or do you think we should still just go with your suggestion
> in comment #3?

Yeah, so I could fix this by setting up the lazy getters before messing with mCurrentTab, but why then make them lazy in the first place? So I guess I should trigger this stuff from the tabbox constructor...
Flags: needinfo?(dao+bmo)
Attachment #8957495 - Flags: review?(gijskruitbosch+bugs)
In latest nightly I get

TypeError: this._browserBindingProperties is undefined
        _insertBrowser chrome://browser/content/tabbrowser.js:2041:1
	getRelatedElement chrome://browser/content/tabbrowser.xml:912:11
	set_selectedIndex chrome://global/content/bindings/tabbox.xml:382:31
	tabs_XBL_Constructor chrome://global/content/bindings/tabbox.xml:251:13
	init chrome://browser/content/tabbrowser.js:33:25
	get chrome://browser/content/browser.js:157:5
	getObjectTag jar:file:///C:/Program%20Files/Mozilla%20Firefox/Test/omni.ja!/components/multiprocessShims.js:118:1
	interposeProperty jar:file:///C:/Program%20Files/Mozilla%20Firefox/Test/omni.ja!/components/multiprocessShims.js:139:45
	<anonymous> chrome://xnotifier/content/overlay.js:469:1

My add-on (https://github.com/Loirooriol/Z-Notifier/) does this:

    window.addEventListener("load", com.tobwithu.xnotifier.onLoad, false);

how come this line ends up causing tabbrowser errors?

Please fix this. And I would appreciate if important things weren't messed up at the very end of a cycle, thanks.
(In reply to Oriol Brufau [:Oriol] from comment #6)
> how come this line ends up causing tabbrowser errors?
> 
> Please fix this. And I would appreciate if important things weren't messed
> up at the very end of a cycle, thanks.

We don't officially support overlay extensions anymore. You'll have to deal with things changing in Nightly even at the end of a cycle. However, feel free to dig into the Firefox source code and let us know if you've found a mistake rather than a mere incompatibility with your add-on.
(In reply to Dão Gottwald [::dao] from comment #7)
> feel free to dig into the Firefox source code and let us know if you've found a
> mistake rather than a mere incompatibility with your add-on.

That's why I included the stack trace. gBrowser.init sets tabContainer in
https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/tabbrowser.js#33

and this ends up accessing _browserBindingProperties, but that's set later in init:
https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/tabbrowser.js#96

So the order is messed up.
(In reply to Oriol Brufau [:Oriol] from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > feel free to dig into the Firefox source code and let us know if you've found a
> > mistake rather than a mere incompatibility with your add-on.
> 
> That's why I included the stack trace. gBrowser.init sets tabContainer in
> https://searchfox.org/mozilla-central/rev/
> 588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/tabbrowser.
> js#33
> 
> and this ends up accessing _browserBindingProperties, but that's set later
> in init:
> https://searchfox.org/mozilla-central/rev/
> 588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/tabbrowser.
> js#96
> 
> So the order is messed up.

Only if there's a selectedindex attribute is set (see code in tabbox.xml), which must be ending up there because of your add-on because we don't hit this codepath without it (I just checked).

(In reply to Oriol Brufau [:Oriol] from comment #6)
> 	getObjectTag
> jar:file:///C:/Program%20Files/Mozilla%20Firefox/Test/omni.ja!/components/
> multiprocessShims.js:118:1
> 	interposeProperty
> jar:file:///C:/Program%20Files/Mozilla%20Firefox/Test/omni.ja!/components/
> multiprocessShims.js:139:45
> 	<anonymous> chrome://xnotifier/content/overlay.js:469:1
> 
> My add-on (https://github.com/Loirooriol/Z-Notifier/) does this:
> 
>     window.addEventListener("load", com.tobwithu.xnotifier.onLoad, false);
> 
> how come this line ends up causing tabbrowser errors?

Your add-on is getting multiprocess shims (because it's not listed as compatible), which we're also removing (bug 1443983).
(In reply to :Gijs (under the weather; responses will be slow) from comment #9)
> Only if there's a selectedindex attribute is set (see code in tabbox.xml),
> which must be ending up there because of your add-on because we don't hit
> this codepath without it (I just checked).

I will look into it, but I suspect the incompatibility is with the multiprocess shim, not with my add-on. So (being very optimistic) its removal might in fact fix the issue. My add-on already required a single process anyways, so I shouldn't need the shim.
Depends on: 1444530
Depends on: 1444614
Depends on: 1444625
Depends on: 1444694
Summary: Avoid touching the tabs, tabbox, and tabpanels elements in gBrowser.init → Make gBrowser.init more predictable by calling it from gBrowserInit, prevent early access to gBrowser
Blocks: 1444510
Comment on attachment 8957495 [details]
Bug 1443849 - Part 1: Don't dispatch the TabAttrModified event for the visuallyselected attribute when it hasn't changed.

https://reviewboard.mozilla.org/r/226396/#review232926

This patch would have been easier to review if the `s/this.tabbrowser/gBrowser/`, if/else bracing changes, and the actual meat of the change had been separated out into 3 different bits. Probably too late now because there's not really a lot left to do with this, but for future reference...

::: browser/base/content/tabbrowser.xml:303
(Diff revision 2)
> -          let visibleTabs = this.tabbrowser.visibleTabs;
> +          if (this.childNodes.length == 1) {
> +            // Cannot access gBrowser before gBrowserInit.
> +            return [ this.firstChild ];
> +          }

How is the childNodes length an appropriate proxy for knowing whether `gBrowserInit` has run here? Can we do something more explicit, and then just return `Array.from(this.children)` or something?

::: browser/base/content/tabbrowser.xml:940
(Diff revision 2)
> +          if (this.childNodes.length == 1 &&
> +              this.tabbox.tabpanels.childNodes.length == 1) {
> +            // Cannot access gBrowser before gBrowserInit.
> +            return this.tabbox.tabpanels.firstChild;
> +          }

Again, this seems fragile...

::: browser/base/content/tabbrowser.xml:1106
(Diff revision 2)
> +        }
>  
>          if (event.target.localName == "tab") {
> -          this.tabbrowser.removeTab(event.target, {animate: true,
> -                byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE});
> +          gBrowser.removeTab(event.target, {
> +            animate: true,
> +            byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE

Nit: trailing comma please.

::: browser/base/content/tabbrowser.xml:1652
(Diff revision 2)
> -          if (val)
> +          if (val == (this.getAttribute("visuallyselected") == "true")) {
> +            return val;
> +          }

You're changing behaviour associated with the async tab switcher stuff here (used to always fire a an attribute change event, not anymore)... in a giant commit that changes a bunch of other stuff. Can you do this separately, and get review from mconley? I think it's probably fine, but bisecting this patch for problems later is going to be super painful.

::: browser/base/content/tabbrowser.xml:2015
(Diff revision 2)
>          }
>  
>          if (event.originalTarget.getAttribute("anonid") == "close-button") {
> -          let tabContainer = this.parentNode;
> -          tabContainer.tabbrowser.removeTab(this, {animate: true,
> -                  byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE});
> +          gBrowser.removeTab(this, {
> +            animate: true,
> +            byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE

Nit: trailing comma please.
Attachment #8957495 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #13)
> > -          let visibleTabs = this.tabbrowser.visibleTabs;
> > +          if (this.childNodes.length == 1) {
> > +            // Cannot access gBrowser before gBrowserInit.
> > +            return [ this.firstChild ];
> > +          }
> 
> How is the childNodes length an appropriate proxy for knowing whether
> `gBrowserInit` has run here? Can we do something more explicit, and then
> just return `Array.from(this.children)` or something?

There's now no sane way to add a tab before gBrowserInit. On the other hand, if we assumed there was a way, we should also expect that tabs could be hidden and Array.from(this.children) wouldn't be correct.

> ::: browser/base/content/tabbrowser.xml:1652
> (Diff revision 2)
> > -          if (val)
> > +          if (val == (this.getAttribute("visuallyselected") == "true")) {
> > +            return val;
> > +          }
> 
> You're changing behaviour associated with the async tab switcher stuff here
> (used to always fire a an attribute change event, not anymore)... in a giant
> commit that changes a bunch of other stuff. Can you do this separately, and
> get review from mconley? I think it's probably fine, but bisecting this
> patch for problems later is going to be super painful.

Not really a behavior change as far as the tab switcher cares; the event is for external consumers (of which we have none that actually cares about the visuallyselected attribute). Last but not least, the TabAttrModified event is generally not supposed to fire when attributes don't change.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [::dao] from comment #14)
> > You're changing behaviour associated with the async tab switcher stuff here
> > (used to always fire a an attribute change event, not anymore)... in a giant
> > commit that changes a bunch of other stuff. Can you do this separately, and
> > get review from mconley? I think it's probably fine, but bisecting this
> > patch for problems later is going to be super painful.
> 
> Not really a behavior change as far as the tab switcher cares; the event is
> for external consumers (of which we have none that actually cares about the
> visuallyselected attribute). Last but not least, the TabAttrModified event
> is generally not supposed to fire when attributes don't change.

Which is to say, I think this is a no-brainer that doesn't need more eyes, and mconley is probably busy with more important things, but let me know if you still want him to check.
(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Dão Gottwald [::dao] from comment #14)
> > > You're changing behaviour associated with the async tab switcher stuff here
> > > (used to always fire a an attribute change event, not anymore)... in a giant
> > > commit that changes a bunch of other stuff. Can you do this separately, and
> > > get review from mconley? I think it's probably fine, but bisecting this
> > > patch for problems later is going to be super painful.
> > 
> > Not really a behavior change as far as the tab switcher cares; the event is
> > for external consumers (of which we have none that actually cares about the
> > visuallyselected attribute). Last but not least, the TabAttrModified event
> > is generally not supposed to fire when attributes don't change.
> 
> Which is to say, I think this is a no-brainer that doesn't need more eyes,
> and mconley is probably busy with more important things, but let me know if
> you still want him to check.

I mean, if you're confident, go ahead and land. There's some other edgecases here (e.g. XUL observers / XBL inheritance and which of these fire/process even when things get re-set) but hopefully nothing cares about those, either.

(In reply to Dão Gottwald [::dao] from comment #14)
> (In reply to :Gijs from comment #13)
> > > -          let visibleTabs = this.tabbrowser.visibleTabs;
> > > +          if (this.childNodes.length == 1) {
> > > +            // Cannot access gBrowser before gBrowserInit.
> > > +            return [ this.firstChild ];
> > > +          }
> > 
> > How is the childNodes length an appropriate proxy for knowing whether
> > `gBrowserInit` has run here? Can we do something more explicit, and then
> > just return `Array.from(this.children)` or something?
> 
> There's now no sane way to add a tab before gBrowserInit. On the other hand,
> if we assumed there was a way, we should also expect that tabs could be
> hidden and Array.from(this.children) wouldn't be correct.

Sure, but after gBrowserInit, `this.childNodes.length` can still be 1, right? Is there no point, even in the midst of other code running (e.g. tab tearoff), where this can now return the wrong thing?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #16)
> > There's now no sane way to add a tab before gBrowserInit. On the other hand,
> > if we assumed there was a way, we should also expect that tabs could be
> > hidden and Array.from(this.children) wouldn't be correct.
> 
> Sure, but after gBrowserInit, `this.childNodes.length` can still be 1,
> right? Is there no point, even in the midst of other code running (e.g. tab
> tearoff), where this can now return the wrong thing?

Yeah, we don't allow hiding the selected tab and we don't close tabs that are the only one in a window -- we close the window instead.
(In reply to Dão Gottwald [::dao] from comment #21)
> (In reply to :Gijs from comment #16)
> > > There's now no sane way to add a tab before gBrowserInit. On the other hand,
> > > if we assumed there was a way, we should also expect that tabs could be
> > > hidden and Array.from(this.children) wouldn't be correct.
> > 
> > Sure, but after gBrowserInit, `this.childNodes.length` can still be 1,
> > right? Is there no point, even in the midst of other code running (e.g. tab
> > tearoff), where this can now return the wrong thing?
> 
> Yeah, we don't allow hiding the selected tab and we don't close tabs that
> are the only one in a window -- we close the window instead.

But I could just null-check gBrowser if you prefer that.
Comment on attachment 8957495 [details]
Bug 1443849 - Part 1: Don't dispatch the TabAttrModified event for the visuallyselected attribute when it hasn't changed.

https://reviewboard.mozilla.org/r/226396/#review233184
Attachment #8957495 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8958436 [details]
Bug 1443849 - Part 2: Don't touch gBrowser before gBrowserInit.

https://reviewboard.mozilla.org/r/227392/#review233188

::: browser/base/content/tabbrowser.xml:309
(Diff revision 1)
>          ]]></getter>
>        </property>
>  
> -      <method name="_setPositionalAttributes">
> +      <method name="_getVisibleTabs">
>          <body><![CDATA[
> -          let visibleTabs = this.tabbrowser.visibleTabs;
> +          if (this.childNodes.length == 1) {

Yes, I would prefer null-checking `gBrowser` here. :-)

::: browser/base/content/tabbrowser.xml:942
(Diff revision 1)
> +          if (this.childNodes.length == 1 &&
> +              this.tabbox.tabpanels.childNodes.length == 1) {

Ditto
Attachment #8958436 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8958437 [details]
Bug 1443849 - Part 3: Get rid of the tabbrowser property, use gBrowser directly.

https://reviewboard.mozilla.org/r/227394/#review233192
Attachment #8958437 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8958438 [details]
Bug 1443849 - Part 4: Call gBrowser.init from gBrowserInit and prevent early access to gBrowser.

https://reviewboard.mozilla.org/r/227396/#review233194
Attachment #8958438 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22799a033471
Part 1: Don't dispatch the TabAttrModified event for the visuallyselected attribute when it hasn't changed. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/b34c63d24344
Part 2: Don't touch gBrowser before gBrowserInit. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e2768d74d30e
Part 3: Get rid of the tabbrowser property, use gBrowser directly. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/54259728e099
Part 4: Call gBrowser.init from gBrowserInit and prevent early access to gBrowser. r=Gijs
Backed out for mochitest bc failures on /browser_tabs_close_beforeunload.js

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=167691571&repo=autoland&lineNumber=3323
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d422e20d52a
Part 1: Don't dispatch the TabAttrModified event for the visuallyselected attribute when it hasn't changed. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d35aa05937cb
Part 2: Don't touch gBrowser before gBrowserInit. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/774929bdd20d
Part 3: Get rid of the tabbrowser property, use gBrowser directly. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/683b4719454d
Part 4: Call gBrowser.init from gBrowserInit and prevent early access to gBrowser. r=Gijs
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd1ec0216879
Backed out 4 changesets backed out for mochitest bc failures on /browser_tabs_close_beforeunload.js
Flags: needinfo?(dao+bmo)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/7a630a4add1f
Part 4: Fix broken tabbrowser.xml after merge conflict. CLOSED TREE
Depends on: 1445732
No longer depends on: 1445732
No longer depends on: 1442582
No longer blocks: 1443800
Comment on attachment 8958438 [details]
Bug 1443849 - Part 4: Call gBrowser.init from gBrowserInit and prevent early access to gBrowser.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1442651 followup (but this was buggy before)
[User impact if declined]: Tabbed browser initialization might fail due to various race conditions (see also the bugs this blocks)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: /
[List of other uplifts needed for the feature/fix]: The uplift request is for all 4 parts. All other patches this depends on are already in 60.
[Is the change risky?]: somewhat
[Why is the change risky/not risky?]: What's mostly scary is how fragile this was before. This bug is supposed to consolidate the initialization and I feel pretty good about it.
[String changes made/needed]: /
Attachment #8958438 - Flags: approval-mozilla-beta?
Comment on attachment 8958438 [details]
Bug 1443849 - Part 4: Call gBrowser.init from gBrowserInit and prevent early access to gBrowser.

sounds like we need this in beta60
Attachment #8958438 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.