1497599 - browser.xhtml: "TypeError: this.tabbox is null; can't access its "tabpanels" property" at startup, triggered by progressmeter attributeChangedCallback

Status: RESOLVED FIXED

(Firefox :: General, defect, P5)

Description

[Brian Grinstead [:bgrins]]

I see these errors on startup with a local build with `MOZ_BROWSER_XHTML=1` as well.

JavaScript error: chrome://browser/content/tabbrowser.xml, line 1098: TypeError: this.tabbox is null; can't access its "tabpanels" property
JavaScript error: chrome://browser/content/tabbrowser.xml, line 146: TypeError: this.tabbox is null; can't access its "tabpanels" property

This appears to be due to the progressmeter Custom Element migration. Somehow the call to `this.textContent = "";` seems to be triggering XBL construction on the `tabbrowser-tabs` binding. I don't really know why yet - I guess there's some timing difference between when CE upgrade happens in XUL vs XHTML. This'll be good to figure out anyway.

Comment 1

[Brian Grinstead [:bgrins]]

A couple of differences:

1) In XHTML it the attributeChangedCallback is firing before connectedCallback for downloadsSummaryProgress
2) In XHTML for some reason clearing out the textContent of downloadsSummaryProgress is triggering the tabbrowser-tabs binding

I'd like to see if (1) is behavior shared for normal HTML docs / web content. We should likely guard against this case anyway to avoid unnecessary work (and probably even defer all initialization until DOMContentLoaded anyway)

For (2) I really have no idea why this is happening. I've never seen changing the textContent of an seemingly unrelated element trigger XBL contructor on another one. If we change (1) to wait for DOMContentLoaded then (2) will go away, but I'd like to know what's going on.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> 1) In XHTML it the attributeChangedCallback is firing before
> connectedCallback for downloadsSummaryProgress

> I'd like to see if (1) is behavior shared for normal HTML docs / web
> content. We should likely guard against this case anyway to avoid
> unnecessary work (and probably even defer all initialization until
> DOMContentLoaded anyway)

Confirmed we see attributeChangedCallback fire before connectedCallback for normal HTML pages:

data:text/html,<script>customElements.define("my-radio", class Radio extends HTMLElement {constructor() {super();console.log(`Radio constructor ${this.id}`);}static get observedAttributes() {return ["foo"];}attributeChangedCallback(name, oldValue, newValue) {console.log(`Radio attributeChangedCallback ${name} ${oldValue} ${newValue}`);}connectedCallback() {console.log(`Radio connectedCallback ${this.id}`);}});</script><my-radio foo="bar"></my-radio>

So we'll need to generally guard against / handle that for our widgets. This is really similar to what we need to guard against when a CE is made with document.createElement, so checking `this.isConnected` in the attributeChangeCallback seems reasonable.

Comment 3

[Arshad Khan [:arshad]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > 1) In XHTML it the attributeChangedCallback is firing before
> > connectedCallback for downloadsSummaryProgress
> 
> > I'd like to see if (1) is behavior shared for normal HTML docs / web
> > content. We should likely guard against this case anyway to avoid
> > unnecessary work (and probably even defer all initialization until
> > DOMContentLoaded anyway)
> 
> Confirmed we see attributeChangedCallback fire before connectedCallback for
> normal HTML pages:
> 
> data:text/html,<script>customElements.define("my-radio", class Radio extends
> HTMLElement {constructor() {super();console.log(`Radio constructor
> ${this.id}`);}static get observedAttributes() {return
> ["foo"];}attributeChangedCallback(name, oldValue, newValue)
> {console.log(`Radio attributeChangedCallback ${name} ${oldValue}
> ${newValue}`);}connectedCallback() {console.log(`Radio connectedCallback
> ${this.id}`);}});</script><my-radio foo="bar"></my-radio>
> 
> So we'll need to generally guard against / handle that for our widgets. This
> is really similar to what we need to guard against when a CE is made with
> document.createElement, so checking `this.isConnected` in the
> attributeChangeCallback seems reasonable.

downloadsSummaryProgress must have an attribute that you are observing, that's why it is getting called before connected callback. If you remove that attribute from xul/xhtml file then you ll see the connectedcallback will be called first. you can add the attr back after adding children if case observed attribute is mapped to some children property or attr.

Comment 4

[Arshad Khan [:arshad]]

> downloadsSummaryProgress must have an attribute that you are observing
downloadsSummaryProgress must have an attribute **set** that you are observing

Comment 5

[Arshad Khan [:arshad]]

if you are observing *label* attr of <custom_element /> then there are two cases when attributeChangedCallback will be called before connected callback -

