1653470 - Cleanup of PServiceWorker, PServiceWorkerContainer, and PServiceWorkerRegistration

Status: NEW

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

Description

[Perry Jiang [:perry] [no longer employee, use ni?]]

• Making things safe-refcounted makes it safe to increment and decrement the refcount in the ctor
• It's okay to send to dead actors

Comment 1

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - refcount PServiceWorker r=#dom-workers-and-storage-reviewers

Comment 2

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - refcount PServiceWorkerContainer r=#dom-workers-and-storage-reviewers

Depends on D83883

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - refcount PServiceWorkerRegistration r=#dom-workers-and-storage-reviewers

Depends on D83884

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - simplify PServiceWorker cleanup r=#dom-workers-and-storage-reviewers

Depends on D83885

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - simplify PServiceWorkerContainer cleanup r=#dom-workers-and-storage-reviewers

Depends on D83886

Comment 6

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - simplify PServiceWorkerRegistration cleanup r=#dom-workers-and-storage-reviewers

Depends on D83887

Comment 7

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - simplify ServiceWorkerParent initialization r=#dom-workers-and-storage-reviewers

Depends on D83888

Comment 8

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - simplify ServiceWorkerContainerParent initialization r=#dom-workers-and-storage-reviewers

Depends on D83889

Comment 9

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - simplify ServiceWorkerRegistrationParent initialization r=#dom-workers-and-storage-reviewers

Depends on D83890

Comment 10

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1653470 - Simplify PServiceWorker, PServiceWorkerContainer, and PServiceWorkerRegistration r=#dom-workers-and-storage-reviewers

• Apply pointer guidelines/safe-refcount things
• Use ServiceWorkerChildActor and ServiceWorkerParentActor to share code
  between concrete actor classes
