983497 - ServiceWorker: Dispatch relevant events on ServiceWorkerContainer

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

NSWs within the relevant scope receive events.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fire install, installend, activate, activateend on relevant NavigatorServiceWorkers.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fire install, installend, activate, activateend on relevant ServiceWorkerContainers.

These only fire simple events for now and not per spec.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Infrastructure to fire events on ServiceWorkerContainers.

The scopes stuff was added in Bug 984048 Patch 6.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2: Fire events on ServiceWorkerContainers.

Fire updatefound and currentchange events.

Comment 5

[(no longer active)]

Comment on attachment 8421355 [details] [diff] [review]
Patch 1: Infrastructure to fire events on ServiceWorkerContainers.

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

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +22,5 @@
>    void MaybeStopControlling(in nsIDocument aDoc);
>  
> +  // aTarget MUST be a ServiceWorkerContainer.
> +  void AddContainerEventListener(in nsIURI aPageURI, in nsIDOMEventTarget aTarget);
> +  void RemoveContainerEventListener(in nsIURI aPageURI, in nsIDOMEventTarget aTarget);

Rev the uuid please.

Also, please make these [noscript] to make sure that the static_cast to ServiceWorkerContainer* is going to remain correct in the future.

::: dom/workers/ServiceWorkerContainer.h
@@ +72,5 @@
>    already_AddRefed<Promise>
>    WhenReady(ErrorResult& aRv);
>  
> +  nsIURI*
> +  GetDocumentURI() const {

Nit: brace on its own line.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1341,5 @@
> +    domainInfo = new ServiceWorkerDomainInfo;
> +    mDomainMap.Put(domain, domainInfo);
> +  }
> +
> +  MOZ_ASSERT(domainInfo);

I think this assertion is pointless since new is infallible, but it probably makes sense to keep it to make sure someone else doesn't break this later on...

@@ +1357,5 @@
> +  if (!domainInfo) {
> +    return NS_OK;
> +  }
> +
> +  MOZ_ASSERT(domainInfo);

Ditto.

@@ +1367,5 @@
> +
> +void
> +ServiceWorkerManager::FireEventOnServiceWorkerContainers(
> +  ServiceWorkerRegistration* aRegistration,
> +  ServiceWorkerManager::FireEventFunc aFunc)

Since both of these events are simple events, I think it's better to send the name of the event here instead of a function pointer.

@@ +1389,5 @@
> +      if (scope.IsEmpty()) {
> +        continue;
> +      }
> +
> +      if (!scope.Equals(aRegistration->mScope)) {

Please || with the previous condition.

::: dom/workers/ServiceWorkerManager.h
@@ +228,5 @@
> +    // This array can't be stored in ServiceWorkerRegistration because one may
> +    // not exist when a certain window is opened, but we still want that
> +    // window's container to be notified if it's in scope.
> +    // The containers inform the SWM on creation and destruction.
> +    nsTArray<ServiceWorkerContainer*> mServiceWorkerContainers;

Please make these strong references to avoid a UAF risk in the future.

Comment 6

[(no longer active)]

Comment on attachment 8421356 [details] [diff] [review]
Patch 2: Fire events on ServiceWorkerContainers.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +937,5 @@
> +{
> +  nsRefPtr<Event> event = new Event(aEventTarget, nullptr, nullptr);
> +  event->InitEvent(NS_LITERAL_STRING("updatefound"), false, true);
> +  event->SetTrusted(true);
> +  aEventTarget->DispatchDOMEvent(nullptr, event, nullptr, nullptr);

This looks sane, but please double check this part with smaug.  He knows this stuff much better than I do.

::: dom/workers/ServiceWorkerManager.h
@@ +349,5 @@
> +  void
> +  FireUpdateFoundEvent(EventTarget* aEventTarget);
> +
> +  void
> +  FireCurrentChangeEvent(EventTarget* aEventTarget);

Based on the comments on part 1, these should be collapsed into a function that takes the name of the event as an argument.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Infrastructure to fire events on ServiceWorkerContainers

Olli,

Can you look at the FireEventOnServiceWorkerContainers method where I create the event and dispatch it.
The spec says "fire a simple event named <foo>".

Ehsan,

You mentioned to use a nsRefPtr<ServiceWorkerContainer>, but looking at this again, how will the container ever get destroyed (the dtor calls StopListeningForEvents()) if the ServiceWorkerManager holds on to it?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8454793 [details] [diff] [review]
Patch 1: Infrastructure to fire events on ServiceWorkerContainers

>+ServiceWorkerManager::FireEventOnServiceWorkerContainers(
>+  ServiceWorkerRegistration* aRegistration,
>+  const nsAString& aName)
>+{
>+  ServiceWorkerDomainInfo* domainInfo =
>+    GetDomainInfo(aRegistration->mScriptSpec);
>+
>+  if (domainInfo) {
>+    for (uint32_t i = 0; i < domainInfo->mServiceWorkerContainers.Length(); ++i) {
What keeps domainInfo alive here?


>+      ServiceWorkerContainer* target =
>+        domainInfo->mServiceWorkerContainers[i];
>+
>+      nsIURI* targetURI = target->GetDocumentURI();
>+      nsCString path;
>+      nsresult rv = targetURI->GetSpec(path);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        continue;
>+      }
>+
>+      nsCString scope = FindScopeForPath(domainInfo->mOrderedScopes, path);
>+      if (scope.IsEmpty() ||
>+          !scope.Equals(aRegistration->mScope)) {
>+        continue;
>+      }
>+
>+      MOZ_ASSERT(target);
>+
>+      nsCOMPtr<nsIDOMEvent> event;
>+      rv = NS_NewDOMEvent(getter_AddRefs(event), target, nullptr, nullptr);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        continue;
>+      }
>+
>+      rv = event->InitEvent(aName, false, false);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        continue;
>+      }
>+
>+      event->SetTrusted(true);
>+      target->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
So you need to just dispatch a trusted simple event to a DETH.
target->DispatchTrustedEvent(aName);
(I guess you need to make the method public, currently it is protected)
target should be at least per COM rules be stored in a strong pointer, since
dispatching the event might otherwise kill the object.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Infrastructure to fire events on ServiceWorkerContainers

The domainInfo is kept alive by the ServiceWorkerManager object in the nsClassHashtable mDomainMap, which is only deleted at shutdown, in the main thread.
target is now a nsRefPtr<>.
Made DETH::DispatchTrustedEvent public.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8455452 [details] [diff] [review]
Patch 1: Infrastructure to fire events on ServiceWorkerContainers

>+ServiceWorkerManager::FireEventOnServiceWorkerContainers(
>+  ServiceWorkerRegistration* aRegistration,
>+  const nsAString& aName)
>+{
>+  AssertIsOnMainThread();
>+  ServiceWorkerDomainInfo* domainInfo =
>+    GetDomainInfo(aRegistration->mScriptSpec);
>+
>+  if (domainInfo) {
>+    for (uint32_t i = 0; i < domainInfo->mServiceWorkerContainers.Length(); ++i) {
Ok, so domainInfo is guaranteed to be alive only to state X, but nothing guarantees dispatching events here
doesn't go after that state.
So, either ServiceWorkerDomainInfo needs to become refcounted, or the loop needs to check that we aren't shutting down.

What should happen if an event listener ends up adding more items to mServiceWorkerContainers or removing stuff from it?
I don't see anything guaranteeing all the needed mServiceWorkerContainers actually get the event in case items are removed.
You probably should use ObserverArray.

>+      nsRefPtr<ServiceWorkerContainer> target =
>+        domainInfo->mServiceWorkerContainers[i];
>+
>+      nsIURI* targetURI = target->GetDocumentURI();
>+      nsCString path;
>+      nsresult rv = targetURI->GetSpec(path);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        continue;
>+      }
>+
>+      nsCString scope = FindScopeForPath(domainInfo->mOrderedScopes, path);
>+      if (scope.IsEmpty() ||
>+          !scope.Equals(aRegistration->mScope)) {
>+        continue;
>+      }
I have no idea what this is about so someone else needs to review.

Comment 11

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #7)
> Ehsan,
> 
> You mentioned to use a nsRefPtr<ServiceWorkerContainer>, but looking at this
> again, how will the container ever get destroyed (the dtor calls
> StopListeningForEvents()) if the ServiceWorkerManager holds on to it?

You're right.  But please add comments etc as relying on this is sort of fragile.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: interdiff

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Infrastructure to fire events on ServiceWorkerContainers

Made ServiceWorkerDomainInfo refcounted.
Used nsTObserverArray for ServiceWorkerContainer, with rawptrs and using a RefPtr when dispatching events.
See interdiff.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8455643 [details] [diff] [review]
Patch 1: Infrastructure to fire events on ServiceWorkerContainers

Olli,

Again, only ::FireEventOnServiceWorkerContainers.
The scopes part in that is for Ehsan.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8455643 [details] [diff] [review]
Patch 1: Infrastructure to fire events on ServiceWorkerContainers

Note, nsIDocument::GetDocumentURI may return null.
(and it is not clear to me whether you want to use that method.)


Perhaps define nsRefPtr<ServiceWorkerContainer> target; inside the while().

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://tbpl.mozilla.org/?tree=Try&rev=4c0b041afbb0

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3ee2c040e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/8111e8d8875b

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd3ee2c040e1
https://hg.mozilla.org/mozilla-central/rev/8111e8d8875b

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/83220639136f