Closed Bug 406900 Opened 14 years ago Closed 14 years ago

[FIX]"ASSERTION: Should be in an update while creating frames" with XBL, <xul:listitem>


(Core :: XUL, defect, P3)






(Reporter: jruderman, Assigned: bzbarsky)


(Blocks 1 open bug)


(Keywords: assertion, testcase)


(3 files, 3 obsolete files)

Attached file testcase
Loading the testcase triggers:

###!!! ASSERTION: Should be in an update while creating frames: 'mUpdateCount != 0', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8611

Previous bugs with this assertion:
* Bug 398326, fixed in October
* Bug 398492, fixed earlier today.
Flags: blocking1.9?
We're appending a child to a node for which:

(gdb) p aParent->IsInDoc()
$12 = 0
(gdb) p aParent->GetParent()->IsInDoc()
$13 = 1

That would be broken...  As a result we don't call Begin/EndUpdate, but aNotify is true, so we call nodeutils, and it finds the document on the parentnode chain and notifies it.

aParent is a <listcell> in this case.
The issue is that there is a <children> tag in the default content.  So when ChangeDocumentForDefaultContent is called, it unbinds this <children> tag.  But the <children> has a <listcell> child whose parent is the <listitem>.  So we never clear the parent on that child.  That's buggy. Or the setup with the parent not being <children> is buggy.  Or something.
The binding looks like:

878     <content>
879       <children>
880         <xul:listcell xbl:inherits="label,crop,disabled,flexlabel"/>
881       </children>
882     </content>

It looks to me like we should enumerate mInsertionPointTable and unbind the default content or something (basically undo what RealizeDefaultContent does).
Er... Except that's what ChangeDocumentForDefaultContent is trying to do.  The problem is that we need to unbind the _kids_ of the default content (as well as the default content itself), since the kids are what we bound in InstallAnonymousContent, as called from RealizeDefaultContent.
Attached patch Like so (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Attachment #291599 - Flags: superreview?(jonas)
Attachment #291599 - Flags: review?
Attachment #291599 - Flags: review? → review?(Olli.Pettay)
Summary: "ASSERTION: Should be in an update while creating frames" with XBL, <xul:listitem> → [FIX]"ASSERTION: Should be in an update while creating frames" with XBL, <xul:listitem>
Target Milestone: --- → mozilla1.9 M11
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Could you perhaps add UnbindDefaultContent method to InsertionPoint and
call that in cases where UnbindFromTree is called currently?
I could, but what are these other cases?
I was thinking nsXBLBinding::RemoveInsertionParent and RemoveInsertionParentForNodeList
Oh, huh.  _That_'s where that code lives!

It looks to me like the caller of RemoveInsertionParent() in ChangeDocumentFor() should really be in ContentRemoved(), no?  As things stand, it won't get called in all the cases it should get called in.

Want a bug on that?
Filed bug 407649 on comment 9.
Attachment #291599 - Attachment is obsolete: true
Attachment #292366 - Flags: superreview?(jonas)
Attachment #292366 - Flags: review?(Olli.Pettay)
Attachment #291599 - Flags: superreview?(jonas)
Attachment #291599 - Flags: review?(Olli.Pettay)
Attachment #292366 - Flags: review?(Olli.Pettay) → review+
Attachment #292366 - Flags: superreview?(jonas) → superreview+
Checked in the fix.

Jesse, do you think you can create a standalone crashtest that doesn't depend on the exact XUL language bindings we use?  If you don't have time to work on this, I can try to...
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Attached file standalone crashtest (obsolete) —
Like this?  Any reason why I need to use setTimeout rather than document.documentElement.offsetHeight?
Hmm, that testcase triggers the assertion for me when I load it locally but not when I load it from Bugzilla.
Attached file with a longer timeout (obsolete) —
I can reproduce even loading it from Bugzilla with the longer timeout.  Is XBL being silly and requesting another copy of the entire file from Bugzilla, making it timing-dependent?
Per bz's suggestion, avoid the timing issue entirely by moving the stuff done by the first chunk of JavaScript into the markup.  This lets the testcase use onload to know when the XBL is done.
Attachment #299908 - Attachment is obsolete: true
Attachment #299909 - Attachment is obsolete: true
Last attachment checked in as a crashtest.
Flags: in-testsuite? → in-testsuite+
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.