Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 11 years ago

NSWs within the relevant scope receive events.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 2 • 10 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 3 • 10 years ago

The scopes stuff was added in Bug 984048 Patch 6.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 4 • 10 years ago

Fire updatefound and currentchange events.

	(no longer active)
	Comment 5 • 10 years ago

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.

	(no longer active)
	Comment 6 • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 7 • 10 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 9 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 10 years ago

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.

	(no longer active)
	Comment 11 • 10 years ago

(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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 13 • 10 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 14 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 10 years ago

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().

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 16 • 10 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 17 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 19 • 10 years ago

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