Closed Bug 404553 Opened 12 years ago Closed 12 years ago

[FIX]ASSERTION: Child not at the right index? with table, marquee, span and title

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file testcase
See testcase, I see this assertion in current debug trunk build, while loading the testcase.
###!!! ASSERTION: Child not at the right index?: '!aContainer || aContainer->Ind
exOf(aChild) == aIndexInContainer', file c:/mozilla-build/mozilla/content/xbl/sr
c/nsBindingManager.cpp, line 1547
Not an XBL bug.  The HTML parser is screwing up its indexing, and has ever since incremental HTML parsing landed in this edge case!

I wonder why marquee triggers it, though...
Component: XBL → DOM
Flags: in-testsuite?
OS: Windows XP → All
QA Contact: xbl → general
Hardware: PC → All
Attached patch FixSplinter Review
Attachment #289551 - Flags: superreview?(peterv)
Attachment #289551 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Summary: ASSERTION: Child not at the right index? with table, marquee, span and title → [FIX]ASSERTION: Child not at the right index? with table, marquee, span and title
Target Milestone: --- → mozilla1.9 M10
Attachment #289551 - Flags: superreview?(peterv)
Attachment #289551 - Flags: superreview+
Attachment #289551 - Flags: review?(peterv)
Attachment #289551 - Flags: review+
Comment on attachment 289551 [details] [diff] [review]
Fix

Fixes very long-standing bug with the content sink lying about where it's just inserted the content.  Luckily layout doesn't rely on the reported index too much, but other consumers do...
Attachment #289551 - Flags: approval1.9?
Attachment #289551 - Flags: approval1.9? → approval1.9+
Oh, the marquee triggers the assert because then there's XBL in the document, so we actually take this codepath....  Should be possible to write a fully stand-alone testcase for crashtest without marquee.
Checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attached file testcase2
Well, this one is without marquee, but uses embed typ="unknown", so not sure if this is useful.
I was thinking something that uses a div with an explicitly attached (via -moz-binding) binding...  The <embed> thing only works in Firefox (due to pluginfinder), and it'd be nice to have a generic Gecko test....
Well, I tried that (and more), but I can't seem to trigger the assertion that way.
Hmm.  What binding did you use?  I think it'd need to be a chrome binding; I can't think of a way to force a content binding load to complete expeditiously enough in this case...
I tried an external empty binding, an empty binding as a data url and the embed binding (plugin placeholder binding) as a data url. And I think I tried the same for the marquee case.
Yeah, data: URI bindings won't work, since they load async.  Is there a chrome binding that we can assume is always there for test purposes?
I don't think the marquee or plugin binding are going away any time soon, but I guess it's bad practice to rely on that?
Yeah...  I guess relying on the marquee binding is OK if we also add a test (mochitest?) that checks that <marquee> is implemented with a binding and points out in the comments that if it's not this test needs to be changed.  Does that sound reasonable?
Attached patch reftestSplinter Review
Ok, this is a reftest that triggers the assertion.
By making it compare against the reference, I'm assured that the marquee binding is applied (unless marquee will be hardcoded in Gecko, which I don't see coming soon).
Attachment #291546 - Flags: review?(bzbarsky)
(Although, I guess the parser could change which could make the test useless, anyhow)
Comment on attachment 291546 [details] [diff] [review]
reftest

Looks fine.  We might want to add this to crashtest, not reftest, right?
Attachment #291546 - Flags: review?(bzbarsky) → review+
Comment on attachment 291546 [details] [diff] [review]
reftest

Or at least document that this is an assertion test so it'll get moved once we have crashtest.
RCS file: /cvsroot/mozilla/layout/reftests/bugs/404553-1.html,v
done
Checking in 404553-1.html;
/cvsroot/mozilla/layout/reftests/bugs/404553-1.html,v  <--  404553-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/404553-1-ref.html,v
done
Checking in 404553-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/404553-1-ref.html,v  <--  404553-1-ref.htm
l
initial revision: 1.1
done
Checking in reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.250; previous revision: 1.249
done

I added a comment that this is an assertion test, I don't know of any crashtest yet. But note that the reference rendering is needed to know that the marquee binding has been applied.
Flags: in-testsuite? → in-testsuite+
(In reply to comment #18)
> I added a comment that this is an assertion test, I don't know of any crashtest
> yet.

See bug 397725 for more information.
(In reply to comment #19)
> See bug 397725 for more information.

Yes, thanks, I knew that, but that hasn't been checked in yet, afaik.

I had to disable the reftest:
Checking in reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.251; previous revision: 1.250
done
..because MacOSX Darwin 8.8.4 bm-xserve11 Dep Debug + Leak Test and MacOSX Darwin 8.8.4 qm-xserve01 dep unit test were failing.
I see: REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/bugs/404553-1.html
I don't really know how to fix my reftest, because it doesn't fail for me here, last time I checked.
Flags: in-testsuite+ → in-testsuite?
> But note that the reference rendering is needed to know that the marquee
> binding has been applied.

Then we probably want to document that in the comment, so the test ends up in both places...

I'll take a look at this test on Mac.
Thanks, Boris.
The reftest is rather similar to the one in bug 336736. There was also a Mac reftest issue, see bug 336736, comment 28 and comment 29.
Maybe that is basically the same issue here?
It was fixed there by changing the reftest, so I guess I should do that too?
Checking in 404553-1.html;
/cvsroot/mozilla/layout/reftests/bugs/404553-1.html,v  <--  404553-1.html
new revision: 1.2; previous revision: 1.1
done
Checking in 404553-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/404553-1-ref.html,v  <--  404553-1-ref.htm
l
new revision: 1.2; previous revision: 1.1
done
Checking in reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.262; previous revision: 1.261
done

Ok, I now update the test with the suggestion in 336736, comment 32. Hopefully, it sticks this time. According to that comment, it isn't necessarily a bug on the Mac, btw.
I also added this comment in reftest.list as suggested in comment 21:
# assertion test, also tests that marquee binding is applied correctly
Flags: in-testsuite? → in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.