1. if xul/xhtml files has custom_element defined like this -  `<custom_element label="..." />`. Solution is remove label attr from xul/xhtml markup and add it via js in connected callback.

2. if you are creating custom_element via js, then if you set label attribute before appending the custom_element to dom. Solution is to add the node to dom, then set the label attribute either in connected callback or some script.

Comment 6

[Brian Grinstead [:bgrins]]

Oh, fun. Apparently xhtml and html behave differently regarding the element's `this.isConnected` property and the initial attributeChangedCallback that fires before connectedCallback. In XHTML, `this.isConnected` is true and in HTML `this.isConnected` is false. Probably better to use the helper we added for `isConnectedAndReady` https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/toolkit/content/customElements.js#59 to determine whether we are getting this callback as a result of the parse.

HTML:
data:text/html;charset=utf-8,<head><script>customElements.define("my-radio", class Radio extends HTMLElement {static get observedAttributes() {return ["foo"];}attributeChangedCallback(name, oldValue, newValue) {console.log(`Radio attributeChangedCallback. this.isConnected: ${this.isConnected}`);}connectedCallback() {console.log(`Radio connectedCallback this.isConnected: ${this.isConnected}`);}});</script></head><body><my-radio foo="bar"></my-radio></body>

- Radio attributeChangedCallback. this.isConnected: false
- Radio connectedCallback this.isConnected: true

XHTML:
data:application/xhtml+xml;charset=utf-8,<!DOCTYPE html><html xmlns="http://www.w3.org/1999/xhtml"><head><script>customElements.define("my-radio", class Radio extends HTMLElement {static get observedAttributes() {return ["foo"];}attributeChangedCallback(name, oldValue, newValue) {console.log(`Radio attributeChangedCallback. this.isConnected: ${this.isConnected}`);}connectedCallback() {console.log(`Radio connectedCallback this.isConnected: ${this.isConnected}`);}});</script></head><body><my-radio foo="bar"></my-radio></body></html>

- Radio attributeChangedCallback. this.isConnected: true
- Radio connectedCallback this.isConnected: true

Comment 7

[Arshad Khan [:arshad]]

<head><script>customElements.define("my-radio", class Radio extends HTMLElement {static get observedAttributes() {return ["foo"];}attributeChangedCallback(name, oldValue, newValue) {console.log(`Radio attributeChangedCallback. this.isConnected: ${this.isConnected}`);}connectedCallback() {console.log(`Radio connectedCallback this.isConnected: ${this.isConnected}`); this.setAttribute("foo", "bar");}});</script></head><body><my-radio></my-radio></body>

Try ^, you ll get same result.

Comment 8

[Arshad Khan [:arshad]]

I can confirm that xul also has same behaviour as xhtml.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1497599 - Lazify progressmeter Custom Element connection;r=paolo

We usually don't want to run attributeChangedCallback logic until we are connected,
since we'll properly initialize the UI based on attribute values in connectedCallback.
However, in XML documents when attributeChangedCallback fires before connectedCallback
during parse, `this.isConnected` is true. `this.isConnectedAndReady` will make sure we
also early return in the parse case.

Comment 10

[Brian Grinstead [:bgrins]]

I've got a patch up for issue #1. Still looking into #2.

Comment 11

[Brian Grinstead [:bgrins]]

So why modfying the DOM of a progressmeter is causing the "tabs" XBL constructor to run is still a mystery to me. But when trying to call delayConnectedCallback() for the MozInputBox I got an error for cxmenu is undefined at: https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/browser/base/content/urlbarBindings.xml#143. This made me realize that we are processing pending connectedCallbacks _after_ XBL constructors run due to layout.

For the MozInputBox specifically, it probably doesn't make sense to delay connected callback since it always runs in XBL anon content anyway, but in general this seems like it could be a source of bugs. A couple of ideas:

1) Skip delaying connected callback for nodes in XBL anon content. Basically, return false here if document.getBindingParent isn't null: https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/toolkit/content/customElements.js#52
2) Process pending connected callbacks at MozBeforeInitialXULLayout (in XUL documents), which would make them run before their parent XBL constructors.

I need to do some performance testing and a bit more cleanup, but I'm leaning towards (2) at the moment. I sort of like (1) since it keeps XUL and XHTML documents more closely aligned, but we may end up needing to expose something like that event to top-level HTML anyway.

Comment 12

[Brian Grinstead [:bgrins]]

There's some previous discussion about MozBeforeInitialXULLayout inside XHTML in 1482448

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: Bug 1497599 - Process pending connectedCallbacks at MozBeforeInitialXULLayout when possible;r=paolo

This ensures that they run before XBL constructors that fire as a result
of initial layout. This makes it so those constructors can rely on Custom Elements
inside of their anonymous content to already have run their connectedCallbacks.

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: Bug 1497599 - Only run re-initialize moz-input-box during attributeChangedCallback if the attribute actually changed;r=paolo

