1438541 - make all nsIGlobalObjects track and disconnect DETH objects

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Over in bug 1438211 I am re-working an webidl implementation object that can execute in both window and worker contexts.  I need DOMEventTargetHelper::DisconnectFromOwner() to be called when the worker scope goes aways, but currently that only happens for windows.

After talking with Olli on IRC we decided that it probably makes sense to move the DETH tracking code from nsGlobalWindowInner down into the nsIGlobalObject base class.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: deth-wip.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de275c65fa5ae0a20ebadd9262d504195d6f3bc8

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: deth-wip.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1812a952fdc448d82f6b80b2cc746652dfe1b857

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Track DOMEventTargetHelper objects in the nsIGlobalObject base class and always call DisconnectFromOwner() on them. r=smaug

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make nsIGlobalObject::GetOrCreateServiceWorker() use the DOMEventTargetHelper list stored on the global. r=smaug

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Allow nsIGlobalObject::ForEachEventTargetObject() callback function to abort iteration. r=smaug

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fb8c86e75f59626fdad52bc1022d5a67781ce7e

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8951775 [details] [diff] [review]
P1 Track DOMEventTargetHelper objects in the nsIGlobalObject base class and always call DisconnectFromOwner() on them. r=smaug

Olli, this patch makes the nsIGlobalObject base class track DETH objects and consistently call DisconnectFromOwner().  I had to do a few additional items here, so I will try to describe each one:

1. Move AddEventTargetObject(), RemoveEventTargetObject(), and mEventTargetObjects hash table from nsGlobalWindowInner to nsIGlobalObject.
2. Moved DisconnectEventTargetObjects() to nsIGlobalObject and makes it automatically call it in the destructor.  I left the explicit calls in nsGlobalWindowInner because the invariants there were not clear to me.
3. Implemented DisconnectEventTargetObjects() in terms of a ForEachEventTargetObject() iteration method.  I chose to do it this way because I know I will need to iterate the objects for another use case in the next patch.
4. I changed DETH::DisconnectFromOwner() to explicitly call RemoveEventTargetObject().  I then added assertions throughout to ensure that we properly manage the weak references involved here.
5. Replace the nsWeakReference in DETH with a raw pointer that is cleared by DisconnectFromOwner().  This is necessary because nsWeakReference could be cleared by CC before the global is destroyed preventing the DETH from calling RemoveEventTargetObject().  Waiting for DisconnectFromOwner() to clear the pointer allows us to make the RemoveEventTargetObject() explicitly.  The assertions I added in the nsIGlobalObject destructor helped catch this problem.
6. I also had to deal with the memory reporting in nsGlobalWindowInner memory reporting.  To do this I use ForEachEventTargetObject() to measure the event targets and listeners.  I moved the mEventTargetObjects hashtable itself into a nsIGlobalObject::ShallowSizeOfExcludingThis() method and added this to the "other" memory.  So some memory moved to a different bucket, but its pretty small.  As a nice side effect we now measure memory on the other containers owned by nsIGlobalObject.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8951776 [details] [diff] [review]
P2 Make nsIGlobalObject::GetOrCreateServiceWorker() use the DOMEventTargetHelper list stored on the global. r=smaug

This patch removes the mServiceWorkerList of cached ServiceWorker c++ objects in nsGlobalWindowInner.  Instead we make GetOrCreateServiceWorker() iterate the DETH objects using ForEachEventTargetObject().  In order to perform the matching we add support do_QueryObject() to the ServiceWorker object.

Right now I keep this code in nsGlobalWindowInner because there are other reasons we cannot yet expose ServiceWorker in WorkerGlobalScope.  In the future I may move GetOrCreateServiceWorker implementation down into nsIGlobalObject.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8951777 [details] [diff] [review]
P3 Allow nsIGlobalObject::ForEachEventTargetObject() callback function to abort iteration. r=smaug

This patch fixes the TODO item in the P2 patch.  Sorry I forgot to mention that in the last patch description.

Basically it adds a `bool* aDoneOut` flag to the ForEachEventTargetObject() callback signature.  The callback can set this true if they want to stop iteration instead of always iterating the entire DETH list each time.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Oh also, I think I will probably try adding NS_ASSERT_OWNINGTHREAD() in the various nsIGlobalObject and DETH objects.  The objects are not thread-safe ref-counting currently, so I think the risk is small, but it might be nice to assert.

Comment 10

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

Comment on attachment 8951775 [details] [diff] [review]
P1 Track DOMEventTargetHelper objects in the nsIGlobalObject base class and always call DisconnectFromOwner() on them. r=smaug

