Closed Bug 983497 Opened 6 years ago Closed 5 years ago

ServiceWorker: Dispatch relevant events on ServiceWorkerContainer

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(2 files, 6 obsolete files)

6.76 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
17.19 KB, patch
ehsan
: review+
smaug
: review+
Details | Diff | Splinter Review
NSWs within the relevant scope receive events.
Assignee: nobody → nsm.nikhil
Summary: ServiceWorker: Dispatch relevant events on NavigatorServiceWorker → ServiceWorker: Dispatch relevant events on ServiceWorkerContainer
Fire updatefound and currentchange events.
Attachment #8421356 - Flags: review?(ehsan)
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.
Attachment #8421355 - Flags: review?(ehsan) → review+
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.
Attachment #8421356 - Flags: review?(ehsan) → review+
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?
Attachment #8454793 - Flags: review?(bugs)
Flags: needinfo?(ehsan)
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.
Attachment #8454793 - Flags: review?(bugs) → review-
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.
Attachment #8455452 - Flags: review?(bugs)
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.
Attachment #8455452 - Flags: review?(bugs) → review-
(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.
Flags: needinfo?(ehsan)
Made ServiceWorkerDomainInfo refcounted.
Used nsTObserverArray for ServiceWorkerContainer, with rawptrs and using a RefPtr when dispatching events.
See interdiff.
Attachment #8455643 - Flags: review?(ehsan)
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.
Attachment #8455643 - Flags: review?(bugs)
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().
Attachment #8455643 - Flags: review?(bugs) → review+
Attachment #8455643 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/cd3ee2c040e1
https://hg.mozilla.org/mozilla-central/rev/8111e8d8875b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.