Closed Bug 1438211 Opened 2 years ago Closed 2 years ago

expose ServiceWorkerRegistration factory methods on nsIGlobalObject

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(9 files, 14 obsolete files)

3.43 KB, patch
asuth
: review+
Details | Diff | Splinter Review
5.36 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.41 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.74 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.06 KB, patch
asuth
: review+
Details | Diff | Splinter Review
8.72 KB, patch
asuth
: review+
Details | Diff | Splinter Review
6.81 KB, patch
asuth
: review+
Details | Diff | Splinter Review
14.07 KB, patch
asuth
: review+
Details | Diff | Splinter Review
6.06 KB, patch
asuth
: review+
Details | Diff | Splinter Review
Currently nsGlobalWindowInner and WorkerGlobalScope have different ways to create ServiceWorkerRegistration binding objects.  We should unify this into an nsIGlobalObject factory method.  We need to pipe it through the global since we want to always return the same registration for the same backing internal registration.

I need this for bug 1436812 in order to avoid writing a lot of window-specific vs worker-specific code.
Priority: -- → P2
Attachment #8951266 - Attachment description: P1 Add nsIGlobalObject::GetOrCreateServiceWorkerRegistration() and assoicated virtual methods. r=asuth → P2 Implement nsGlobalWindowInner::GetOrCreateServiceWorkerRegistration() and associated methods. r=asuth
Attached patch wip (obsolete) — Splinter Review
Work-in-progress patch.

Before I finish this bug, though, I realized I have to make DETH work correctly on non-window globals.
Depends on: 1438541
Attached patch wip (obsolete) — Splinter Review
Attachment #8951268 - Attachment is obsolete: true
Attachment #8951269 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8952245 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Attachment #8953151 - Attachment is obsolete: true
Depends on: 1440407
Attached patch wip (obsolete) — Splinter Review
Attachment #8953152 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
We need to add a registration identifier.

Also, I think I was being a bit too aggressive with letting these objects be garbage collected.  We need to hold them alive until the window dies or the backing implementation is torn down.

I think there are still problems in this patch, but lets see how bad it is:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e3bbeaf0bc1a2f42854e4789c565f971dc93d89
Comment on attachment 8952242 [details] [diff] [review]
P1 Add nsIGlobalObject::GetOrCreateServiceWorkerRegistration(). r=asuth

Andrew, this adds the virtual nsIGlobalObject::GetOrCreateServiceWorkerRegistration() method.  The default implementation just asserts that its not called on globals that should not be creating registrations.
Attachment #8952242 - Flags: review?(bugmail)
Comment on attachment 8952243 [details] [diff] [review]
P2 Implement nsGlobalWindowInner::GetOrCreateServiceWorkerRegistration(). r=asuth

This patch adds an nsGlobalWindowInner implementation of the virtual method added in P1.  It uses the common list of DETH objects in nsIGlobalObject to find existing registrations.  This is done via the ForEachEventTargetObject() iteration method.  This requires making ServiceWorkerRegistration QI'able, however, so we can distinguish it from the other DETH objects.
Attachment #8952243 - Flags: review?(bugmail)
Comment on attachment 8952244 [details] [diff] [review]
P3 Implement WorkerGlobalScope::GetOrCreateServiceWorkerRegistration(). r=asuth

This adds the WorkerGlobalScope version of GetOrCreateServiceWorkerRegistration().
Attachment #8952244 - Flags: review?(bugmail)
Comment on attachment 8953150 [details] [diff] [review]
P4 Actually set the worker global on ServiceWorkerRegistration. r=asuth

Previously we were calling ServiceWorkerRegistration::CreateForWorker() directly in the ServiceWorkerGlobalScope() constructor.  At this point the WorkerPrivate did not have the scope set yet (because it was still being created).  So our code to get the scope from the worker private didn't work.

This patch makes us explicitly pass global reference in as another argument instead of trying to get it out of the WorkerPrivate.

A consequence of this change, though, is now we actually try to create ServiceWorker objects on the worker thread.  We're not ready to do this yet, so this patch also adds an NS_IsMainThread() check in UpdateState().
Attachment #8953150 - Flags: review?(bugmail)
Comment on attachment 8954955 [details] [diff] [review]
P5 Replace direct window/worker calls with nsIGlobalObject::GetOrCreateServiceWorkerRegistration(). r=asuth

This patch just makes us call nsIGlobalObject::GetOrCreateServiceWorkerRegistration() whenever we need a registration binding object.
Attachment #8954955 - Flags: review?(bugmail)
Comment on attachment 8955288 [details] [diff] [review]
P6 Add an integer identifier to each ServiceWorkerRegistrationDescriptor. r=asuth

The legacy code path used to explicitly evict the registration binding object from the window's cache when the registration was removed from SWM.  This is the InvalidateServiceWorkerRegistration() call in RegistrationRemoved().  This was necessary to prevent the code from finding an old registration binding object when unregister() and register() were called in series in the same global.

We can't really rely on this mechanism any more.  While we could do some kind of manual invalidation right now, what we really want to do instead is just make the new registration not match the old one.  The only reason that matching happened before is because the legacy code only looked at the scope string.

This patch fixes the issue for the new code path by adding a one-up integer identifier.  Its basically the same as the ServiceWorkerDescriptor id value.

By matching on the id value in addition to scope/principal, we are able to distinguish separate registrations when unregister()/register() are called within the same global.
Attachment #8955288 - Flags: review?(bugmail)
Comment on attachment 8955642 [details] [diff] [review]
P7 Hold the ServiceWorker object alive until either the window is closed or the backing ServiceWorkerInfo becomes redundant. r=asuth

It turns out my previous changes were a bit to overzealous in allowing the binding objects to be GC'd.  We need to hold them alive until either:

1) We are sure the content script cannot ask for them again.
2) We are sure the underlying implementation will never return them to content script again.
3) We are sure that we will not need to fire an event on the binding object ever again.

This is necessary so events fire even if the script is not holding the DOM object alive.  Its also necessary to preserve expandos across periods where the script is not holding the DOM object alive.

This patch does that by creating a ref-cycle between the ServiceWorker binding object and the inner ServiceWorkerInfo.  The cycle is broken when the global calls DisconnectFromOwner() or the ServiceWorkerInfo transitions to the redundant state.
Attachment #8955642 - Flags: review?(bugmail)
Comment on attachment 8955644 [details] [diff] [review]
P8 Hold the ServiceWorkerRegistration alive until the global is detached or the backing ServiceWorkerRegistrationInfo is removed. r=asuth

This patch is like P7, but for ServiceWorkerRegistration binding objects instead of ServiceWorker.

We add a ref-cycle between the ServiceWorkerRegistration and the impl inner object.  Again we break the cycle if the global calls DisconnectFromOwner().  We also break the cycle if ServiceWorkerManager calls RegistrationRemoved().
Attachment #8955644 - Flags: review?(bugmail)
Comment on attachment 8955645 [details] [diff] [review]
P9 Remove nsPIDOMWindowInner::GetServiceWorkerRegistration() and InvalidateServiceWorkerRegistration(). r=asuth

Finally, this removes the old nsPIDOMWindowInner::GetServiceWorkerRegistration() and InvalidateServiceWorkerRegistration() methods.  It also removes the hash table they were using.
Attachment #8955645 - Flags: review?(bugmail)
Attachment #8952242 - Flags: review?(bugmail) → review+
Attachment #8952243 - Flags: review?(bugmail) → review+
Attachment #8952244 - Flags: review?(bugmail) → review+
Attachment #8953150 - Flags: review?(bugmail) → review+
Attachment #8954955 - Flags: review?(bugmail) → review+
Attachment #8955288 - Flags: review?(bugmail) → review+
Comment on attachment 8955642 [details] [diff] [review]
P7 Hold the ServiceWorker object alive until either the window is closed or the backing ServiceWorkerInfo becomes redundant. r=asuth

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

Fantastic comments.
Attachment #8955642 - Flags: review?(bugmail) → review+
Comment on attachment 8955644 [details] [diff] [review]
P8 Hold the ServiceWorkerRegistration alive until the global is detached or the backing ServiceWorkerRegistrationInfo is removed. r=asuth

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

The removal bounce complexity, at least for a human to understand, is surprisingly high.  This will be amazingly nice once it's normalized to be actor-based.

Here's my notes from analyzing the dead prototype, yuck:
## Part 8

### Glitch:
* WorkerListener::RegistrationRemovedWorkerThread prototype/decl is added, but
  never actually implemented/defined.
  * WorkerListener is the on-worker-thread implementation.

#### Surrounding Context:

There's 2 interfaces going on:
* ServiceWorkerRegistrationListener: How SWM talks to registrations.  Methods
  are invoked on the main thread.
* ServiceWorkerRegistration::Inner: Implementation indirection so we can switch
  from same-process to remoted but still using the same outer binding class in
  the future.  Currently also handles the main thread/worker thread differences.

There are 2 implementation classes, each of which implement SWR::Inner, they
respectively handle main/worker thread:
* ServiceWorkerRegistrationMainThread: Implements
  ServiceWorkerRegistrationListener itself because it already lives on the main
  thread.
* ServiceWorkerRegistrationWorkerThread: Lives on the worker thread, so it can't
  (sanely) implement ServiceWorkerRegistrationListener itself.  Instead, we have
  WorkerListener:
  * WorkerListener: Created by ServiceWorkerRegistrationWorkerThread to handle
    the main-thread/worker-thread dance.

