334430 - [FIX]nsXBLService::LoadBindings causes reentry into frame construction

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsXBLService::LoadBindings, called from ConstructFrameInternal (which can be arbitrarily deep within a frame construction entry point, unlike the calls to ProcessAttachedQueue which are always made from the entry point), sometimes causes reentry into frame construction.  Because of this, I had to make one of the assertions added for bug 310985 a warning instead.

Stack coming shortly.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack of first assertion opening Firefox options dialog

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 218792 [details]
stack of first assertion opening Firefox options dialog

Oops, sorry, that stack was actually a different bug; I didn't look closely enough.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: stack of second assertion opening Firefox options dialog

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 334450 on the first stack.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So why are we passing PR_TRUE for aNotify here?  I'd think we want to pass PR_FALSE.  I'm looking, in particular, at:

          textContent->SetText(value, PR_TRUE);
          realElement->AppendChildTo(textContent, PR_TRUE);

This looks like it's handling xbl:inherits="xbl:text=foo" and xbl:inherits="value=foo" (the latter only for <xul:html> nodes).

Is there something we should test to make sure it doesn't break if we're going to twiddle this code?

Comment 6

[[e-mail]]

I think checkboxes and radiobuttons use xbl:text=label although offhand I don't know of an example of changing a checkbox or radio button label.

Out of interest, whose job would it be to create the frame(s) for the new text?

Comment 7

[Boris Zbarsky [:bzbarsky]]

The frame constructor's.  Changing is a different thing; we're lookin at nsXBLPrototypeBinding::SetInitialAttributes here.  There are no frames anywhere in sight for this stuff.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should forbid nsIDocumentObservers to cause mutations to the DOM since that can cause a whole set of issues similar to the ones we have with mutation events.

A simple solution would be to wait until EndUpdate is called (enough times) and then perform the mutation.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That's a separate issue from this bug.  But yeah, that would be nice....

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So don't we want to use PR_TRUE there? To get layout to update.

Comment 12

[Boris Zbarsky [:bzbarsky]]

What layout?  This is just-installed anonymous content for a binding we're still setting up.  There is no layout.

We do use PR_TRUE in AttributeChanged, and we should file a separate bug on making that not suck.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 218893 [details] [diff] [review]
Proposed patch

ah, ok. r/sr=sicking

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed.