526178 - binding loading order changed in 3.6b1 compared to previous version of firefox

Status: RESOLVED FIXED

(Core :: XBL, defect, P2)

Description

[Franck Stauffer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.2b1) Gecko/20091029 Firefox/3.6b1 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.2b1) Gecko/20091029 Firefox/3.6b1 (.NET CLR 3.5.30729)

We have the following DOM (simplified testcase: http://www.rbs.fr/static/test.xul):
<bindinga>
 <bindingb />
</bindinga>
where bindinga and bindinga are mapped to custom bindings using the -moz-binding CSS property:
* In firefox 3.5.4, bindinga gets constructed before bindingb.
* In firefox 3.6b1, bindingb gets constructed before bindinga.

This is a major change in the way bindings get loaded, which breaks a lot code that was written based on the current loading mechanism (parent gets constructed before children).


Reproducible: Always

Steps to Reproduce:
1. Load http://www.rbs.fr/static/test.xul 
Actual Results:  
Two alerts : constructing B then constructing A

Expected Results:  
Two alerts : constructing A then constructing B

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like this behavior changed between 2009-03-05-02 (rev 5e81fbdd55e4) and 2009-03-06-02 (rev 3270fd0db0f0).

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e81fbdd55e4&tochange=3270fd0db0f0

Likely a change from bug 480979.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, so the old code flow was that we'd process all the kids for the frame _then_ call AddToAttachedQueue (misnamed, since it's actually adding to a stack) on the binding of the frame.  So the constructor for the parent would run before the constructors for the kids.

The new code flow is that the order actually depends on the display type; the binding is enqueued from AddFrameConstructionItemsInternal, which usually does NOT recurse into kids, but might for inline frames.

It's a little sad-making that we didn't have a test for this.  :(

Of course this raises the interesting question of what we do now that we shipped b1 this way...  Its pretty easy to restore the old behavior; will put up a patch for it tomorrow (well, later today).

Comment 3

[Johnny Stenback (:jst)]

This is a regression since Firefox 3.5, blocking. bz is working on a patch for this, initial ETA later today.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  So the two behaviors we need to preserve from 1.9.1, presumably, are:

1)  Sibling constructors fire in reverse order, even if some of the nodes are
    display:none.
2)  Ancestor constructors fire before descendant constructors.

So effectively the constructors fire in a depth-first preorder traversal of the DOM, with child lists walked backwards...

In terms of calling AddToAttachedQueue(), that means it needs to be called in a depth-first post-order traversal of the tree, walking child lists normally.

