Don't keep nsContentList's alive

NEW
Unassigned

Status

()

enhancement
P2
normal
2 years ago
3 months ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

2 years ago
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.
Assignee: nobody → jdai
Status: NEW → ASSIGNED
Posted patch wip, v1 (obsolete) — Splinter Review
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!
Attachment #8888631 - Flags: feedback?(ehsan)
Reporter

Comment 2

2 years ago
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?
Attachment #8888631 - Flags: feedback?(ehsan) → feedback-
Reporter

Comment 3

2 years ago
John, are you still working on this?
Flags: needinfo?(jdai)
Reporter

Updated

2 years ago
See Also: → 1392891
Reporter

Comment 4

2 years ago
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.
Sorry for the delay, I'll continue working on this bug. Keep NI for tracking.
(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.
Flags: needinfo?(ehsan)
Reporter

Comment 7

2 years ago
(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?
Flags: needinfo?(ehsan)
Posted patch wip, v2 (obsolete) — Splinter Review
-  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
Attachment #8888631 - Attachment is obsolete: true
Flags: needinfo?(jdai)
Attachment #8902559 - Flags: feedback?(ehsan)
Reporter

Comment 9

2 years ago
(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.
Reporter

Comment 10

2 years ago
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.
Attachment #8902559 - Flags: feedback?(ehsan)
(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.)
Priority: -- → P2
Posted patch wip, v3 (obsolete) — Splinter Review
Upload correct patch.
Attachment #8911745 - Attachment is obsolete: true
Attachment #8911745 - Flags: feedback?(ehsan)
Attachment #8911747 - Flags: feedback?(ehsan)
Reporter

Comment 14

2 years ago
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.
Attachment #8911747 - Flags: feedback?(ehsan) → feedback+
(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.
Posted patch patch, v4 (obsolete) — Splinter Review
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.
Attachment #8911747 - Attachment is obsolete: true
Attachment #8915447 - Flags: feedback?(ehsan)
Reporter

Comment 17

2 years ago
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.
Attachment #8915447 - Flags: feedback?(ehsan) → feedback+
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 ?
Attachment #8924404 - Flags: review?(bugs) → review-
Assignee: jdai → nobody
Status: ASSIGNED → NEW
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.