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

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase2

Well, this one is without marquee, but uses embed typ="unknown", so not sure if this is useful.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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....

Comment 8

[Martijn Wargers (dead)]

Well, I tried that (and more), but I can't seem to trigger the assertion that way.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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...

Comment 10

[Martijn Wargers (dead)]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Martijn Wargers (dead)]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Martijn Wargers (dead)]

Attachment: reftest

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).

Comment 15

[Martijn Wargers (dead)]

(Although, I guess the parser could change which could make the test useless, anyhow)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 291546 [details] [diff] [review]
reftest

Looks fine.  We might want to add this to crashtest, not reftest, right?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Martijn Wargers (dead)]

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.

Comment 19

[Reed Loden [:reed]]

(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.

Comment 20

[Martijn Wargers (dead)]

(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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 22

[Martijn Wargers (dead)]

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?

Comment 23

[Martijn Wargers (dead)]

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