Jesse Ruderman Reporter
	Description • 18 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 18 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 18 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 18 years ago

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)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 18 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 18 years ago

(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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 18 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 18 years ago

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]

	Jesse Ruderman Reporter
	Comment 10 • 18 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 11 • 18 years ago

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

	Jesse Ruderman Reporter
	Comment 12 • 17 years ago

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

	Jesse Ruderman Reporter
	Comment 13 • 17 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 17 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 17 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 17 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 17 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 17 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 17 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 20 • 17 years ago

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.

	Damon Sicore (:damons)
	Comment 21 • 17 years ago

+'ing w/ P3.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 22 • 17 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 17 years ago

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

Yay, thanks for documenting those hashes!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 17 years ago

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

	Stuart Parmenter
	Comment 25 • 17 years ago

is this fixed?  if not, renominate

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 17 years ago

Er, yeah.  This is fixed.