Closed Bug 1450274 Opened 2 years ago Closed 2 years ago

remove service worker binding object ref-cycle code in favor of DOMEventTarget::KeepAliveIfHasListenersFor()

Categories

(Core :: DOM: Service Workers, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(3 files, 6 obsolete files)

7.87 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.34 KB, patch
baku
: review+
Details | Diff | Splinter Review
9.57 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
It turns out DETH has a mechanism to keep binding objects alive for event listeners.  We should use that in service worker binding code instead of our bespoke ref-cycle stuff.

This needs to wait until bug 1447871 is fixed first, though.
Priority: -- → P2
Attachment #8965111 - Attachment is obsolete: true
Comment on attachment 8965077 [details] [diff] [review]
P1 Make ServiceWorker use DOMEventTargetHelper::KeepAliveIfHasListenersFor(). r=asuth

Andrew, this patch essentially reverts bug 1438211 P7 that created a ref-cycle for each ServiceWorker binding object.  Instead this makes ServiceWorker use the DETH::KeepAliveIfHasListenersFor() mechanism.  We stop keeping things alive if we detect the state has transitioned to redundant.  DETH automatically stops keeping things alive via DisconnectFromOwner().

This is better than the ref-cycle approach because it uses a standard mechanism.  Also, it only keeps the object alive if an event listener is actually added.
Attachment #8965077 - Flags: review?(bugmail)
Comment on attachment 8965445 [details] [diff] [review]
P2 Make ServiceWorkerRegistration use DETH::KeepAliveIfHasListenersFor(). r=asuth

This patch moves ServiceWorkerRegistration to the DETH KeepAliveIfHasListenersFor() approach instead of a ref-cycle.  We calling IgnoreKeepAliveIfHasListenersFor() when the registration is removed.
Attachment #8965445 - Flags: review?(bugmail)
Comment on attachment 8965457 [details] [diff] [review]
P3 Make WorkerGlobalScope call DisconnectEventTargetObjects() when the worker reaches the Terminating state. r=baku

Andrea, this makes WorkerGlobalScope call DisconnectFromOwner() on all its child DETH objects when the worker hits Terminating.  This is similar to how windows call DisconnectFromOwner() on FreeInnerObjects().

We need this in order to use KeepAliveIfHasListenerFor() on DETH objects bound to a worker global.  We can't wait for ~WorkerGlobalScope() to run.
Attachment #8965457 - Flags: review?(amarchesini)
Attachment #8965457 - Flags: review?(amarchesini) → review+
Comment on attachment 8965077 [details] [diff] [review]
P1 Make ServiceWorker use DOMEventTargetHelper::KeepAliveIfHasListenersFor(). r=asuth

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

Restating:
- mInner is now always a strong reference and not cleared by DisconnectFromOwner() because DETH's keepalive mechanism acts on `this`, so mInner's refcount will be dropped at destruction time.
- I added notes for DETH at https://github.com/asutherland/asuth-gecko-notes/blob/master/dom/events/DOMEventTargetHelper.md that I can probably mine for comments for the tree.
Attachment #8965077 - Flags: review?(bugmail) → review+
Comment on attachment 8965445 [details] [diff] [review]
P2 Make ServiceWorkerRegistration use DETH::KeepAliveIfHasListenersFor(). r=asuth

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

Restating:
- Same deal as part 1 but for SWR.

::: dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp
@@ +1090,5 @@
>  
>    mRegistration->UpdateFound();
>  }
>  
> +class RegistrationRemovedWorkerRunnable final : public WorkerControlRunnable

Do you really mean to be doing this?  The whole point of the main-thread ServiceWorkerRegistrationMainThread::RegistrationRemoved using NewRunnableMethod to defer a turn of the event loop was to avoid removing the registration out from under other events still waiting in the event queue.  Making this a control runnable also causes it to jump the line and eliminates the ordering expectations.  (Also, syncloop edge cases).  It'd be great to get a comment here if this is intentional.

Right now, pre bug 1450644 (and post :baku's changes on bug 1445554), this doesn't really seem necessary since the the WorkerWeakRef callback will be invoked on the worker thread during WorkerHolder::Notify(Closing), invoking the callback which does ServiceWorkerRegistrationWorkerThread::ReleaseListener() which invokes WorkerListener::ClearRegistration() which nulls out WorkerListener::mRegistration (guarded by a mutex), which will cause WorkerListener::RegistrationRemoved to fast-path out at the top because `if (!mRegistration)`.  In the event there's a race around this where the runnable has not yet run before Notify(Closing) happens, the runnable gets nuked by ClearMainEventQueue(), so it doesn't matter.

Post bug 1450644 (which will land soon, about to finish those reviews next), the WorkerWeakRef callback will happen at Notify(Canceling) but the runnable will be doomed at Closing, so that helps avoid a Closing-Canceling gap, but it's not obvious why that's important since we know the worker will reliably get closed (bug 1450644 adds a timeout too) and a control runnable is not strong enough to provide any guarantees about the worker not sending the main thread requests about the already-removed registration.
Attachment #8965445 - Flags: review?(bugmail) → review+
(In reply to Andrew Sutherland [:asuth] from comment #14)
> > +class RegistrationRemovedWorkerRunnable final : public WorkerControlRunnable
> 
> Do you really mean to be doing this?

I added this before I realized I needed to do something like P3.  Now that we have the P3 patch I'll see if I can remove this.  The goal here was just to allow registration cleanup on the worker thread soon enough to avoid js runtime assertions around leaked objects.
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1838e2fbb4f
P1 Make ServiceWorker use DOMEventTargetHelper::KeepAliveIfHasListenersFor(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6337703ef9d8
P2 Make ServiceWorkerRegistration use DETH::KeepAliveIfHasListenersFor(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd2cafb5b21
P3 Make WorkerGlobalScope call DisconnectEventTargetObjects() when the worker reaches the Terminating state. r=baku
https://hg.mozilla.org/mozilla-central/rev/c1838e2fbb4f
https://hg.mozilla.org/mozilla-central/rev/6337703ef9d8
https://hg.mozilla.org/mozilla-central/rev/6cd2cafb5b21
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.