Reloading or closing the testcase triggers the assertion in nsGenericDOMDataNode::~nsGenericDOMDataNode: ###!!! ASSERTION: Please remove this from the document properly: '!IsInDoc()', file mozilla/content/base/src/nsGenericDOMDataNode.cpp, line 76

Hmm... So the issue here is that blocks destroy their floats in Destroy() before destroying their in-flows. Which means that the ::first-letter frame dies before the ::after pseudo-frame dies, and then the ::after pseudo can't get to the textframe that's a child of the ::first-letter to unbind that content. If there were more than one letter of text, there would be in-flows for that text too, and things would be fine. But there isn't, in this case. We should probably make floating first-letter frames whose placeholders are in generated content unbind their kid on Destroy. Question is how to best detect this.

Perhaps generated content frames should have references to the content they created so they don't have to crawl their children? (as a frame property, of course)

With an owning reference in the property (e.g. an nsCOMArray<nsIContent>)? Yeah, that would be a good idea. Much more flexible than what we do now. Nickolay, would you be interested in doing that?

> With an owning reference in the property (e.g. an nsCOMArray<nsIContent>)? Yeah.

Do you mean something like this? If so, I'd appreciate quick comments on the XXXs in the patch, so that I don't have to read the code :p I'm also not sure if the XXXbz comment in ns{Inline/Block}Frame::Destroy is addressed (or what does it mean).

This looks like the right idea! + nsCOMArray<nsIContent>* generatedContent = + reinterpret_cast<nsCOMArray<nsIContent>*>( + GetProperty(nsGkAtoms::generatedContent)); You could just call RemoveProperty I think, so you don't need to do UnsetProperty later. I think this is implicitly addressing bz's XXX comment or at least making it moot.

That looks about right, modulo error handling and nitpicks. And this definitely addresses my XXX comment. Oh, and now nsInlineFrame::Destroy can go away.

> You could just call RemoveProperty I think, so you don't need to do > UnsetProperty later. I don't see a RemoveProperty; I think you meant DeleteProperty, but it only functions correctly when provided a destructor function. I can UnsetProperty as the first step though and avoid an extra GetProperty call. > + // XXXnickolay I /think/ I'm handling NS_ERROR_FRAME_REPLACED properly here, > + // but haven't checked Interesting, NS_ERROR_FRAME_REPLACED isn't used anymore since the patch for bug 11011. Can I just remove it in this bug, file a separate one, or is it kept for a reason? http://mxr.mozilla.org/seamonkey/search?string=NS_ERROR_FRAME_REPLACED

Using a destructor function here would make sense. And yes, nixing mention of NS_ERROR_FRAME_REPLACED in this patch is just fine by me. I just forgot to remove those few spots, I guess. :(

Comment on attachment 273348 [details] [diff] [review] patch, v2 bah, didn't see the previous comment.

With the NS_ERROR_FRAME_REPLACED cleanup and an inaccurate comment removed. I'm not sure a destructor function would be useful here, since we only clean up in one place - Destroy(), but can use it if you think it's better.

Comment on attachment 273351 [details] [diff] [review] patch + reinterpret_cast<nsCOMArray<nsIContent>*>( I think this should be a static_cast<>. A destructor function is not needed because we only set the property once, near where we create the frame, and we always remove the property in nsContainerFrame::Destroy. But you should add a comment to that effect where you set the property without a destructor function.

mozilla/content/base/src/nsGkAtomList.h 3.75 mozilla/layout/base/nsCSSFrameConstructor.cpp 1.1371 mozilla/layout/base/nsCSSFrameConstructor.h 1.233 mozilla/layout/base/nsLayoutErrors.h 1.11 mozilla/layout/generic/nsBlockFrame.cpp 3.844 mozilla/layout/generic/nsContainerFrame.cpp 1.287 mozilla/layout/generic/nsContainerFrame.h 3.78 mozilla/layout/generic/nsIFrame.h 3.371 mozilla/layout/generic/nsInlineFrame.cpp 3.287 mozilla/layout/generic/nsInlineFrame.h 1.67

Crashtest checked in.