1058311 - ServiceWorkerClients API spec changes

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

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

https://github.com/slightlyoff/ServiceWorker/issues/414
&
https://github.com/slightlyoff/ServiceWorker/issues/428

Comment 1

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

Andrea would you like to take this? Depending on issue 428 we may also need to dynamically obtain the list of open windows for an origin.

Comment 2

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

Attachment: ServiceWorkerClients API changes

This was the patch I had. Doesn't modify tests. Maybe we should hold on this until the issues are resolved, since otherwise it's not a good use of resources.

Comment 3

[Andrea Marchesini [:baku]]

> This was the patch I had. Doesn't modify tests. Maybe we should hold on this
> until the issues are resolved, since otherwise it's not a good use of
> resources.

Sure. don't assignee it to yourself if you are not working on. I can take it in case these 2 issues are integrated in the spec.

Comment 4

[Cătălin Badea (:catalinb)]

Some of the changes discussed in 414 have been integrated in the spec:
getServiced  - renamed to getAll and now has an option parameter.
reloadAll - removed.

I think it's safe to switch to getAll without support for the uncontrolled option.

Comment 5

[Cătălin Badea (:catalinb)]

Attachment: Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll

Comment 6

[Cătălin Badea (:catalinb)]

Attachment: Update client interface. Implement matchAll WindowClient.

Comment 7

[Cătălin Badea (:catalinb)]

Attachment: Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll

Comment 8

[Cătălin Badea (:catalinb)]

Comment on attachment 8565998 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

Patch 1. Removes prefix renames getServiced to matchAll. Adds stubs for extra options in matchAll.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8566003 [details] [diff] [review]
Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll

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

lgtm

::: dom/workers/ServiceWorkerClients.h
@@ +9,5 @@
>  
>  #include "nsAutoPtr.h"
>  #include "nsWrapperCache.h"
>  #include "mozilla/dom/WorkerScope.h"
> +#include "mozilla/ErrorResult.h"

alphabetic order. Then, you can just forward ErrorResult.

Comment 10

[Cătălin Badea (:catalinb)]

Attachment: Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll

Comment 11

[Cătălin Badea (:catalinb)]

Updated the patch to have includes in alphabetical order, but didn't used forward declaration because it broke the build in a bunch of ServiceWorker sources (e.g. ServiceWorkerEvents.cpp) that are beyond the scope of this patch.

Comment 12

[Cătălin Badea (:catalinb)]

Comment on attachment 8567094 [details] [diff] [review]
Remove prefix for SWClients. Rename getServiced to matchAll. Add stub query options for matchAll

https://hg.mozilla.org/integration/mozilla-inbound/rev/39354f436e2b

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/39354f436e2b

Comment 14

[Cătălin Badea (:catalinb)]

There is a second patch that needs to be landed first.

Comment 15

[Cătălin Badea (:catalinb)]

Attachment: Update client interface. Implement matchAll WindowClient.

This patch adds WindowClient and implements functionality for its getters.
Furthermore, I updated the Client and Clients webidl and added stubs.

I had difficulty testing frameType == top-level and visibilityState != visible.
For frameType == top-level, :bz suggested using a mochitest-browser, but I
would rather test it after implementing clients.claim.

Any suggestions on how I should cover these cases are welcomed.

Comment 16

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

Catalin, is this a dupe? https://bugzilla.mozilla.org/show_bug.cgi?id=1130685

Comment 17

