1377560 - Don't keep nsContentList's alive

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[(no longer active)]

We have code pattern where we do things like <https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/dom/html/nsHTMLDocument.cpp#2351>

Here, the mForms variable and the like would keep the content list object alive for as long as the holder object is alive.

It would be nice if these were switched to be a weak pointer instead, so that the content list objects could be GCed earlier on if the page would drop its own references to them.

Comment 1

[John Dai[:jdai]]

Attachment: wip, v1

This is just a draft. In this patch, I switched some variables which use RefPtr hold the content list object to be a weak pointer instead, so that the content list objects could be GCed earlier on if the page would drop its own references to them. 
Ehsan, could you take a look if I'm in the right direction? Thanks!

Comment 2

[(no longer active)]

Comment on attachment 8888631 [details] [diff] [review]
wip, v1

Review of attachment 8888631 [details] [diff] [review]:
-----------------------------------------------------------------

So with this, when each one of these objects dies, the respective pointer becomes dangling.

You need to also make nsContentList::RemoveFromCaches() able to tell its holder object (nsHTMLDocument) that it is dying, so that it can go ahead and null out its reference to the object when that happens...

::: dom/html/nsHTMLDocument.h
@@ +311,5 @@
> +  nsContentList* mLinks;
> +  nsContentList* mAnchors;
> +  nsContentList* mScripts;
> +  nsContentList* mForms;
> +  nsContentList* mFormControls;

Can you please annotate these as MOZ_NON_OWNING_REF while you're here?

Comment 3

[(no longer active)]

John, are you still working on this?

Comment 4

[(no longer active)]

Because of bug 1392891, I'm going to assume that this will help with Speedometer V2.  But we should really fix that bug independently of this IMO.

Comment 5

[John Dai[:jdai]]

Sorry for the delay, I'll continue working on this bug. Keep NI for tracking.

Comment 6

[John Dai[:jdai]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away 8/23) from comment #2)
> Comment on attachment 8888631 [details] [diff] [review]
> wip, v1
> 
Thanks for your feedback.

> Review of attachment 8888631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So with this, when each one of these objects dies, the respective pointer
> becomes dangling.
> 
> You need to also make nsContentList::RemoveFromCaches() able to tell its
> holder object (nsHTMLDocument) that it is dying, so that it can go ahead and
> null out its reference to the object when that happens...
> 
I saw nsContentList::RemoveFromCaches() is called by cycle collection unlink, I am not sure it's a suitable place to null out its reference to the object. Do you think we should null out its reference in destructor of nsContentList is a better place? 

> ::: dom/html/nsHTMLDocument.h
> @@ +311,5 @@
> > +  nsContentList* mLinks;
> > +  nsContentList* mAnchors;
> > +  nsContentList* mScripts;
> > +  nsContentList* mForms;
> > +  nsContentList* mFormControls;
> 
> Can you please annotate these as MOZ_NON_OWNING_REF while you're here?
Will do.

Comment 7

[(no longer active)]