>+
>+  // Weak references to bound DETH objects.  These are added by
>+  // AddEventTargetObject().  The DETH object must call
>+  // RemoveEventTargetObject() before its destroyed to clear
>+  // its weak ref here.
>+  nsTHashtable<nsPtrHashKey<mozilla::DOMEventTargetHelper>> mEventTargetObjects;
nsPtrHashKey doesn't mean weak reference but raw pointer. Could you clarify the comment
(weak references are safe to use, raw pointers aren't, and those need to be handled carefully).

> DOMEventTargetHelper::BindToOwner(DOMEventTargetHelper* aOther)
> {
>+  if (mParentObject) {
>+    mParentObject->RemoveEventTargetObject(this);
>+  }
>   if (mOwnerWindow) {
>-    nsGlobalWindowInner::Cast(mOwnerWindow)->RemoveEventTargetObject(this);
>     mOwnerWindow = nullptr;
>     mParentObject = nullptr;
>     mHasOrHasHadOwnerWindow = false;
>   }
Don't you want to set mParentObject null in the if (mParentObject), not in if (mOwnerWindow)

Comment 11

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

Comment on attachment 8951777 [details] [diff] [review]
P3 Allow nsIGlobalObject::ForEachEventTargetObject() callback function to abort iteration. r=smaug

that out param is a tad ugly. I would have used return value.
But this is fine too.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #10)
> >+  nsTHashtable<nsPtrHashKey<mozilla::DOMEventTargetHelper>> mEventTargetObjects;
> nsPtrHashKey doesn't mean weak reference but raw pointer. Could you clarify
> the comment
> (weak references are safe to use, raw pointers aren't, and those need to be
> handled carefully).

I'll update the comment to specify raw pointers.  However, I think there are quite a few places that describe raw pointers as "weak" vs strong RefPtr/nsCOMPtr.  I agree its not very precise.

> > DOMEventTargetHelper::BindToOwner(DOMEventTargetHelper* aOther)
> > {
> >+  if (mParentObject) {
> >+    mParentObject->RemoveEventTargetObject(this);
> >+  }
> >   if (mOwnerWindow) {
> >-    nsGlobalWindowInner::Cast(mOwnerWindow)->RemoveEventTargetObject(this);
> >     mOwnerWindow = nullptr;
> >     mParentObject = nullptr;
> >     mHasOrHasHadOwnerWindow = false;
> >   }
> Don't you want to set mParentObject null in the if (mParentObject), not in
> if (mOwnerWindow)

Good catch!  I think this bug exists in the tree today as well:

https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/dom/events/DOMEventTargetHelper.cpp#138

I wish I could just make this method forward to BindToOwner(nsIGlobalObject*), but I don't think I can.  They don't handle the mHasOrHasHadOwnerWindow flag quite the same way.  This method wants to copy the value from the other target.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #11)
> that out param is a tad ugly. I would have used return value.
> But this is fine too.

Yea, a return value was also my initial instinct, but I find returning a bare boolean can be confusing to read.  I tried making an easy to read return value system for nsGlobalWindowInner::CallOnChildren() here and it got more complex then I wanted:

https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/dom/base/nsGlobalWindowInner.h#1123

I thought I would try the out parameter here for now.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #12)
> Good catch!  I think this bug exists in the tree today as well:
> 
> https://searchfox.org/mozilla-central/rev/
> 47cb352984bac15c476dcd75f8360f902673cb98/dom/events/DOMEventTargetHelper.
> cpp#138
> 
> I wish I could just make this method forward to
> BindToOwner(nsIGlobalObject*), but I don't think I can.  They don't handle
> the mHasOrHasHadOwnerWindow flag quite the same way.  This method wants to
> copy the value from the other target.

Actually, I think I see how to do this.  I will do it in a follow-up patch for review, though.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Track DOMEventTargetHelper objects in the nsIGlobalObject base class and always call DisconnectFromOwner() on them. r=smaug

Updated comment to describe raw pointers instead of weak pointers.

I did not fix the other issue here.  That will be addressed in P4.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 De-duplicate code in DOMEventTargetHelper::BindToOwner() methods. r=smaug

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fbc8f3ff609c9122b710ec27ef5a768a35fa103

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8952761 [details] [diff] [review]
P4 De-duplicate code in DOMEventTargetHelper::BindToOwner() methods. r=smaug

Olli, this addresses your review feedback from comment 10.  It de-duplicates that code by just forwarding to the BindToOwner(nsIGlobalObject*) method.  Conveniently the problematic code from your comment goes away.

Note, the old BindToOwner(DETH*) method was setting the mHasOrHasHadOwnerWindow method based on the aOther value.  I continue this practice, but moved the assignment to after calling the underlying BindToOwner().  I believe this is equivalent.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d30eced092
P1 Track DOMEventTargetHelper objects in the nsIGlobalObject base class and always call DisconnectFromOwner() on them. r=smaug

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2fc66acc13
P2 Make nsIGlobalObject::GetOrCreateServiceWorker() use the DOMEventTargetHelper list stored on the global. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/26cad18f73fa
P3 Allow nsIGlobalObject::ForEachEventTargetObject() callback function to abort iteration. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5675db58f9e9
P4 De-duplicate code in DOMEventTargetHelper::BindToOwner() methods. r=smaug

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/a1d30eced092
https://hg.mozilla.org/mozilla-central/rev/9a2fc66acc13
https://hg.mozilla.org/mozilla-central/rev/26cad18f73fa
https://hg.mozilla.org/mozilla-central/rev/5675db58f9e9