#### remove() changes
These are the changes that happened in the remove() space:

Main Thread:
* ServiceWorkerRegistrationMainThread::RegistrationRemovedInternal introduced,
  new home to the conditionalized InvalidateServiceWorkerRegistration() call.
  * Newly invokes StopListeningForEvents().  Previously that was only called by
    ClearServiceWorkerRegistration which was in turn called by SWR's destructor
    and its DOMEventTargetHelper::DisconnectFromOwner() override.  This makes
    sense because removal is the end of the line.
  * Newly nulls out mOuter, with nice comment explaining about the cycle
    breaking.
  * This was done so that...
* ServiceWorkerRegistrationMainThread::RegistrationRemoved uses
  NewRunnableMethod to defer execution of the internal method so that other
  runnables that update the registration state don't get caught out.
  * Now that most of the SWR::Inner implementations throw if !mOuter, this
    deferral makes sense because previously moot API calls would work.  Now they
    throw, likely resulting in annoying test breakage.

WorkerThread impl on Main Thread:
* WorkerListener::RegistrationRemoved, which was an inline assertion that it was
  invoked on the main thread, is now an override with separate implementation.
  * It invokes the new runnable to invoke RegistrationRemoved on the worker
    thread:
* RegistrationRemovedWorkerRunnable introduced to
  invoke ServiceWorkerRegistrationWorkerThread::RegistrationRemoved on the
  Worker thread.


WorkerThread impl on Worker Thread:
* ServiceWorkerRegistrationWorkerThread::RegistrationRemoved prototype/decl
  introduced in header.
* ServiceWorkerRegistrationWorkerThread::RegistrationRemoved just nulls out
  mOuter.
* The confusing WorkerListener::RegistrationRemovedWorkerThread prototype was
  added.
* ServiceWorkerRegistrationWorkerThread::Notify nulls out mOuter, with a comment
  that explains why posting a runnable doesn't work.

#### Conclusion

Options:
* SWRWT::RegistrationRemoved ended up being what
  WorkerListener::RegistrationRemovedWorkerThread thought it was going to be.

::: dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp
@@ +830,5 @@
>      MOZ_ASSERT(!mListeningForEvents);
>    }
> +
> +  void
> +  RegistrationRemovedWorkerThread();

This prototype without corresponding implementation or callers looks like it's leftover from what the newly introduced WorkerListener::RegistrationRemoved => RegistrationRemovedWorkerRunnable => ServiceWorkerRegistrationWorkerThread::RegistrationRemoved thread bounce call-chain accomplishes.  I'm removing this in my landing.
Attachment #8955644 - Flags: review?(bugmail) → review+
Attachment #8955645 - Flags: review?(bugmail) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a255b87cb3dbbbf58a62a5b8e195703c24474c8
Bug 1438211 P1 Add nsIGlobalObject::GetOrCreateServiceWorkerRegistration(). r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7708d7ee6961d8d38acecd27d8bea1d5176dab
Bug 1438211 P2 Implement nsGlobalWindowInner::GetOrCreateServiceWorkerRegistration(). r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/658d8694715a91b86fc778c9d4d46adbd4ffad54
Bug 1438211 P3 Implement WorkerGlobalScope::GetOrCreateServiceWorkerRegistration(). r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee87211590b5c0640ceb9c08a1c2cf21731889ed
Bug 1438211 P4 Actually set the worker global on ServiceWorkerRegistration. r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/98cfdc9a78265eaa1369b0f0f2c53a7ed6fa46d6
Bug 1438211 P5 Replace direct window/worker calls with nsIGlobalObject::GetOrCreateServiceWorkerRegistration(). r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/43c8c3b917ea151d00dda882ff3b4d39e0befbf1
Bug 1438211 P6 Add an integer identifier to each ServiceWorkerRegistrationDescriptor. r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/076cd67616db7b970a0ac9ca19d4da920481b140
Bug 1438211 P7 Hold the ServiceWorker object alive until either the window is closed or the backing ServiceWorkerInfo becomes redundant. r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcea03ca0cdabc5a8b84f36f0b222b36ab866a6f
Bug 1438211 P8 Hold the ServiceWorkerRegistration alive until the global is detached or the backing ServiceWorkerRegistrationInfo is removed. r=asuth

https://hg.mozilla.org/integration/mozilla-inbound/rev/42f6cbb08529f5edbb1a7069b6e3ebb610e92d3a
Bug 1438211 P9 Remove nsPIDOMWindowInner::GetServiceWorkerRegistration() and InvalidateServiceWorkerRegistration(). r=asuth
Depends on: 1447871
You need to log in before you can comment on or make changes to this bug.