Open Bug 1653470 Opened 4 years ago Updated 4 months ago

Cleanup of PServiceWorker, PServiceWorkerContainer, and PServiceWorkerRegistration

Categories

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

enhancement

Tracking

()

People

(Reporter: perry, Assigned: edenchuang)

References

Details

(Keywords: leave-open)

Attachments

(4 files, 6 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
  • Making things safe-refcounted makes it safe to increment and decrement the refcount in the ctor
  • It's okay to send to dead actors
Attachment #9164232 - Attachment is obsolete: true
Attachment #9164233 - Attachment is obsolete: true
Attachment #9164234 - Attachment is obsolete: true
Attachment #9164235 - Attachment is obsolete: true
Attachment #9164236 - Attachment is obsolete: true
Attachment #9164237 - Attachment is obsolete: true
  • 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

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

Assignee: perry → ssengupta

Move to Andrew for landing.

Flags: needinfo?(bugmail)
Assignee: ssengupta → bugmail
Pushed by sgiesecke@mozilla.com:
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

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

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 ...
Assignee: bugmail → echuang
Flags: needinfo?(bugmail)

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

Flags: needinfo?(echuang)

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.

Flags: needinfo?(echuang)

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.
Depends on: 1715547
Flags: needinfo?(echuang)
Assignee: echuang → nobody

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.

Flags: needinfo?(hsingh)

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

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

No.

Flags: needinfo?(hsingh)
Assignee: nobody → echuang
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: