257690 - CreateGeneratedFrameFor() doesn't add the generated content as anonymous content

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Johnny Stenback (:jst)]

... and that causes the content to not be properly torn down, which causes leaks
etc. Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Mark tell the presshell about anonymous content for generated frames.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So is the problem here that the generated content holds a strong ref to the
document and the document somehow ends up holding a ref to the content?

If so, why are we limiting this to image content and not the other types of
generated content here?

Comment 3

[Johnny Stenback (:jst)]

Oh, all types of generated content should get the same behaviour. The problem is
that if we don't tell the presshell about the anon content, the anon content
will never be properly destroyed, so it'll hold dangling pointers to the
document etc even, and most importantly, since the anon content is never
notified about the document going away, it never clears its references to its
event listener, and that's the primary cause of the leaks.

Where else do I need to patch this?

Comment 4

[Johnny Stenback (:jst)]

Oh, I see... just other places in the same method...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Wait.  The only thing holding a ref to the anon content is the frame, right?  So
when the frame goes away it should get destroyed...

How do event listeners come into that?

I agree the patch is probably the right thing to do; just trying to figure out
how we actually leak.

Comment 6

[Johnny Stenback (:jst)]

Sorry, this doesn't actually cause any leaks as I thought it did, it only makes
us hit the assertion at

http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericElement.cpp#817

which isn't that bad, but it's still the right thing to do. I've got a diff to
do this for all types of generated content, not just for images.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Tell the presshell about anonymous content for generated frames.

Updated diff that should cover all types of generated content.

Comment 8

[Johnny Stenback (:jst)]

Attachment: diff -w for review

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 157641 [details] [diff] [review]
diff -w for review

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>@@ -1524,38 +1525,36 @@ nsCSSFrameConstructor::CreateGeneratedFr
>           rv = NS_NewAttributeContent(aContent, attrNameSpace, attrName,
>                                       getter_AddRefs(content));

Now that you're falling through later, you need to bail here if rv fails.  In
fact, this code should have been returning early on allocation failure here all
along.

>           NS_NewTextFrame(shell, &textFrame);
>           textFrame->Init(aPresContext, content, aParentFrame, aStyleContext,

As long as you're here, mind doing a null-check on this too?  And making sure
NS_NewTextFrame will set the out param to null on alloc failure?

>+    nsresult rv = NS_NewISupportsArray(getter_AddRefs(anonymousItems));

The api really sucks here.. want to file a bug to clean it up so we can just
pass in a content node here and the presshell will deal?

r+sr=bzbarsky with those first two issues fixed.

Comment 10

[Johnny Stenback (:jst)]

Filed bug 257868 on the lame nsIPresShell::SetAnonymousContentFor() API.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fixed patch

Comment 12

[Johnny Stenback (:jst)]

Fix checked in.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 157641 [details] [diff] [review]
diff -w for review

We should probably take this on the aviary branch, to keep our developers'
sanity (by eliminating lots and lots of asserts when browsing mozilla.org
pages), if nothing else.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 157641 [details] [diff] [review]
diff -w for review

a=asa for branch checkins.

Comment 15

[Johnny Stenback (:jst)]

Fixed on aviary.

Comment 16

[Mike Kaply [:mkaply]]

jst, can you put this on 1.7 as well please?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Could we PLEASE land stuff on both branches in parallel?  This checkin made a
patch I needed to land on both branches only apply to one of them (and made me
waste a good bit of time trying to figure out what went wrong).  That should NOT
be happening with Gecko changes.  As long as people land this sort of thing on
both branches at once consistently, it makes it easier for everyone else to do
the same...

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What bz said. It is not acceptable to put off landing patches on 1.7. The
branches need to stay in sync as we go or it's tons more work for everyone.

Comment 19

[Johnny Stenback (:jst)]

Fixed on the 1.7 branch.