342954 - [FIX]"ASSERTION: SetMayHaveFrame failed?" involving XBL

Status: RESOLVED FIXED

(Core :: XBL, defect, P3)

Description

[Jesse Ruderman]

Using a Mac trunk build with Smaug's patch in bug 205735 comment 44, loading the testcase causes a few assertions and warnings, including:

###!!! ASSERTION: SetMayHaveFrame failed?: 'mContent->MayHaveFrame()', file /Users/admin/trunk/mozilla/layout/generic/nsFrame.cpp, line 547

WARNING: Content has no document.: file /Users/admin/trunk/mozilla/layout/generic/nsTextFrame.cpp, line 5937

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I didn't see those. I'll check those.

Comment 2

[Jesse Ruderman]

Attachment: testcase

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

The ASSERTION and WARNING are there even without the patch for
bug 205735.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for testing.

This removes the assertion and warning, but the real question is that
why we're trying to create a frame for a node which is not in document.
(I'm not proposing this patch as a fix)

Comment 5

[Boris Zbarsky [:bzbarsky]]

So we end up reframing the whole thing after we're done with our DOM mutations; that's when we notice things have gone wrong...

Would it be possible to minimize the XBL too?  I'm a little confused by this XUL element we're coming out with; I'd love to know where it comes from.  Scrollars or something?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> I'm a little confused by this
> XUL element we're coming out with; I'd love to know where it comes 
> from.  Scrollars or something?

What XUL element are you talking about?

Somehow XBL keeps a pointer to #text node which isn't anymore 
in document. Then when CSSFC tries to create frames for the
(old) parent of the #text, BindingManager()->GetXBLChildNodesFor
returns a list of nodes which contains also the #text

Comment 7

[Boris Zbarsky [:bzbarsky]]

> What XUL element are you talking about?

When I had this in a debugger last night, GetContent() on aParentFrame was a XUL element when we asserted.

> Somehow XBL keeps a pointer to #text node 

Right.  I just think it would be easier to debug this with a minimal binding.  ;)

Comment 8

[Jesse Ruderman]

Attachment: testcase with small xbl binding

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I don't see XUL anywhere.

Using the first testcase:
"content [#text] is not in doc"

Then parent chain, starting from aParentFrame
         \ content [div]
         \ content [div]
         \ content [div]
         \ content [marquee]
         \ content [body]
         \ content [html]

Comment 10

[Jesse Ruderman]

Attachment: similar testcase as two files (zip)

It's easier to reproduce the assertion locally (using the file: protocol) with this version.  I don't know why.

Comment 11

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

Won't block since this only asserts. But I'll take a look at it.

Comment 12

[Jesse Ruderman]

Still happens on trunk with the two-file testcase in comment 10.

Comment 13

[Jesse Ruderman]

FWIW, this still happens on trunk even with the patch for bug 405178.  (That bug also triggered the "SetMayHaveFrame failed?" assertion.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

This looks like more general breakage in the way XBL handles dynamic changes.  In particular, we keep track of the explict kids of an insertion parent (like the kid being removed here) in the nsAnonymousContentList* keyed by that parent.  They're represented by the insertion points with insertion index -1.  We don't update those points on document mutations.  But the frame constructor relies on that information being correct...

So in this case we're constructing a textframe for a textnode with a null parent, for example.

You can actually get things to mis-render this way.  For example, replace the

  removeNode(marqAnonymousSomething);

part of the testcase with:

  document.getAnonymousNodes(marq)[0]
          .appendChild(document.createTextNode("WHERE?"));

Notice that the text does not appear.  Notice that it _does_ appear if you either put the textnode in the binding to start with or remove the "s1.appendChild" call (which just serves to cause a top-down frame tree reconstruct in this case).

The right solution is to update the insertion point lists correctly, imo...  Doing this on removal might not be too bad, actually; doing it on insert is a little harder (have to insert in the right place).

I really wish we had a slightly better API for managing the flattened tree.  :(

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, getting this to work for content insertions is a bit of a huge pain.  I'm not sure I should bother, since it'll lead to wrong rendering but probably not crashes or anything like that.

For removes, I really don't like building frames for things that are not in the DOM, so I'll put up a fix.  But I need to clear out bug 398492 from my tree first--it's touching the same code.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, and if we do think that we should get insertions right, I can work on that.  the biggest part of the pain is actually writing the tests, not the code, so if I can get someone to volunteer to help with that, it would be quite nice.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Renominating for blocking, since this blocks a crash bug (and I could probably produce non-debug crashes here too).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This makes sure we don't leave frames pointing to nodes that are no longer in the tree.  We still don't handle insertions correctly, but this is a start....

Please review this carefully!  I dug around for other places where we might need a similar change, and didn't find them, but the number of hashtables and various other metadata we have hanging around is a bloody mess.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, and this still needs a test..  I haven't managed to write a sane mochitest or reftest yet, sadly.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 292365 [details] [diff] [review]
Proposed fix

>+  // <content> is used instead as a performance optimization.  There
>+  // is an entry for a content node in this table only of that content
>+  // node has a binding with a <content> attached and this <content>
>+  // contains <children> elements directly.
s/only of that/only if that/
I'll still re-read this. Tests would be more than great.

Comment 21

[Damon Sicore (:damons)]

+'ing w/ P3.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 292365 [details] [diff] [review]
Proposed fix

Should be ok, and because LookupObject is optimized using NODE_MAY_BE_IN_BINDING_MNGR bit, performance shouldn't be affected too much in normal cases.

Comment 23

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

Comment on attachment 292365 [details] [diff] [review]
Proposed fix

Yay, thanks for documenting those hashes!

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in the patch, and Jesse's tests as crashtests.  Filed bug 414305 on the insertion bug.

Comment 25

[Stuart Parmenter]

is this fixed?  if not, renominate

Comment 26

[Boris Zbarsky [:bzbarsky]]

Er, yeah.  This is fixed.