Closed Bug 1438541 Opened 2 years ago Closed 2 years ago

make all nsIGlobalObjects track and disconnect DETH objects

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
Priority: -- → P2
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.
Attachment #8951775 - Flags: review?(bugs)
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.
Attachment #8951776 - Flags: review?(bugs)
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.
Attachment #8951777 - Flags: review?(bugs)
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 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)
Attachment #8951775 - Flags: review?(bugs) → review+
Attachment #8951776 - Flags: review?(bugs) → review+
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.
Attachment #8951777 - Flags: review?(bugs) → review+
(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.
(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.
(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.
Updated comment to describe raw pointers instead of weak pointers.

I did not fix the other issue here.  That will be addressed in P4.
Attachment #8951775 - Attachment is obsolete: true
Attachment #8952759 - Flags: review+
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.
Attachment #8952761 - Flags: review?(bugs)
Attachment #8952761 - Flags: review?(bugs) → review+
Pushed by bkelly@mozilla.com:
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
Pushed by bkelly@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.