[Cătălin Badea (:catalinb)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #16)
> Catalin, is this a dupe? https://bugzilla.mozilla.org/show_bug.cgi?id=1130685

No, but it should be renamed to something like "Implement matchAll queryParam option".

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8568833 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

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

It looks good except for the nsAutoPtr. Happy to review it again when this issue is solved. Thanks!

::: dom/base/nsGlobalWindow.h
@@ +678,5 @@
> +  IsTopLevelWindow()
> +  {
> +    MOZ_ASSERT(IsOuterWindow());
> +    nsCOMPtr<nsIDOMWindow> parentWindow;
> +    GetParent(getter_AddRefs(parentWindow));

GetScriptableTop.

::: dom/workers/ServiceWorkerClient.cpp
@@ +29,5 @@
> +{
> +  MOZ_ASSERT(aDoc);
> +  MOZ_ASSERT(aDoc->GetWindow());
> +
> +  nsRefPtr<nsGlobalWindow> outerWindow = static_cast<nsGlobalWindow*>(aDoc->GetWindow());

You don't need to case it to retrieve the WindowID(). Just use nsCOMPtr<nsPIDOMWindow>

@@ +40,5 @@
> +  if (NS_WARN_IF(result.Failed())) {
> +    NS_WARNING("Failed to get focus information.");
> +  }
> +
> +  if (outerWindow->IsTopLevelWindow()) {

wait... if is topLevel it should be FrameType::Top_Level, right?

::: dom/workers/ServiceWorkerClient.h
@@ +18,5 @@
>  namespace workers {
>  
> +// Used as a container object for information needed to create
> +// client objects.
> +class ServiceWorkerClientInfo MOZ_FINAL {

{ in a new line.

@@ +20,5 @@
> +// Used as a container object for information needed to create
> +// client objects.
> +class ServiceWorkerClientInfo MOZ_FINAL {
> +  friend class ServiceWorkerClient;
> +  friend class ServiceWorkerWindowClient;

forward declaration for these 2 classes, or move this class after those.

@@ +35,5 @@
> +  bool mFocused;
> +  FrameType mFrameType;
> +};
> +
> +class ServiceWorkerClient : public nsISupports,

MOZ_FINAL?

::: dom/workers/ServiceWorkerManager.cpp
@@ +2133,5 @@
>    nsMainThreadPtrHandle<nsIInterceptedChannel> mInterceptedChannel;
>    nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
>    nsTArray<nsCString> mHeaderNames;
>    nsTArray<nsCString> mHeaderValues;
> +  nsAutoPtr<ServiceWorkerClientInfo> mClientInfo;

It seems to me that you are usin nsAutoPtr as nsRefPtr. nsAutoPtr deletes the object when its dtor is called.
Probably you want make ServiceWorkerClientInfo refcnted and then use nsRefPtr everywhere here.

@@ +2141,5 @@
>  public:
>    FetchEventRunnable(WorkerPrivate* aWorkerPrivate,
>                       nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
>                       nsMainThreadPtrHandle<ServiceWorker>& aServiceWorker,
> +                     nsAutoPtr<ServiceWorkerClientInfo>& aClientInfo)

This clientInfo will be delete twice. mClientInfo, and in the caller.

Comment 19

[Cătălin Badea (:catalinb)]

Attachment: Update client interface. Implement matchAll WindowClient.

Comment 20

[Cătălin Badea (:catalinb)]

(In reply to Andrea Marchesini (:baku) from comment #18)
> Comment on attachment 8568833 [details] [diff] [review]
> Update client interface. Implement matchAll WindowClient.
> 
> Review of attachment 8568833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good except for the nsAutoPtr. Happy to review it again when this
> issue is solved. Thanks!
> 
> ::: dom/base/nsGlobalWindow.h
> @@ +678,5 @@
> > +  IsTopLevelWindow()
> > +  {
> > +    MOZ_ASSERT(IsOuterWindow());
> > +    nsCOMPtr<nsIDOMWindow> parentWindow;
> > +    GetParent(getter_AddRefs(parentWindow));
> 
> GetScriptableTop.
Fixed.

> ::: dom/workers/ServiceWorkerClient.cpp
> @@ +29,5 @@
> > +{
> > +  MOZ_ASSERT(aDoc);
> > +  MOZ_ASSERT(aDoc->GetWindow());
> > +
> > +  nsRefPtr<nsGlobalWindow> outerWindow = static_cast<nsGlobalWindow*>(aDoc->GetWindow());
>
> You don't need to case it to retrieve the WindowID(). Just use
> nsCOMPtr<nsPIDOMWindow>
I need the nsGlobalWindow cast for IsTopLevel and HadOriginalOpener.

> @@ +40,5 @@
> > +  if (NS_WARN_IF(result.Failed())) {
> > +    NS_WARNING("Failed to get focus information.");
> > +  }
> > +
> > +  if (outerWindow->IsTopLevelWindow()) {
> 
> wait... if is topLevel it should be FrameType::Top_Level, right?
You're right. This was actually doubled by an error in IsTopLevel which caused the test to pass.
Fixed.
> 
> ::: dom/workers/ServiceWorkerClient.h
> @@ +18,5 @@
> >  namespace workers {
> >  
> > +// Used as a container object for information needed to create
> > +// client objects.
> > +class ServiceWorkerClientInfo MOZ_FINAL {
> 
> { in a new line.
> 
> @@ +20,5 @@
> > +// Used as a container object for information needed to create
> > +// client objects.
> > +class ServiceWorkerClientInfo MOZ_FINAL {
> > +  friend class ServiceWorkerClient;
> > +  friend class ServiceWorkerWindowClient;
> 
> forward declaration for these 2 classes, or move this class after those.
Fixed.
> 
> @@ +35,5 @@
> > +  bool mFocused;
> > +  FrameType mFrameType;
> > +};
> > +
> > +class ServiceWorkerClient : public nsISupports,
> 
> MOZ_FINAL?
We now have ServiceWorkerWindowClient which inherits from ServiceWorkerClient.

> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +2133,5 @@
> >    nsMainThreadPtrHandle<nsIInterceptedChannel> mInterceptedChannel;
> >    nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
> >    nsTArray<nsCString> mHeaderNames;
> >    nsTArray<nsCString> mHeaderValues;
> > +  nsAutoPtr<ServiceWorkerClientInfo> mClientInfo;
> 
> It seems to me that you are usin nsAutoPtr as nsRefPtr. nsAutoPtr deletes
> the object when its dtor is called.
> Probably you want make ServiceWorkerClientInfo refcnted and then use
> nsRefPtr everywhere here.
> 
> @@ +2141,5 @@
> >  public:
> >    FetchEventRunnable(WorkerPrivate* aWorkerPrivate,
> >                       nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
> >                       nsMainThreadPtrHandle<ServiceWorker>& aServiceWorker,
> > +                     nsAutoPtr<ServiceWorkerClientInfo>& aClientInfo)
> 
> This clientInfo will be delete twice. mClientInfo, and in the caller.
My intention was to pass the ownership of the ClientInfo object and I believe it works as expected.
Please have another look and let me know if you still think it is wrong.

Thanks for the review!

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8572609 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

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

Thanks for sending this patch for a new review! It's good!
The only big comment is about the allocation of nsTArray. I guess you can just use a reference.

::: dom/base/nsGlobalWindow.h
@@ +682,5 @@
> +  {
> +    MOZ_ASSERT(IsOuterWindow());
> +    nsCOMPtr<nsIDOMWindow> parentWindow;
> +    nsresult rv = GetScriptableTop(getter_AddRefs(parentWindow));
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv))) {
  return false;
}

@@ +686,5 @@
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("Failed to get scriptable top.");
> +    }
> +
> +    return parentWindow.get() == static_cast<nsIDOMWindow*>(this);

do you really need .get() ?

::: dom/workers/ServiceWorkerClient.cpp
@@ +39,5 @@
> +  ErrorResult result;
> +  mFocused = aDoc->HasFocus(result);
> +  if (NS_WARN_IF(result.Failed())) {
> +    NS_WARNING("Failed to get focus information.");
> +  }

no double warning. remove NS_WARN_IF or NS_WARNING.

::: dom/workers/ServiceWorkerClients.cpp
@@ +127,5 @@
>  
>  public:
>    ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
>                                 PromiseHolder* aPromiseHolder,
> +                               nsAutoPtr<nsTArray<ServiceWorkerClientInfo>>& aValue)

What about if this is an NsTArray<ServiceWorkerClientInfo>& aValue and you do: mValue.SwapElements(aValue) ?

@@ +216,5 @@
>        return NS_OK;
>      }
>  
>      nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    nsAutoPtr<nsTArray<ServiceWorkerClientInfo>> result(new nsTArray<ServiceWorkerClientInfo>());

What about:

nsTArray<ServiceWorkerClientInfo> result;
swm->GetAllClients(mScope, &result);

then you swap the elements in ResolvePromiseWorkerRunnable?

@@ +221,4 @@
>  
>      swm->GetAllClients(mScope, result);
>      nsRefPtr<ResolvePromiseWorkerRunnable> r =
>        new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result);

Test: this doesn't need to be allocated.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2565,5 @@
>  
>  class MOZ_STACK_CLASS FilterRegistrationData
>  {
>  public:
> +  FilterRegistrationData(nsTArray<ServiceWorkerClientInfo>* aDocuments,

nsTArray<ServiceWorkerClientInfo>& aDocuments

@@ +2572,5 @@
>      mRegistration(aRegistration)
>    {
>    }
>  
> +  nsTArray<ServiceWorkerClientInfo>* mDocuments;

nsTArray<ServiceWorkerClientInfo>& mDocuments;

@@ +2593,5 @@
>        return PL_DHASH_NEXT;
>    }
>  
> +  ServiceWorkerClientInfo clientInfo(document);
> +  data->mDocuments->AppendElement(clientInfo);

data->mDocuments.AppendElement(clientInfo);

@@ +2641,5 @@
>  } // anonymous namespace
>  
>  void
>  ServiceWorkerManager::GetAllClients(const nsCString& aScope,
> +                                    nsTArray<ServiceWorkerClientInfo>* aControlledDocuments)

nsTArray<ServiceWorkerClienTInfo>& aControlledDocuments)

::: dom/workers/ServiceWorkerManager.h
@@ +381,5 @@
>                uint32_t aFlags);
>  
>    void
>    GetAllClients(const nsCString& aScope,
> +                nsTArray<ServiceWorkerClientInfo>* aControlledDocuments);

nsTArray<ServiceWQorkerClientInfo>& aControlledDocuments);

::: dom/workers/ServiceWorkerWindowClient.cpp
@@ +14,5 @@
> +NS_IMPL_ADDREF_INHERITED(ServiceWorkerWindowClient, ServiceWorkerClient)
> +NS_IMPL_RELEASE_INHERITED(ServiceWorkerWindowClient, ServiceWorkerClient)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServiceWorkerWindowClient)
> +NS_INTERFACE_MAP_END_INHERITING(ServiceWorkerClient)

you can get rid of this.

::: dom/workers/ServiceWorkerWindowClient.h
@@ +15,5 @@
> +
> +class ServiceWorkerWindowClient MOZ_FINAL : public ServiceWorkerClient
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED

do you need this?

::: dom/workers/test/serviceworkers/test_match_all_client_properties.html
@@ +65,5 @@
> +  }
> +
> +  function testAuxiliaryWindow() {
> +    var p = getMessageListener();
> +    var w = window.open(clientURL);

have you tested this test with b2g emulator?
Window.open has to be handled differently, as far as I remember.

Comment 23

[Cătălin Badea (:catalinb)]

Attachment: Update client interface. Implement matchAll WindowClient.

Comment 24

[Cătălin Badea (:catalinb)]

With worker limit removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db636454ce61
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fe99a74ddf3

Comment 25

[Cătălin Badea (:catalinb)]

Comment on attachment 8573783 [details] [diff] [review]
Update client interface. Implement matchAll WindowClient.

https://hg.mozilla.org/integration/mozilla-inbound/rev/16da34714117

Intermittent failures on mochitest-4 might spike, but the real cause is the worker per domain limitation applying on SW: bug 1053275.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16da34714117

Comment 27

[Chris Mills [:cmills]]

See

https://developer.mozilla.org/en-US/docs/Web/API/Client
https://developer.mozilla.org/en-US/docs/Web/API/Clients
https://developer.mozilla.org/en-US/docs/Web/API/WindowClient