The display:none thing means that we can't just store the binding to call AddToAttachedQueue on in the frame construction item, which is what I was initially planning to do.  If we relax condition 1 above for display:none nodes, then we could go with that plan.  If not, I need to think about the right data structure to use here...  It's probably better to preserve compat for now, I guess.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and ETA is pretty definitely tomorrow afternoon (since I'm taking tomorrow morning off as a half-belated-birthday kind of thing).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

The idea is described in the big comment in nsCSSFrameConstructor.h.  Most of the patch is passing around the relevant nsBindingTracker; the fact that some of the construction functions (e.g. ConstructBlock or the select stuff) don't take FrameConstructionItems made this a bit more annoying.

I made sure that native anonymous content gets the same treatment, with one exception: scrollframes.  Their native anonymous content is somewhat inconsistent anyway, and the ordering there shouldn't matter.  I can add some more code to handle that if desired, but I don't think we need that.

This passes the content/xbl tests; I pushed to try for the rest.

If desired we could easily allocate nsBindingTracker out of the presshell arena; I just figured it wasn't worth the hassle.  If this seems to be a Txul hit, I'll reconsider.

roc, I think beltzner wants this on 1.9.2 sometime tomorrrow.  :(

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 410343 [details] [diff] [review]
Patch

+      // No parent anything here with pending binding ctors

I can only barely parse this, please rephrase.

Would it make more sense to call nsBindingTracker nsPendingBinding? Or just PendingBinding since this is file scope.

+  nsBindingTracker* bindingTracker = nsnull;

Make this an nsAutoPtr and use .forget() when calling AddBindingTracker?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I can only barely parse this, please rephrase.

How about:

      // We don't have a parent frame with a pending binding constructor here,
      // so no need to worry about ordering of the kids' constructors with it.
      // Pass null for the PendingBinding.

> Would it make more sense to call nsBindingTracker nsPendingBinding? Or just
> PendingBinding since this is file scope.

Yeah, ok.  I can do that.  It's not quite file scope since it's mentioned in the header, but good enough.

> Make this an nsAutoPtr and use .forget() when calling AddBindingTracker?

I need the pointer after that point in the function.  I suppose I could have a separate variable for that, and use an nsAutoPtr around the allocation to keep track of ownership and simplify logic (and set the variable right before adding to the state).  I'll do that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/2fa27d8cd3d2

Comment 11

[Boris Zbarsky [:bzbarsky]]

Backed out due to browser-chrome failures in browser_sanitizeDialog.js:

http://hg.mozilla.org/mozilla-central/rev/6258b513a554

It seems to be confused about whether the expander is actually open and whether the warning panel should be appearing...  I have no idea yet whether this is a real issue or a bug in the test (though I can't see any problems when manually opening the dialog in question, so I suspect bug in the test).  Will investigate tomorrow.

Comment 12

[(no longer active)]

I didn't have any ideas after a quick look; maybe Drew has some?

Comment 13

[Drew Willcoxon :adw]

It's the last test that's failing, the one made up of six functions testing persisting the details expansion.  Investigating...

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  With Drew's help, I think I know what's up here.  The problem is that the sanitize dialog is not ending up sizing intrinsically.  This happens because its nsXULWindow::mIntrinsicallySized is false, and _that_ happens because nsXULWindow::SetSize gets called before nsXULWindow::OnChromeLoaded.

The caller of SetSize is a resizeBy call from JS, with this JS stack:

1 anonymous() ["chrome://browser/content/sanitizeDialog.js":114]
    warningBox = [object XULElement @ 0x1a9a4990 (native @ 0x1a9bbf10)]
    this = [object Object]
2 onselect(event = [object Event @ 0x1989dfd0 (native @ 0x1989df90)]) ["chrome://brow
ser/content/sanitize.xul":1]
    this = [object XULElement @ 0x1ed50830 (native @ 0x19858050)]
3 set_selectedItem(val = [object XULElement @ 0x19868980 (native @ 0x1a9bdf30)]) ["ch
rome://global/content/bindings/menulist.xml":249]
    event = [object Event @ 0x1989dfd0 (native @ 0x1989df90)]
    oldval = [object XULElement @ 0x1a92e700 (native @ 0x197de430)]
    this = [object XULElement @ 0x1ed50830 (native @ 0x19858050)]
4 set_value(val = 0) ["chrome://global/content/bindings/menulist.xml":101]
    popup = [object XULElement @ 0x1ac101f0 (native @ 0x197e64c0)]
    arr = [object HTMLCollection @ 0x19630880 (native @ 0x198ed740)]
    this = [object XULElement @ 0x1ed50830 (native @ 0x19858050)]
5 setValue(value = 0, attribute = "value", element = [object XULElement @ 0x1ed50830 
(native @ 0x19858050)]) ["chrome://global/content/bindings/preferences.xml":423]
    this = [object ChromeWindow @ 0x197c3870 (native @ 0x197af510)]
6 setElementValue(aElement = [object XULElement @ 0x1ed50830 (native @ 0x19858050)]) 
["chrome://global/content/bindings/preferences.xml":440]
    setValue = [function]
    val = 0
    f = undefined
    event = undefined
    rv = undefined
    this = [object XULElement @ 0x1eef0580 (native @ 0x198e8280)]
7 updateElements() ["chrome://global/content/bindings/preferences.xml":524]
    i = 0
    elements = [object HTMLCollection @ 0x19630840 (native @ 0x197c9cb0)]
    this = [object XULElement @ 0x1eef0580 (native @ 0x198e8280)]
8 _setValue(aUpdate = false, aValue = 0) ["chrome://global/content/bindings/preferences.xml":168]
    this = [object XULElement @ 0x1eef0580 (native @ 0x198e8280)]
9 () ["chrome://global/content/bindings/preferences.xml":118]
    l = undefined
    parentPrefs = undefined
    k = undefined
    parentPreferences = undefined
    preference = undefined
    pdoc = undefined
    this = [object XULElement @ 0x1eef0580 (native @ 0x198e8280)]

This is the <preference> constructor running, reading the privacy.sanitize.timeSpan preference and calling this._setValue on the <preference> element.  That goes and updates the elements observing the preference, which in this case is a <menulist>.  The menulist's value setter triggers its onselect handler, which calls gSanitizePromptDialog.selectByTimespan() which makes the resizeBy call.  This code was added in bug 480169, which did land on 1.9.1, but didn't land on trunk until after bug 480979...

Note that the failing part of the test (the last 6 entries in the test array) was largely added post-1.9.1 (in bug 488691).

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, I changed frame construction to fix this bug (so fire parent constructors before children) but not restore the 3.5 behavior otherwise (so construct children in order, not reverse order) and I still get the sanitize failure.

If I do children before parents, children in reverse order, I do NOT get the sanitize failure.

Comment 16

[Boris Zbarsky [:bzbarsky]]

In other words, the sanitize test is somehow actually depending on the buggy behavior.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, a bit more digging... the first failing test is the first one that calls checkDetails after the sanitize.timeSpan pref has been set to Sanitizer.TIMESPAN_EVERYTHING.  The test before that one also has the bogus SetSize call, but doesn't ever check the window size, so passes.

And the reason for the behavior change is that the <prefwindow> constructor fires paneload on the prefpane here (because it has no src=""), so that calls gSanitizePromptDialog.init() from he prefwindow constructor.  selectByTimespan(), which is what makes those resizeBy() calls, is a no-op if init() hasn't been called yet.

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK, if on 1.9.1 I change selectByTimespan to not pay attention to whether init() has been called, I get failures there too.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Aaaand on 1.9.1 the <preference> constructors fire _before_ the <prefwindow> constructor does.

Comment 20

[Boris Zbarsky [:bzbarsky]]

And _that_ is because the parent's constructor fires before the children _unless_ the parent is the root.  In that case it fires after.  In 1.9.1, that is.

And that behavior dates back to when hyatt initially made constructors run on the root at all, back in bug 73137.

The good news is that this simply means I need to take out the ConstructDocElementFrame parts of this patch.  Will post updated patch for relanding, but after dinner.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: With those changes

This just adjusts the test to test the root element too, and reverts the changes to ConstructDocElementFrame, replacing them with comments about the screwed-up-behavior.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Pushed that last patch as http://hg.mozilla.org/mozilla-central/rev/77512fa7701e

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.2 branch merge

Comment 24

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c598f579bf98

Comment 25

[Boris Zbarsky [:bzbarsky]]

Somehow I failed to remove AutoEnqueueBinding in these patches.  Just pushed http://hg.mozilla.org/mozilla-central/rev/d859fbeb4fca to remove that on m-c; not going to worry for 1.9.2.