Closed Bug 495354 Opened 15 years ago Closed 15 years ago

"ASSERTION: Must have parent here" with <xul:caption>, XBL, {ib}

Categories

(Core :: XBL, defect)

Product:
Core
Component:
XBL
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Unassigned)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files)

testcase
436 bytes, application/xhtml+xml
Details
Slight modification of the testcase to show the ensuing rendering bug
452 bytes, application/xhtml+xml
Details
testcase that shows the rendering issue is not really a regression
484 bytes, application/xhtml+xml
Details
Attached file testcase — 
###!!! ASSERTION: Must have parent here: 'aContent->GetParent()', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 4726

###!!! ASSERTION: Reparenting something that has no usable parent? Shouldn't happen!: 'Not Reached', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 947

The first assertion was added last week in bug 78070.  The second assertion is much older.

The testcase doesn't show it, but I think this can lead to:

###!!! ASSERTION: undisplayed content must have a parent, unless it's the root content: 'parent || (mPresShell && mPresShell->GetDocument() && mPresShell->GetDocument()->GetRootContent() == aContent)', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 602
So the fundamental problem is that when we do the frame reconstruct, the nsXBLInsertionPoint for the <xul:caption> contains two nodes in it: the <div> and the textnode containing the string "9".  The former is the issue: it should no longer be there, and when we go to resolve style for it we end up with the assertions above.
OK, the rendering regressed on m-c between 2009-01-29-02 (rev e95fb8b811ac) and 2009-01-30-02 (rev 76540a65adc0).

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e95fb8b811ac&tochange=76540a65adc0
Keywords: regression
Oh, in comment 1 I mean the insertion point that gets used by the childiterator for the <caption>, of course.
What changed in the range from comment 3 is we started rendering the text of the div, not just it itself (and stopped asserting about it not being in the document), due to this change:

     6.3 @@ -58,15 +58,14 @@ ChildIterator::Init(nsIContent*    aCont
     6.4    if (! aContent)
     6.5      return NS_ERROR_NULL_POINTER;
     6.6  
     6.7 -  nsCOMPtr<nsIDocument> doc = aContent->GetDocument();
     6.8 +  nsIDocument* doc = aContent->GetOwnerDoc();
     6.9    NS_ASSERTION(doc, "element not in the document");
    6.10    if (! doc)
    6.11      return NS_ERROR_FAILURE;

I dug around some more, and what's going on here is that we do properly remove the <div> from the insertion point... but this is from the one we get from mAnonymousNodesTable.  After that, this insertion point is empty.  So when we go to add the text, we call nsBindingManager::GetXBLChildNodesInternal, get an empty nodelist from GetAnonymousNodesInternal, and stick the text at the end of the insertion point we get from mContentListTable.  Unfortunately, that insertion point already contains the <div>!

Now it is in fact correct that the <caption> has entries in both tables (since it has a <children> kid _and_ has a binding attached to it whose <content> has a <children> direct child).  But we shouldn't be falling back on the mContentListTable list while handling DOM mutations in the binding manager.

In fact, now that I think about it, I'm not sure why the length==0 fallback is there at all...  This is in nsBindingManager::GetXBLChildNodesInternal.  Neil, any idea?
Oh, and the point is that we used to assert back then too; the "not in document" assert and then the "Reparenting something that has no usable parent?" assert.
The checkin comment for that length check is oh-so-enlightening:

1.44 <waterson@netscape.com>  2001-02-19 17:05
Bug 39054, redux. Hack around problem (?) with XBL child nodes: we'll just query for the list of real kids up front. Also, need to break 'AddSubtreeToDocument()' into pre- and post-order steps. r=hyatt, sr=brendan
And that just moved the code.  It was added in:

1.554 <hyatt@netscape.com>  2001-02-01 16:54
Fix for 55292, r=ben, sr=brendan
So obvious thoughts:

1) Removal of a node should remove it from both the mAnonymousNodesTable list and
   the mContentListTable list.
2) Insertion of a node should add it to both lists.  This is somewhat less
   important to me, in some ways, because the worst case here shouldn't be
   unexpectd nodes popping up in layout...

Ideally, with nested insertion points, we'd do this for all the insertion points in the chain, but I'm really not that interested in finally rewriting insertion points to be sane in this bug; I believe sicking will be tackling that very soon.

Which brings me to the obvious question... is it worth fixing removal and insertion per items 1 and 2 above, or should we just make this code sane in general and not worry about this for now?  We're no worse off on trunk than we are on 1.9.1 and 1.9.0, I believe.
Flags: blocking1.9.2?
Certainly that last testcase shows the green box in 1.9.0.
1. I'm not sure why there are two lists. Is it to deal with the case of (a) foo has a real baz kid, plus a binding whose content is bar, whose content is baz plus foo's kids (b) bar also has a binding, whose content is baz plus bar's kids? (So bar's frame ends up with three baz child frames.)
2. I have no objection to removing nodes from both lists, though as per 1. I don't see how they get there.
3. What would be the minumum differences necessary to the testcase to produce a length > 0 and therefore not exhibit the bug?
> 1. I'm not sure why there are two lists.

This is actually well-documented in nsBindingManager.h now.  One list is there because the node is in the anonymous content of a binding and has a <children> child.  That list represents the list of its actual kids in the binding and all the kids of the bound element for that binding that got put inside that <children>.  The other list is because the node has a binding attached to it and that binding has a <children> as a direct child of <content>.  This list represents all the nodes that are direct kids of that <content> and the nodes that end up inside that <children>...  If the latter list ends up empty we use the former.  I can't tell you why, though.

I can't quite tell whether the above matches your "deal with the case".

> I don't see how they get there.

I hope the above makes it clearer...

> 3. What would be the minumum differences necessary

Given the use of SetInnerHTML, which kills off all the <span>'s kids, the only way to end up with a length > 0 during the append is to have the <caption> in the testcase have another child in addition to the <children>.
To clarify my "case":
foo
  xbl:content
    bar
      baz
      xbl:children
bar
  xbl:content
    baz
    xbl:children
foo
  bar (anonymous child of foo)
    baz (anonymous child of bar) [1]
    baz (anonymous grandchild of foo, real child of bar) [2]
    baz (real child of foo) [3]
I take it baz[1] belongs in the anonymous list, and baz[2] in the content list; I'm unclear as to which list baz[3] belongs in, but it only belongs in the one?

Anyway, given the rest of your answer, the length==0 fallback does look wrong.
Blocks: 429623
Blocks: 463511
I'm having a hard time judging whether this needs to block 1.9.2 or not. Any thoughts?
Given the new schedule, I would think no.  I _think_ this shouldn't be a security problem, and Jonas is working on insertion point stuff sanity for XBL2 anyway.
Not blocking per previous comment.
Flags: wanted-next+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Depends on: 514300
The patch in bug 514300 implements comment 9 item 1, and fixes all three testcases in this bug.
The patch in bug 514300 landed.  Do we want to keep this bug open for getting rid of that length check?
No, please file a new bug for that.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
in-testsuite+: the patch in bug 514300 included testcases from this bug.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: