Closed Bug 388709 Opened 14 years ago Closed 14 years ago

"ASSERTION: Please remove this from the document properly: '!IsInDoc()'" with :after, floating :first-letter

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: jruderman, Assigned: asqueella)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 3 obsolete files)

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.
Flags: in-testsuite-
Flags: blocking1.9?
Flags: in-testsuite-
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.
Attached patch untested patch (obsolete) — Splinter Review
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).
Assignee: nobody → asqueella
Status: NEW → ASSIGNED
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
OS: Mac OS X → All
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.  :(
Attached patch patch, v2 (obsolete) — Splinter Review
Attachment #273072 - Attachment is obsolete: true
Attachment #273348 - Flags: superreview?(roc)
Attachment #273348 - Flags: review?(roc)
Comment on attachment 273348 [details] [diff] [review]
patch, v2

bah, didn't see the previous comment.
Attachment #273348 - Attachment is obsolete: true
Attachment #273348 - Flags: superreview?(roc)
Attachment #273348 - Flags: review?(roc)
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #273351 - Flags: superreview?(roc)
Attachment #273351 - Flags: review?(roc)
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.
Attachment #273351 - Flags: superreview?(roc)
Attachment #273351 - Flags: superreview+
Attachment #273351 - Flags: review?(roc)
Attachment #273351 - Flags: review+
Attached patch for checkinSplinter Review
Attachment #273351 - Attachment is obsolete: true
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9beta1
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.