This is unrelated to the other changesets in the bug, just a cleanup based on
patterns that have emerged in other attributeChangedCallbacks like in progressmeter

Comment 15

[Brian Grinstead [:bgrins]]

OK, so the progressmeter _initUI call sets `this.textContent = "";` which is somehow causing XBL construction to happen for the tabbrowser-tabs node. I think this is related to layout - if I comment out the rest of _initUI and just do `document.documentElement.style.display = "-moz-box";` the same error happens.

It's not clear why this only happens with the downloadsSummaryProgress, which is the second progressmeter to get connected.

1) `progressmeter id="addon-progress-notification-progressmeter">` is included in popup-notifications.inc and _does not_ cause XBL construction to happen: https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/browser/base/content/browser.xul#618.
2) `<progressmeter id="downloadsSummaryProgress">` is included in downloadsPanel.inc.xul (https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/browser/base/content/browser.xul#622) and _does_ cause XBL construction to happen.
3) Further down in the markup is: `<tab id="tabbrowser-tabs">`: https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/browser/base/content/browser.xul#762 which gets XBL construction after (2).

AIUI, in XUL documents we wait to do layout until the parse is complete (and "MozBeforeInitialXULLayout" happens), but in HTML documents we are able to do layout more incrementally. Emilio, is that right? Maybe this explains why we are seeing this.

We can fix this particular bug with the patches I already have up by waiting until DOMContentLoaded to process connectedCallback for progressmeter, but if this is what's happening I expect we'll continue to see this as we migrate more stuff to CE. I'm not sure what the 'right' thing to do here is - is there a way to suppress initial layout and act more like a XUL document? Or should we make sure to defer every connectedCallback/attributeChangedCallback until DOMContentLoaded (either in the CE impl or on the somehow platform side).

Comment 16

[Brian Grinstead [:bgrins]]

If what I think is happening in Comment 15 is right, then maybe if we had a way to suppress xbl-construction-due-to-layout in html docs from happening until after the parse is complete, that would provide a similar-enough environment to make things sane while we are still working with a mixed XBL/CE setup.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ca368cb3bbf
Lazify progressmeter Custom Element connection;r=paolo
https://hg.mozilla.org/integration/autoland/rev/8f52497c3d96
Only run re-initialize moz-input-box during attributeChangedCallback if the attribute actually changed;r=paolo

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Grinstead [:bgrins] from comment #15)
I'm not sure I totally grasped the problem you're describing, but inserting a XUL element inside another XUL element which has a layout box does cause us to create frames and load XBL, regardless of the load state of the document.

Additionally, anything that causes a layout flush in an HTML document will cause us to actually do layout.

I don't recall the specifics of XUL, but it should be easy to see if it behaves the same way if doing a layout query like document.documentElement.getBoundingClientRect() returns 0 before we're done parsing or not.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)
> (In reply to Brian Grinstead [:bgrins] from comment #15)
> I don't recall the specifics of XUL, but it should be easy to see if it
> behaves the same way if doing a layout query like
> document.documentElement.getBoundingClientRect() returns 0 before we're done
> parsing or not.

OK, if I log out `document.documentElement.getBoundingClientRect()` at https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/toolkit/content/widgets/progressmeter.js#89, then I see:

browser.xul:
DOMRect { x: 0, y: 0, width: 0, height: 0, top: 0, right: 0, bottom: 0, left: 0 }

browser.xhtml:
DOMRect { x: 0, y: 0, width: 1039, height: 492, top: 0, right: 1039, bottom: 492, left: 0 }

Not sure that this'd be a good idea, but I wonder if setting display:none on the documentElement and then unsetting it after the parse would achieve what Comment 16 suggests.

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #18)
> Not sure that this'd be a good idea, but I wonder if setting display:none on
> the documentElement and then unsetting it after the parse would achieve what
> Comment 16 suggests.

This does seem to work. Going to do some try pushes and see if that breaks anything.

Comment 21

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

FWIW, XUL loading is very different to (x)html. First XUL layout happens explicitly once we've loaded all the needed stuff.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #21)
> FWIW, XUL loading is very different to (x)html. First XUL layout happens
> explicitly once we've loaded all the needed stuff.

What do you mean by "loaded" here? Does that include fetching all subresources and XBL bindings?

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Ah, right. At least we wait for stylesheets, see the callers to XULDocument::DoneWalking:

  https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/dom/xul/XULDocument.cpp#1797

Comment 24

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/8ca368cb3bbf
https://hg.mozilla.org/mozilla-central/rev/8f52497c3d96