(In reply to John Dai[:jdai] from comment #6)
> > Review of attachment 8888631 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So with this, when each one of these objects dies, the respective pointer
> > becomes dangling.
> > 
> > You need to also make nsContentList::RemoveFromCaches() able to tell its
> > holder object (nsHTMLDocument) that it is dying, so that it can go ahead and
> > null out its reference to the object when that happens...
> > 
> I saw nsContentList::RemoveFromCaches() is called by cycle collection
> unlink, I am not sure it's a suitable place to null out its reference to the
> object. Do you think we should null out its reference in destructor of
> nsContentList is a better place? 

No, why?  When the object gets unlinked by the CC it gets into an semi-dead state waiting to be deleted by the snowwhite killer.  You don't want to return an object in such a state because it exists in the hashtable, right?

Comment 8

[John Dai[:jdai]]

Attachment: wip, v2

-  Revise mImages, mEmbeds, mLinks, mAnchors, mScripts from RefPtr to a raw pointer. The reason I didn't change mForms and mFormControls from RefPtr to a raw pointer is because it'll use in nsContentUtils::GenerateStateKey[1].
                 
- Add a holder function to null out its reference to the object.

- Add MOZ_NON_OWNING_REF for raw pointers.

[1] https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/base/nsContentUtils.cpp#3039-3040

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37bf119eb58a5da5c9581fda14c30888f38e8fb0&filter-tier=1&group_state=expanded

Comment 9

[(no longer active)]

(In reply to John Dai[:jdai] from comment #8)
> -  Revise mImages, mEmbeds, mLinks, mAnchors, mScripts from RefPtr to a raw
> pointer. The reason I didn't change mForms and mFormControls from RefPtr to
> a raw pointer is because it'll use in nsContentUtils::GenerateStateKey[1].

Note that I recently changed that code in bug 1392891...  Also see bug 1395442 where I'm removing mFormControls completely.

But I think you should do this for mForms.

Comment 10

[(no longer active)]

Comment on attachment 8902559 [details] [diff] [review]
wip, v2

Review of attachment 8902559 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsHTMLDocument.cpp
@@ +1161,2 @@
>      mImages = new nsContentList(this, kNameSpaceID_XHTML, nsGkAtoms::img, nsGkAtoms::img);
> +    mImages->SetHolderFn([self] () {

The lambda that you store on the nsContentList object here has a RefPtr<nsHTMLDocument> member, so you're effectively adding a strong reference from the nsContentList object to the document which isn't declared to the cycle collector.  Why is this OK?  Isn't it better to have this pointer be a normal member of the class, and declare it to the CC normally?

With that, I think you can pass a pointer to a member function of nsHTMLDocument here instead and call that inside RemoveFromHashtable.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #10)

> ::: dom/html/nsHTMLDocument.cpp
> @@ +1161,2 @@
> >      mImages = new nsContentList(this, kNameSpaceID_XHTML, nsGkAtoms::img, nsGkAtoms::img);
> > +    mImages->SetHolderFn([self] () {
> 
> The lambda that you store on the nsContentList object here has a
> RefPtr<nsHTMLDocument> member, so you're effectively adding a strong
> reference from the nsContentList object to the document which isn't declared
> to the cycle collector.

Haa, you just gave me yet another reason to dislike lambda!
(these modern C++ features which hide what actually happens are really annoying, IMHO.)

Comment 12

[John Dai[:jdai]]

Attachment: wip, v3

Address comment 10.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6235ada487d13477a242c16d05c7b95fdd3d419&filter-tier=1&group_state=expanded

Comment 13

[John Dai[:jdai]]

Attachment: wip, v3

Upload correct patch.

Comment 14

[(no longer active)]

Comment on attachment 8911747 [details] [diff] [review]
wip, v3

Review of attachment 8911747 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great overall, a few nits below.

::: dom/html/nsHTMLDocument.cpp
@@ -202,5 @@
> -                                   mEmbeds,
> -                                   mLinks,
> -                                   mAnchors,
> -                                   mScripts,
> -                                   mForms,

Hmm, I still think you need to tell the CC about these members.  You can declare overloads of ImplCycleCollectionTraverse() and ImplCycleCollectionUnlink() functions for ContentListHolder to make sure the hunk you have removed here can be added back and it would still compile fine.

::: dom/html/nsHTMLDocument.h
@@ +345,5 @@
>  
>    friend class ContentListHolder;
>    ContentListHolder* mContentListHolder;
>  
> +  class ContentListHelper : public ContentListHolderNotifier

How about calling this ContentListHolder?

@@ +378,5 @@
> +        mContentList = nullptr;
> +      }
> +    }
> +
> +    void OnDestroy()

Nit: please add an override keyword here.

Comment 15

[John Dai[:jdai]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #14)
> Comment on attachment 8911747 [details] [diff] [review]
> wip, v3
> 
> Review of attachment 8911747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, looks great overall, a few nits below.
>
Thanks for your feedback. 
 
> ::: dom/html/nsHTMLDocument.cpp
> @@ -202,5 @@
> > -                                   mEmbeds,
> > -                                   mLinks,
> > -                                   mAnchors,
> > -                                   mScripts,
> > -                                   mForms,
> 
> Hmm, I still think you need to tell the CC about these members.  You can
> declare overloads of ImplCycleCollectionTraverse() and
> ImplCycleCollectionUnlink() functions for ContentListHolder to make sure the
> hunk you have removed here can be added back and it would still compile fine.
> 
Will do. Thank you for catching this.

> ::: dom/html/nsHTMLDocument.h
> @@ +345,5 @@
> >  
> >    friend class ContentListHolder;
> >    ContentListHolder* mContentListHolder;
> >  
> > +  class ContentListHelper : public ContentListHolderNotifier
> 
> How about calling this ContentListHolder?
> 
Unfortunately, We have ContentListHolder class in nsHTMLDocument[1].

[1] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/html/nsHTMLDocument.h#320-343 

> @@ +378,5 @@
> > +        mContentList = nullptr;
> > +      }
> > +    }
> > +
> > +    void OnDestroy()
> 
> Nit: please add an override keyword here.
Will do.

Comment 16

[John Dai[:jdai]]

Attachment: patch, v4

Address comment 14. 

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59dce8a5e75f73fd109136da747a91ce344f120b&filter-tier=1&group_state=expanded&selectedJob=134626249

Hi Ehsan,
Could you help to feedback it again? Thank you.

Comment 17

[(no longer active)]

Comment on attachment 8915447 [details] [diff] [review]
patch, v4

Review of attachment 8915447 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsHTMLDocument.cpp
@@ +207,5 @@
>                                               nsIDOMHTMLDocument)
>  
> +inline void
> +ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,
> +                            nsHTMLDocument& aField,

I meant defining these for nsHTMLDocument::ContentListHelper, not nsHTMLDocument itself.  That way you can leave the above macro unchanged.

@@ +220,5 @@
> +  CycleCollectionNoteChild(aCallback, aField.mForms.GetList(), aName, aFlags);
> +}
> +
> +inline void
> +ImplCycleCollectionUnlink(nsHTMLDocument& aField)

Ditto.

Comment 18

[John Dai[:jdai]]

Attachment: patch, v5

Address comment 17.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b81ba319d85d18fc1ea21057e1ed73f8cfb34ebf&filter-tier=1&group_state=expanded

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8924404 [details] [diff] [review]
patch, v5

>+inline void
>+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,
>+                            nsHTMLDocument::ContentListHelper& aField,
>+                            const char* aName,
>+                            uint32_t aFlags = 0)
>+{
>+  CycleCollectionNoteChild(aCallback, aField.GetList(), aName, aFlags);
>+}
>+
>+inline void
>+ImplCycleCollectionUnlink(nsHTMLDocument::ContentListHelper& aField)
>+{
>+  aField.Reset();
>+}
>+
> NS_IMPL_CYCLE_COLLECTION_INHERITED(nsHTMLDocument, nsDocument,
>                                    mAll,
>                                    mImages,
>                                    mApplets,
>                                    mEmbeds,
>                                    mLinks,
>                                    mAnchors,
>                                    mScripts,
This looks wrong.
We shouldn't traverse those ContentList objects we don't have a strong reference to.
I don't know why ehsan said something else in comment 14.

>+
>+    void CreateList(nsContentList* aList)
>+    {
>+      MOZ_ASSERT(aList && !mContentList);
>+      mContentList = aList;
>+      mContentList->SetupHolderNotifier(this);
Could this method be called something else, since it doesn't create any list.
Perhaps AssignList ?