• Get rid of "MaybeSendDelete" because now it is fine to send to dead actors
• Get rid of two-phase init for proxy classes (the two-phase init was to
  prevent the refcount going from 0 to 1 to 0 in the ctor, which isn't possible
  with SafeRefCounted<T>'s refcount initialized to 1).

Depends on D83885

Comment 11

[Perry Jiang [:perry] [no longer employee, use ni?]]

Reassigning to Subhamoy for landing once it's all reviewed.

Comment 12

[Jens Stutte [:jstutte]]

Move to Andrew for landing.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e8e6038be95
refcount PServiceWorker r=dom-workers-and-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/5aea34984375
refcount PServiceWorkerContainer r=dom-workers-and-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/9e89d5e29a61
refcount PServiceWorkerRegistration r=dom-workers-and-storage-reviewers,asuth

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5e8e6038be95
https://hg.mozilla.org/mozilla-central/rev/5aea34984375
https://hg.mozilla.org/mozilla-central/rev/9e89d5e29a61

Comment 15

[Simon Giesecke [:sg] [he/him]]

The open patch is currently assigned to me, but we discussed that Andrew will commandeer it and maybe split it up.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Eden is taking over landing the last patch. I'm pasting my notes from my WIP review that got a bit long because of the number of combined cleanups going on in the patch. Hopefully my notes are helpful and Eden can conclude the changes here are safe. As :sg said in comment 15, we were thinking that perhaps trying to basically split up the patch, possibly by re-doing the patch in smaller atomic changes might help, but maybe we're close enough.

asuth's notes

• General Context

  • PServiceWorkerRegistration: Used by ServiceWorkerRegistration and
    ServiceWorker bindings to be kept up-to-date as to the canonical state of
    the registration as well as make update/unregister calls.
    • The ServiceWorker binding data doesn't travel over PServiceWorker because
      of a need for changes on the ServiceWorkerRegistration and ServiceWorker
      instance (which is visible from the registration) to be perceived
      coherently. This was a design issue that was refined during the original
      IPC implementation.
  • PServiceWorker: Because of the registration coherency issues mentioned
    above, this is primarily about being able to postMessage the ServiceWorker
    from a binding. (And might not have existed had the registration
    coherency issue been understood before this was implemented.)
  • PServiceWorkerContainer: Mainly just a propagation mechanism for the calls
    exposed on the binding.

• IPC high-level:

  • ServiceWorkerParentActor/ServiceWorkerChildActor created to contain common
    lifecycle code with boring SafeRefCounted base class ServiceWorkerActor.
    • Note that there's somewhat of a naming near-collision here, as
      PServiceWorker gives us ServiceWorkerParent and ServiceWorkerChild which
      are different things than the suffixed versions.
  • ServiceWorkerParentActor, template parameters <Actor, Proxy>
    • Parent actors have "Proxy" instances because as
      https://bugzilla.mozilla.org/show_bug.cgi?id=1459209#c35 expresses,
      the parent actors live on PBackground but the ServiceWorkerManager
      authoritative state lives on the main thread, and so the proxies are
      necessary to bridge this gap.
      • In the future the ServiceWorkerManager's authoritative state might move
        to PBackground and this could eliminate the proxies.
    • mProxy of type LazyInitializedOnceNotNullEarlyDestructible which
      provides single-lifetime guarantees that RevokeActor tried to help to
      provide.
      • destroy()ed/cleared by ActorDestroy.
  • ServiceWorkerChildActor, template parameters <Actor>
    • Init method (on the worker) acquires an IPC Worker Ref (that won't prevent
      idle shutdown) which will invoke Send__delete__ when the worker
      transitions to Canceling.
    • In the event the worker has already reached the Canceling state, then
      the worker ref will not be obtained and Init will return false.
    • The WorkerRef will be explicitly dropped when the actor is destroyed.
      This ensures the worker ref is removed when notified and that the ref is
      only held as long as the actor is alive.
    • mWorkerRef is LazyInitializedOnceNotNullEarlyDestructible similar to
      the parent's mProxy.
  • Teardown ping/pong deletion idiom removed. This was made possible by
    changes announced/documented at
    https://groups.google.com/g/mozilla.dev.platform/c/Nb12HBM7pUs/m/hLoxgtNJAgAJ
    with specific changes:
    • PServiceWorker
    • PServiceWorkerContainer
    • PServiceWorkerRegistration
  • Owner/Actor terminology/relationship for remoted binding infra:
    • Remote- binding inner impls held a strong (RefPtr) mActor.
    • The actors held by these Remote- inner impls in turn had a raw pointer
      reference to the owner.
    • The lifecycle here was one where shutdown could happen from either
      direction:
      • The binding could be torn down and result in a call to
        RemoteServiceWorker{,Container,Registration}Impl::Shutdown which would
        invoke RevokeOwner on the actor followed by MaybeStartTeardown.
      • The actor could receive ActorDestroy and then invoke RevokeActor on its
        mOwner which would then synchronously invoke RevokeOwner during that
        call. (Which is also why the calls to RevokeActor were followed by
        diagnostic asserts that mOwner was null after that call.)

• IPC lower level:

  • RevokeActor removed for RemoteServiceWorkerImpl,
    RemoteServiceWorkerContainerImpl, RemoteServiceWorkerRegistrationImpl but
    not ServiceWorkerProxy, ServiceWorkerRegistrationProxy.
    • It provided assertions that the current mActor was non-null and was the
      same actor as the one being removed, nullifying the actor.
    • RSWRI::RevokeActor became ActorDidDestroy which still needs to clear
      mActor and call SWR::RegistrationCleared with a death-grip (content may be
      called).
    • ServiceWorkerProxy retains RevokeActor because it kinda needs to
      do special legwork to drop its ServiceWorkerInfo on the main thread.
      • I say kinda because we're dispatching a runnable to the main thread to
        drop mInfo which is an nsMainThreadPtrHandle. The awkwardness here is
        that from a proxy-release perspective we don't need to do this, but from
        a "what thread touches this" perspective we do.
  • RevokeOwner was replaced by direct calls to Send__delete__

Places where checks were removed in Recv Methods in favor of direct invocations
of MutableProxyRef:

• ServiceWorkerContainerParent: Recv methods
• ServiceWorkerRegistrationParent: Recv methods now directly invoke
  MutableProxyRef()

Minutae (organized by Phabricator alphabetic ordering):

• PServiceWorker: Teardown() gone, __delete__ now bidirectional.
• PServiceWorkerContainer: ditto
• PServiceWorkerRegistration: ditto
• RemoteServiceWorkerContainerImpl:
  • mShutdown removed
    • Previously set to idempotently bail in Shutdown()
  • mActor RefPtr converted to LazyInitializedOnceNotNullEarlyDestructible
    which ensures the lifecycle can happen only once (and that the smart pointer
    can never be null).
  • MutableActorRef added to expose ServiceWorkerContainerChild&
    • The general replacement pattern is s/mActor->/MutableActorRef()./.
  • Shutdown logic simplified
    • actor::RevokeOwner() removed. WHY
    • (actor::MaybeStartTeardown directly converted to __delete__)
  • Init logic changed slightly:
    • mActor now uses life-cycle init() method, SetOwner call gone.
  • RevokeActor method removed:
    • This asserted about the actor being removed and called the actor's
      RevokeOwner() method, which is separately what Shutdown() had been
      calling.
• RemoteServiceWorkerImpl: ditto
• RemoteServiceWorkerRegistationImpl: Partial ditto:
  • Same:
    • mShutdown removal, changes to Shutdown() match.
  • Different:
    • mActor upgraded to SafeRefPtr from RefPtr but not wrapped in a LazyInit.
    • RevokeActor was not removed, but instead renamed to ActorDidDestroy
    • ActorDidDestroy no longer takes the previous actor (which was just for
      more thorough assertions) and no longer calls RevokeOwner() on the
      actor, but continues to make a strongly held call to mOuter's
      RegistrationCleared if mOuter was non-null.
• ServiceWorkerActor:
  • Templated SafeRefCounted base class.
• ServiceWorkerChild:
  • Loses:
    • IPCWorkerRef: moves
    • raw RemoteServiceWorkerImpl mOwner.
    • mTeardownStarted (ping/pong no longer needed)
  • Create method moved up from cpp to header, calls actor Init() with name.
• ServiceWorkerChildActor
  • This is a binding actor that can be created on worker threads or the main
    thread.
  • ActorDestroy:
    • Drops mWorkerRef if not-main-thread. The main thread never gets a worker
      ref because it doesn't need one.
  • Init:
    • IPCWorkerRef created if not-main-thread with a callback that calls
      Send__delete__ when the callback is invoked.
      • Send__delete__ has a CanSend() guard which will return false if the
        actor is being destroyed (state is LinkStatus::Doomed) or already
        destroyed (state is LinkStatus::Destroyted). That makes this action
        idempotent and safe even in the face of ActorDestroy already having
        been called via some other means. (Which shouldn't happen. A child
        process can't survive the death of the parent process and the parent
        won't delete the actor.)
• ServiceWorkerContainerChild
  • The same situation as ServiceWorkerChild.
• ServiceWorkerContainerParent
  • Loses:
    • ActorDestroy: moves into superclass ServiceWorkerParentActor
    • mProxy: moves into superclass
    • RecvTeardown: gone.
    • refcounting: moved into grandparent ServiceWorkerActor.
    • Init: void method (which didn't really need a separate method because
      it's void) partially moved into ServiceWorkerParentActor::InitProxy which
      is now called from the constructor (with the MakeSafeRefPtr concrete class
      creation which is the other half) because it's void and depends on
      infallible construction.
• ServiceWorkerContainerProxy
  • Now AtomicSafeRefCounted (via subclassing template)
  • No longer holds an actor. It didn't actually need the actor.
• ServiceWorkerEvents.cpp: just a minor reformatting.
• ServiceWorkerInfo.cpp: Just an include addition
• ServiceWorkerParent ...

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:edenchuang, maybe it's time to close this bug?

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:edenchuang, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Reflecting the status note I just added to the patch (and updating dependencies).

Status:

• We recognized that this patch is trying to clean up too many things in a single stack, at least for review purposes.
• So we want to reconstitute the key clean-ups up in this patch as a new stack with somewhat more atomic changes.
• There are a number of cleanups in https://bugzilla.mozilla.org/show_bug.cgi?id=1715547 that are very close to landing and which potentially bit-rot any work in this stack, so we want that to land first.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:hsingh, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 21

[Jens Stutte [:jstutte]]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #20)

:hsingh, maybe it's time to close this bug?

No.