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

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

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

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)

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?

(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

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

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]

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

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

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

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

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

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.

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.

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

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.

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

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.

+'ing w/ P3.

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 on attachment 292365 [details] [diff] [review] Proposed fix Yay, thanks for documenting those hashes!

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

is this fixed? if not, renominate

Er, yeah. This is fixed.