985549 - xul:iframe sometimes fires load event before it's contentWindow's xbl property binding is applied

Status: NEW

(Core :: XUL, defect)

Description

[Alexandre Poirot [:ochameau] [PTO back August 21]]

We are hitting this race pretty badly in devtools as we are (over) using xul iframes for all panels.
It happens to throw various exception when you stress a bit the UI because of any iframe property being defined in the xbl binding to be undefined:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/general.xml#86

I do not have any small script to reproduce this issue, but I verified it.
I can get the contentWindow using the same code we have in the XBL.
And the "load" event fires correctly, when the document is loaded (readyState=complete).

var iframe=document.createElement("iframe");
iframe.addEventListener("load",function (evt) {
  // Here iframe.contentWindow is undefined,
  // whereas evt.target.readyState is complete
  // and iframe.boxObject.
                QueryInterface(Ci.nsIContainerBoxObject).docShell
  // is correctly defined.
},false);
iframe.setAttribute("src", "something");
document.documentElement.appendChild(iframe);
  

Is there any way to implement these few very simple attributes in C++?
I'm willing to patch this if that's reasonably simple to fix,
in order to prevent even worse workaround all over devtools codebase!

Comment 1

[Mike Hommey [:glandium]]

It sounds like it is unfortunate that there's a race between the iframe load event being emitted and the bindings from xul.css being applied. Neil, is that really expected or something that should be fixed?

Comment 2

[Neil Deakin]

You'll want to set the src attribute after appending it to the document.

Comment 3

[Alexandre Poirot [:ochameau] [PTO back August 21]]

That doesn't prevent such race. We are already setting the src after the appendChild.

Also I'm not looking for workarounds that would require modifying all the chrome code using xul:iframe.
I already have various JS workarounds that do work fine, I just would like to avoid having workarounds all over devtools/browser codebase because of this.

I know almost nothing about xul/xbl internals. The only way I could imagine would be to add contentWindow to nsIDOMXULElement and implement it in nsXULElement.cpp. I know that's bad as it would add this attribute to all xul elements... but really I can't think about any other way to fix that at the root. Also I'm seeing XULElement.webidl, as webidl are way more flexible than idl, would there be something to do here? (Did we already switched to webidl for xul or is this a WIP?)

Comment 4

[Neil Deakin]

(In reply to Alexandre Poirot (:ochameau) from comment #3)
> That doesn't prevent such race. We are already setting the src after the
> appendChild.

That's not what the code you provided does.

Comment 5

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Neil Deakin from comment #4)
> (In reply to Alexandre Poirot (:ochameau) from comment #3)
> > That doesn't prevent such race. We are already setting the src after the
> > appendChild.
> 
> That's not what the code you provided does.

Sorry about that, that was just a pseudo code. As said, I do not have minimal test script to reproduce this issue. There is most likely something special being done in devtools codebase to get into this race. But it occurs in different iframes we are using, at least the one used in the inspector and another in the style editor. It is pretty challending to reduce, but it is very easy to reproduce with production code as described in bug 982710.

Anyone to ping to get some hints?

Comment 6

[Alexandre Poirot [:ochameau] [PTO back August 21]]

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> is very easy to reproduce with production code as described in bug 982710.

I meant bug 985555.

Comment 7

[Alexandre Poirot [:ochameau] [PTO back August 21]]

I've been able to workaround this in bug 985555, in the inspector by not hidding the container.
But in bug 982710, I'm not sure we can do the same, even if that ends up being the same issue.
In this bug, in the style editor, we end up creating an iframe in a toolbox that isn't visible on purpose, as we are in another UI.

Comment 8

[Quest]

I had a similar problem when both contentWindow and contentDocument were undefined, and I found the possible culprit: previously I placed two overlays inside one <overlay>, as below:

<overlay id="stockbot-sbtoolbar"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        xmlns:html="http://www.w3.org/1999/xhtml">
  
  <toolbar id="PersonalToolbar">
    <!-- Here goes the main UI -->
  </toolbar>
  
  <vbox id="browser-bottombox">
    <!-- Here go the iframes -->
  </vbox>
</overlay>

The contentWindow and contentDocument in the iframes were undefined. So I experimented by inserting an iframe inside the <toolbar> block, and I could get the contentWindow and contentDocument of this iframe. I have no idea how it works in the background, but I will just leave it here.