ServiceWorkers: Implement interaction with controlled windows from worker scope.

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nsm, Assigned: catalinb)

Tracking

unspecified
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(10 attachments, 35 obsolete attachments)

30.47 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
5.22 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
5.60 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
2.36 KB, patch
baku
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
4.28 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
33.45 KB, patch
Details | Diff | Splinter Review
3.27 KB, patch
baku
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
15.45 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
1.08 KB, patch
nsm
: review+
catalinb
: checkin+
Details | Diff | Splinter Review
4.70 KB, patch
nsm
: review+
catalinb
: checkin+
Details | Diff | Splinter Review
Assignee: nobody → cbadea
(Assignee)

Comment 1

5 years ago
Created attachment 8456568 [details] [diff] [review]
WIP: ServiceWorkerClients and Client interfaces with getServiced().
(Assignee)

Comment 2

5 years ago
Comment on attachment 8456568 [details] [diff] [review]
WIP: ServiceWorkerClients and Client interfaces with getServiced().

Nikhil, could you please take a look at the patch and give me some early feedback?
Attachment #8456568 - Flags: review?(nsm.nikhil)
Comment on attachment 8456568 [details] [diff] [review]
WIP: ServiceWorkerClients and Client interfaces with getServiced().

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

Excellent!

::: dom/bindings/Bindings.conf
@@ +230,5 @@
>  },
>  
> +'Client': {
> +    'nativeType': 'mozilla::dom::workers::Client',
> +    'headerFile': 'mozilla/dom/workers/bindings/Client.h',

If this is only available on ServiceWorkerGlobalScope, you should add |workers: True| and use the '_workers' form of namespace so that it isn't exposed on the main thread.

@@ +1069,5 @@
>  
> +'ServiceWorkerClients': {
> +    'nativeType': 'mozilla::dom::workers::ServiceWorkerClients',
> +    'headerFile': 'mozilla/dom/workers/bindings/ServiceWorkerClients.h',
> +},

same for this.

::: dom/workers/Client.h
@@ +20,5 @@
> +
> +namespace workers {
> +
> +class Client MOZ_FINAL : public nsISupports,
> +                         public nsWrapperCache {

Nit: { on newline.

@@ +23,5 @@
> +class Client MOZ_FINAL : public nsISupports,
> +                         public nsWrapperCache {
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(Client)

This doesn't hold an jsvals, so don't need to be SCRIPT_HOLDER.

::: dom/workers/ServiceWorkerClients.cpp
@@ +62,5 @@
> +    }
> +    mPromise->MaybeResolve(ret);
> +
> +    // release the reference on the worker thread.
> +    nsRefPtr<Promise> kungFu = mPromise.forget();

This isn't a kungFuDeathGrip :) I'd call it something different.

@@ +75,5 @@
> +  nsCString mDomain;
> +  nsCString mScope;
> +public:
> +  explicit GetServicedRunnable(WorkerPrivate* aWorkerPrivate,
> +                               nsRefPtr<Promise> aPromise,

Promise* aPromise?

@@ +83,5 @@
> +      mPromise(aPromise),
> +      mDomain(aDomain),
> +      mScope(aScope)
> +  {
> +  }

I'm assuming you intend to do AddFeature/RemoveFeature before landing this.

::: dom/workers/ServiceWorkerClients.h
@@ +25,5 @@
> +class ServiceWorkerClients MOZ_FINAL : public nsISupports,
> +                                       public nsWrapperCache {
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ServiceWorkerClients)

Same here about SCRIPT_HOLDER.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1245,5 @@
> +ServiceWorkerManager::GetServicedClients(const nsCString& aDomain,
> +  const nsCString& aScope, nsTArray<uint64_t>* controlledDocuments)
> +{
> +  nsRefPtr<ServiceWorkerDomainInfo> domainInfo;
> +  mDomainMap.Get(aDomain, getter_AddRefs(domainInfo));

You can now use GetDomainInfo(). Although not strictly required here, let's keep accesses to mDomainMap in one location.

@@ +1247,5 @@
> +{
> +  nsRefPtr<ServiceWorkerDomainInfo> domainInfo;
> +  mDomainMap.Get(aDomain, getter_AddRefs(domainInfo));
> +
> +  nsRefPtr<ServiceWorkerRegistration> registration = domainInfo->GetRegistration(aScope);

Add an assertion that this can never be null.
Attachment #8456568 - Flags: review?(nsm.nikhil) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 8459813 [details] [diff] [review]
ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()
Attachment #8456568 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 8459855 [details] [diff] [review]
ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()
Attachment #8459813 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 8459857 [details] [diff] [review]
test_kill_worker.patch

Close the worker right after dispatching GetServiced to test that the promise is released gracefully.
(Assignee)

Comment 7

5 years ago
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #3)
I made all the changes you requested, except for the SCRIPT_HOLDER as we've discussed offline.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=7767ee6b112a

Try run with getServiced closing the worker right after the first dispatch (test_kill_worker attachment):
https://tbpl.mozilla.org/?tree=Try&rev=dd74942841d8
(Assignee)

Comment 8

5 years ago
Created attachment 8459874 [details] [diff] [review]
Patch 2 - Add postMessage to service workers.
(Assignee)

Comment 9

5 years ago
Created attachment 8459981 [details] [diff] [review]
Patch 3 - Implement client.postMessage and add tests for getServiced().
(Assignee)

Comment 10

5 years ago
Comment on attachment 8459855 [details] [diff] [review]
ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()

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

Kyle, please have a look at this patch - the PromiseHolder / WorkerFeature part in particular.
Attachment #8459855 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Attachment #8459874 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

5 years ago
Attachment #8459981 - Flags: review?(nsm.nikhil)
Comment on attachment 8459981 [details] [diff] [review]
Patch 3 - Implement client.postMessage and add tests for getServiced().

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

Please also add a test that enumerates the array of clients and postmessages to all of them and ensure that messages are received.

r=me with the below changes.

::: dom/webidl/Client.webidl
@@ +14,1 @@
>    void postMessage(any message, DOMString targetOrigin,

https://github.com/slightlyoff/ServiceWorker/issues/190 ?

Please remove targetorigin.

::: dom/workers/Client.cpp
@@ +91,5 @@
> +    if (!window) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    AutoSafeJSContext cx;

Please use AutoJSAPI.

@@ +99,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +void Client::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,

Do get a once over from khuey/bent (feedback?) on this method. It seems like you distilled much of the worker messaging stuff, into what is required only for worker thread to main thread messaging. Good!

::: dom/workers/test/serviceworkers/message_posting_worker.js
@@ +2,5 @@
> +  self.clients.getServiced().then(function(res) {
> +    if (!res.length) {
> +      dump("ERROR: no clients are currently controlled.\n");
> +    }
> +      res[0].postMessage(e.data, "*");

Indentation.

::: dom/workers/test/serviceworkers/test_get_serviced_advanced.html
@@ +15,5 @@
> +<pre id="test"></pre>
> +<script class="testbody" type="text/javascript">
> +
> +  function start() {
> +    opened = [];

Just define this outside of the function. Do use var.

@@ +17,5 @@
> +
> +  function start() {
> +    opened = [];
> +    return navigator.serviceWorker.register("get_serviced_worker_advanced.js").then(function(e) {
> +      worker = e;

Same two comments here as in test_post_message.html.

::: dom/workers/test/serviceworkers/test_post_message.html
@@ +16,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +  function start() {
> +    magic_value = "MAGIC_VALUE_123";
> +    return navigator.serviceWorker.register("message_posting_worker.js").then(function(e) {

Please pass a scope localized to the dom/workers/test/serviceworkers directory at least :)
Also unregister() the worker in runTest() below.

Nit: Call the parameter sw instead of e.

@@ +17,5 @@
> +
> +  function start() {
> +    magic_value = "MAGIC_VALUE_123";
> +    return navigator.serviceWorker.register("message_posting_worker.js").then(function(e) {
> +      worker = e;

Please declare worker in the global scope before using it here.

@@ +48,5 @@
> +      .then(function(e) {
> +        SimpleTest.finish();
> +      }).catch(function(e) {
> +        ok(false, "Some test failed with error " + e);
> +        SimpleTest.finish();

You can avoid the duplicate SimpleTest.finish() by using

.then(testPostMessage)
.catch(function(e) {
  ok(false, ...);
})
.then(SimpleTest.finish)
Attachment #8459981 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8459874 [details] [diff] [review]
Patch 2 - Add postMessage to service workers.

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

r=me with the [Throws] removed.

::: dom/webidl/ServiceWorker.webidl
@@ +19,5 @@
>    attribute EventHandler onstatechange;
> +
> +  // FIXME(catalinb): Temporary fix needed for testing service workers.
> +  [Throws]
> +  void postMessage(any message, optional sequence<Transferable> transferable);

The worker spec does not define postMessage as throwing. See my comment below.

::: dom/workers/ServiceWorker.cpp
@@ +49,5 @@
> +ServiceWorker::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
> +                           const Optional<Sequence<JS::Value>>& aTransferable,
> +                           ErrorResult& aRv)
> +{
> +  WorkerPrivate* workerPrivate = GetWorkerPrivate();

Please add an assertion for workerPrivate.

@@ +50,5 @@
> +                           const Optional<Sequence<JS::Value>>& aTransferable,
> +                           ErrorResult& aRv)
> +{
> +  WorkerPrivate* workerPrivate = GetWorkerPrivate();
> +  workerPrivate->PostMessage(aCx, aMessage, aTransferable, aRv);

You can just create an |ErrorResult result;| on the stack here and pass it to PostMessage.
Attachment #8459874 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Updated

5 years ago
Attachment #8459855 - Flags: review?(nsm.nikhil)
Attachment #8459855 - Flags: review?(khuey)
Attachment #8459855 - Flags: feedback?(khuey)
Comment on attachment 8459855 [details] [diff] [review]
ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()

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

Kyle/Ben should look at the Promise keep alive.
You'll need dom peer review on the webidl.

::: dom/webidl/Client.webidl
@@ +9,5 @@
> + */
> +
> +interface Client {
> +  readonly attribute unsigned long id;
> +  void postMessage(any message, DOMString targetOrigin,

remove targetorigin here too.

::: dom/workers/Client.h
@@ +35,5 @@
> +  }
> +
> +  uint32_t Id() const
> +  {
> +    // XXXcatalinb the id exposed to JS should be mangled.

In discussion with catalin, this comment is no longer relevant.

::: dom/workers/ServiceWorkerClients.cpp
@@ +202,5 @@
> +
> +  return promise.forget();
> +}
> +
> +already_AddRefed<Promise> ServiceWorkerClients::ReloadAll()

Please add a FIXME comment with Bug number to actually implement this.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1252,5 @@
> +  const nsCString& aScope, nsTArray<uint64_t>* controlledDocuments)
> +{
> +  nsRefPtr<ServiceWorkerDomainInfo> domainInfo = GetDomainInfo(aScriptURL);
> +
> +  nsRefPtr<ServiceWorkerRegistration> registration = domainInfo->GetRegistration(aScope);

You can just use SWM::GetRegistration(aScope) since aScope is a full URL. That way you don't need the aScriptURL argument.
Attachment #8459855 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Updated

5 years ago
Blocks: 1045257
Comment on attachment 8459855 [details] [diff] [review]
ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()

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

It's only ok to call Clean() on the worker thread (since you're going to call RemoveWorkerFeature).  Please add an assertion for that.

Checking mClean on the main thread is unnecessary.  If the worker has shut down already ResolveWorkerPromiseRunnable will fail to dispatch, and if it shuts down between dispatch and running that event the runnable will be canceled and discarded.  Getting rid of that check also lets you get rid of the lock.

I didn't look at this exhaustively.

::: dom/workers/ServiceWorkerClients.cpp
@@ +60,5 @@
> +  }
> +
> +  Promise* Get() const
> +  {
> +    return mPromise.get();

You should be able to just return mPromise, I would think.

@@ +107,5 @@
> +public:
> +  ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
> +                               PromiseHolder* aPromiseHolder,
> +                               nsAutoPtr<nsTArray<uint64_t> > aValue)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount),

This causes script to run, so it needs to be WorkerThreadModifyBusyCount.
Attachment #8459855 - Flags: feedback?(khuey) → feedback-
(Assignee)

Comment 15

5 years ago
Created attachment 8466486 [details] [diff] [review]
Patch 1 -  ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()
Attachment #8459855 - Attachment is obsolete: true
Attachment #8459857 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 8466489 [details] [diff] [review]
Patch 2 - Add postMessage to service workers.
Attachment #8459874 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 8466490 [details] [diff] [review]
Patch 3 - Implement client.postMessage and add tests for getServiced().
Attachment #8459981 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8466486 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #8466490 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #8466486 - Flags: review?(bent.mozilla) → review?(amarchesini)
(Assignee)

Updated

5 years ago
Attachment #8466490 - Flags: review?(bent.mozilla) → review?(amarchesini)
(Assignee)

Updated

5 years ago
Attachment #8466489 - Flags: review?(amarchesini)
Comment on attachment 8466486 [details] [diff] [review]
Patch 1 -  ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()

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

::: dom/bindings/Bindings.conf
@@ +238,5 @@
>      'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate',
>  },
>  
> +'Client': {
> +    'workers': True,

This is not needed.

@@ +1073,5 @@
>      'headerFile': 'mozilla/dom/ServiceWorkerContainer.h',
>  },
>  
> +'ServiceWorkerClients': {
> +    'workers': True,

Remove this.

::: dom/webidl/Client.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html
> + *
> + */
> +

spec is changed. This is called ServiceWorkerClient.

Use: [Exposed=ServiceWorker] to expose the object to worker.

::: dom/webidl/ServiceWorkerClients.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html
> + *
> + */
> +

[Exposed=ServiceWorker]

::: dom/workers/Client.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_workers_client_h
> +#define mozilla_dom_workers_client_h
> +
> +#include "mozilla/dom/ClientBinding.h"

Remove this.

@@ +9,5 @@
> +
> +#include "mozilla/dom/ClientBinding.h"
> +
> +#include "nsISupportsImpl.h"
> +#include "nsIGlobalObject.h"

do you need this?

@@ +40,5 @@
> +  }
> +
> +  void PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
> +                   const Optional<Sequence<JS::Value>>& aTransferable)
> +  {

This comes with the other patches, I guess.

@@ +49,5 @@
> +    return mOwner;
> +  }
> +
> +  JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE
> +  {

Move this to the .cpp file.

@@ +58,5 @@
> +  ~Client()
> +  {
> +  }
> +
> +  nsISupports* mOwner;

nsCOMPtr<nsISupports> mOwner;

@@ +66,5 @@
> +} // namespace workers
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif

// mozilla_dom_workers_client_h

::: dom/workers/ServiceWorkerClients.cpp
@@ +7,5 @@
> +#include "ServiceWorkerManager.h"
> +#include "WorkerScope.h"
> +#include "Client.h"
> +
> +#include "nsDOMString.h"

DO you need this?

@@ +8,5 @@
> +#include "WorkerScope.h"
> +#include "Client.h"
> +
> +#include "nsDOMString.h"
> +#include "nsPIDOMWindow.h"

this is not needed and probably RuntimeService.h can be removed as well.

@@ +20,5 @@
> +using namespace mozilla::dom::workers;
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(ServiceWorkerClients)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(ServiceWorkerClients)
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(ServiceWorkerClients)

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ServiceWorkerClients, mWorkerScope)

@@ +35,5 @@
> +}
> +
> +// Helper class used for passing the promise between threads while
> +// keeping the worker alive.
> +class PromiseHolder : public WorkerFeature

1. MOZ_FINAL
2. put this class in an anonymous namespace.

But most important, probably what you really want is just PromiseWorkerProxy. Take a look at ./dom/promise/PromiseWorkerProxy.h

@@ +37,5 @@
> +// Helper class used for passing the promise between threads while
> +// keeping the worker alive.
> +class PromiseHolder : public WorkerFeature
> +{
> +  friend class GetServicedRunnable;

Why this?

@@ +42,5 @@
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PromiseHolder)
> +
> +public:
> +  explicit PromiseHolder(WorkerPrivate* aWorkerPrivate,

explicit is not needed if you have 2 params.

@@ +97,5 @@
> +
> +  bool mClean;
> +};
> +
> +class ResolvePromiseWorkerRunnable : public WorkerRunnable

anonymous namespace + MOZ_FINAL

@@ +104,5 @@
> +  nsAutoPtr<nsTArray<uint64_t> > mValue;
> +public:
> +  ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
> +                               PromiseHolder* aPromiseHolder,
> +                               nsAutoPtr<nsTArray<uint64_t> > aValue)

>>

@@ +116,5 @@
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    aWorkerPrivate->AssertIsOnWorkerThread();
> +
> +    nsTArray<nsRefPtr<Client> > ret;

>>

@@ +117,5 @@
> +    MOZ_ASSERT(aWorkerPrivate);
> +    aWorkerPrivate->AssertIsOnWorkerThread();
> +
> +    nsTArray<nsRefPtr<Client> > ret;
> +    for (size_t i = 0; i < mValue.get()->Length(); i++) {

is get() needed?

@@ +162,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +already_AddRefed<Promise> ServiceWorkerClients::GetServiced()

I guess you should mark this method as [Throws] in the the WebIDL file. Then use the ErrorResult object to propagate errors.

@@ +167,5 @@
> +{
> +  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +  MOZ_ASSERT(workerPrivate);
> +
> +  DOMString scope;

nsString scope;

@@ +175,5 @@
> +
> +  ErrorResult rv;
> +  nsRefPtr<Promise> promise = Promise::Create(global, rv);
> +  if (rv.Failed()) {
> +    return nullptr;

Once you have the ErrorResult object, this will be:
nsRefPtr<Promise> promise = Promise::Create(global, aRv);
if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

@@ +179,5 @@
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<GetServicedRunnable> r =
> +    new GetServicedRunnable(workerPrivate, promise, NS_ConvertUTF16toUTF8(scope.AsAString()));

NS_ConvertUTF16toUTF8(scope)

@@ +180,5 @@
> +  }
> +
> +  nsRefPtr<GetServicedRunnable> r =
> +    new GetServicedRunnable(workerPrivate, promise, NS_ConvertUTF16toUTF8(scope.AsAString()));
> +  rv = NS_DispatchToMainThread(r);

nsresult rv = NS_DispatchToMainThread(r);
if (NS_WARN_IF(NS_FAILED(rv)) { ...

@@ +190,5 @@
> +  return promise.forget();
> +}
> +
> +// FIXME(catalinb): Bug 1045257 - Implement ReloadAll
> +already_AddRefed<Promise> ServiceWorkerClients::ReloadAll()

Also this method should have a ErrorResult param. Add [Throws] in the WebIDL.

::: dom/workers/ServiceWorkerClients.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_workers_serviceworkerclients_h
> +#define mozilla_dom_workers_serviceworkerclients_h
> +
> +#include "mozilla/dom/ServiceWorkerClientsBinding.h"

remove this

@@ +9,5 @@
> +
> +#include "mozilla/dom/ServiceWorkerClientsBinding.h"
> +
> +#include "nsISupportsImpl.h"
> +#include "nsIGlobalObject.h"

do you need this?

@@ +22,5 @@
> +
> +class ServiceWorkerGlobalScope;
> +
> +class ServiceWorkerClients MOZ_FINAL : public nsISupports,
> +                                       public nsWrapperCache {

{ in a new line.

@@ +34,5 @@
> +  already_AddRefed<Promise> ReloadAll();
> +
> +  JSObject* WrapObject(JSContext* cx)
> +  {
> +    return ServiceWorkerClientsBinding_workers::Wrap(cx, this);

in the cpp file.

@@ +36,5 @@
> +  JSObject* WrapObject(JSContext* cx)
> +  {
> +    return ServiceWorkerClientsBinding_workers::Wrap(cx, this);
> +  }
> +  nsISupports* GetParentObject() const;

new line before nsISupports.

Change this in:

ServiceWorkerGlobalScope* GetParentObject() const
{
  return mWorkerScope;
}

@@ +43,5 @@
> +  ~ServiceWorkerClients()
> +  {
> +  }
> +
> +  ServiceWorkerGlobalScope* mWorkerScope;

nsRefPtr

::: dom/workers/ServiceWorkerManager.cpp
@@ +1410,5 @@
>  
>    aScope = NS_ConvertUTF8toUTF16(r->mScope);
>    return NS_OK;
>  }
>  

all of this goes into an anonymous namespace.

::: dom/workers/ServiceWorkerManager.h
@@ +315,5 @@
>    static already_AddRefed<ServiceWorkerManager>
>    GetInstance();
>  
> +  NS_IMETHODIMP
> +  GetServicedClients(const nsCString& aScope,

You don't need this. Just change nsIServiceWorkerManager.idl
Attachment #8466486 - Flags: review?(amarchesini) → review-
I'll wait for a new version of patch 1 before reviewing patch 2 and 3. Is it ok?
(Assignee)

Comment 20

5 years ago
(In reply to Andrea Marchesini (:baku) from comment #19)
> I'll wait for a new version of patch 1 before reviewing patch 2 and 3. Is it
> ok?

Yes, I'll also update patches 2 and 3 based on your feedback for the first patch. I have another version for patch 1 (as per your comments), but I want to go through the code one more time before uploading.

I have to work on my intern presentation right now. :D
(Assignee)

Comment 21

5 years ago
Created attachment 8471210 [details] [diff] [review]
Patch 1(v2) -  ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()
Attachment #8466486 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 8471249 [details] [diff] [review]
Patch 1(v2) -  ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()
Attachment #8471210 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Comment on attachment 8471249 [details] [diff] [review]
Patch 1(v2) -  ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()

I have addressed the issues you mentioned, with a few exceptions:

(In reply to Andrea Marchesini (:baku) from comment #18)
> @@ +35,5 @@
> > +}
> > +
> > +// Helper class used for passing the promise between threads while
> > +// keeping the worker alive.
> > +class PromiseHolder : public WorkerFeature
> 
> But most important, probably what you really want is just
> PromiseWorkerProxy. Take a look at ./dom/promise/PromiseWorkerProxy.h

I've already looked at PromiseWorkerProxy. The reason I can't use it is because it requires creating a second promise on the main thread, which is not possible since there is no available global object on the main thread for sw.

> @@ +167,5 @@
> > +{
> > +  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> > +  MOZ_ASSERT(workerPrivate);
> > +
> > +  DOMString scope;
> 
> nsString scope;

The getter for the scope requires a DOMString and the compiler complains.

> ::: dom/workers/ServiceWorkerManager.h
> @@ +315,5 @@
> >    static already_AddRefed<ServiceWorkerManager>
> >    GetInstance();
> >  
> > +  NS_IMETHODIMP
> > +  GetServicedClients(const nsCString& aScope,
> 
> You don't need this. Just change nsIServiceWorkerManager.idl

I removed the NS_IMETHODIMP return type since it wasn't really being used. I've kept the declaration in the .h for the following reasons:
1. The method is only used from c++ code and I'm using the singleton instance getter for ServiceWorkerManager which returns the exact SWM type.
2. Converting the arguments to IDL types will add unnecessary conversions
(or maybe I missing something): there is no IDL equivalent for nsTArray<uint64_t>.

Also, in ServiceWorkerClients, because the reference to the parent object is now a strong reference, I had to add cycle collecting info for ServiceWorkerGlobalScope.
Attachment #8471249 - Flags: review?(amarchesini)
(Assignee)

Comment 24

5 years ago
Created attachment 8471259 [details] [diff] [review]
Patch 2(v2) - Add postMessage to service workers.
Attachment #8466489 - Attachment is obsolete: true
Attachment #8466489 - Flags: review?(amarchesini)
Attachment #8471259 - Flags: review?(amarchesini)
(Assignee)

Comment 25

5 years ago
Created attachment 8471261 [details] [diff] [review]
Patch 3(v2) - Implement client.postMessage and add tests for getServiced().
Attachment #8466490 - Attachment is obsolete: true
Attachment #8466490 - Flags: review?(amarchesini)
Attachment #8471261 - Flags: review?(amarchesini)
Comment on attachment 8471249 [details] [diff] [review]
Patch 1(v2) -  ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced()

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

Good. Can I see it again with this comments fixed? Thanks.

::: dom/webidl/ServiceWorkerClient.webidl
@@ +9,5 @@
> + */
> +
> +[Exposed=ServiceWorker]
> +interface ServiceWorkerClient {
> +  readonly attribute unsigned long id;

no postMessage? I guess it's in another patch.

::: dom/workers/ServiceWorkerClient.cpp
@@ +20,5 @@
> +  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +JSObject* ServiceWorkerClient::WrapObject(JSContext* cx)

JSObject*
Ser... aCx)

::: dom/workers/ServiceWorkerClient.h
@@ +40,5 @@
> +  {
> +    return mOwner;
> +  }
> +
> +  JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE;

aCx

::: dom/workers/ServiceWorkerClients.cpp
@@ +13,5 @@
> +
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/dom/ServiceWorkerClientsBinding.h"
> +
> +using namespace mozilla;

I guess this is just for ErrorResult, so just do:

using mozilla::ErrorResult;

@@ +33,5 @@
> +  MOZ_ASSERT(mWorkerScope);
> +  SetIsDOMBinding();
> +}
> +
> +JSObject* ServiceWorkerClients::WrapObject(JSContext* cx)

JSObject*
ServiceWorkerClients::WrapObject(JSContext* aCx)

@@ +111,5 @@
> +
> +public:
> +  ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
> +                               PromiseHolder* aPromiseHolder,
> +                               nsAutoPtr<nsTArray<uint64_t>> aValue)

nsAutoPtr<...>&

@@ +172,5 @@
> +};
> +
> +} // anonymous namespace
> +
> +already_AddRefed<Promise> ServiceWorkerClients::GetServiced(ErrorResult& aRv)

returnType
Class::Method()

everywhere in any CPP file.

@@ +175,5 @@
> +
> +already_AddRefed<Promise> ServiceWorkerClients::GetServiced(ErrorResult& aRv)
> +{
> +  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +  MOZ_ASSERT(workerPrivate);

Add mWorkerPrivate->AssertIsOnWorkerThread();

@@ +184,5 @@
> +  nsRefPtr<Promise> promise = Promise::Create(mWorkerScope, aRv);
> +  if (NS_WARN_IF(aRv.Failed())) {
> +    return nullptr;
> +  }
> +

you want to add the feature at this point because the worker can be CC/GCed before the runnable is executed.
Create the PromiseHolder obj here.

::: dom/workers/ServiceWorkerClients.h
@@ +33,5 @@
> +
> +  already_AddRefed<Promise> GetServiced(ErrorResult& aRv);
> +  already_AddRefed<Promise> ReloadAll(ErrorResult& aRv);
> +
> +  JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE;

aCx

::: dom/workers/ServiceWorkerManager.cpp
@@ +1433,5 @@
> +                        ServiceWorkerRegistration* aRegistration,
> +                        void* aData)
> +{
> +  FilterRegistrationData* data = static_cast<FilterRegistrationData*>(aData);
> +  if (data->mRegistration == aRegistration) {

if (data->mRegistration != aRegistration) {
  return PL_DHASH_NEXT;
}

@@ +1435,5 @@
> +{
> +  FilterRegistrationData* data = static_cast<FilterRegistrationData*>(aData);
> +  if (data->mRegistration == aRegistration) {
> +    nsCOMPtr<nsIDocument> document = do_QueryInterface(aKey);
> +    if (!document->GetInnerWindow()) {

document can be null.

@@ +1447,5 @@
> +
> +} // anonymous namespace
> +
> +void
> +ServiceWorkerManager::GetServicedClients(const nsCString& aScope, nsTArray<uint64_t>* aControlledDocuments)

indent in a new line the second param.

::: dom/workers/WorkerScope.h
@@ +197,5 @@
>    {
>      // FIXME(nsm): Bug 982728
>    }
>  
> +  already_AddRefed<ServiceWorkerClients>

ServiceWorkerClients*

@@ +201,5 @@
> +  already_AddRefed<ServiceWorkerClients>
> +  Clients() {
> +    if (!mClients) {
> +      mClients = new ServiceWorkerClients(this);
> +      MOZ_ASSERT(mClients);

remove this assert. "new" doesn't fail.

@@ +203,5 @@
> +    if (!mClients) {
> +      mClients = new ServiceWorkerClients(this);
> +      MOZ_ASSERT(mClients);
> +    }
> +

return mClients;
Attachment #8471249 - Flags: review?(amarchesini) → review+
Comment on attachment 8471259 [details] [diff] [review]
Patch 2(v2) - Add postMessage to service workers.

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

I think this patch should change ServiceWorkerClient and not ServiceWorker.
This is what the spec says.

::: dom/webidl/ServiceWorker.webidl
@@ +21,5 @@
>    readonly attribute ServiceWorkerState state;
>    attribute EventHandler onstatechange;
> +
> +  [Throws]
> +  void postMessage(any message, optional sequence<Transferable> transferable);

This goes in ServiceWorkerClient... right?
Attachment #8471259 - Flags: review?(amarchesini)
Flags: needinfo?(cbadea)
Comment on attachment 8471261 [details] [diff] [review]
Patch 3(v2) - Implement client.postMessage and add tests for getServiced().

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

I would like to see a test for:

1. postMessage with DOMFile/DOMBlob, and maybe add also normal object, booleans, strings, arrays.
2. postMessage with transferable objects. ArrayBuffers are transferable and check that, after the postMessage, the arrayBuffer is empty from 1 side, and fully populated on the other side.

A separated patch for this is ok.

Smaug, can you take a look at how the MessageEvent is dispatched?

::: dom/workers/ServiceWorkerClient.cpp
@@ +34,5 @@
>  }
>  
> +namespace {
> +
> +class ServiceWorkerPostMessageRunnable MOZ_FINAL : public nsRunnable

ServiceWorkerClientPostMessageRunnable?

@@ +38,5 @@
> +class ServiceWorkerPostMessageRunnable MOZ_FINAL : public nsRunnable
> +{
> +  uint64_t mId;
> +  JSAutoStructuredCloneBuffer mBuffer;
> +  nsTArray<nsCOMPtr<nsISupports> > mClonedObjects;

>>

@@ +43,5 @@
> +
> +public:
> +  ServiceWorkerPostMessageRunnable(uint64_t aId,
> +                                   JSAutoStructuredCloneBuffer&& aData,
> +                                   nsTArray<nsCOMPtr<nsISupports> >& aClonedObjects)

>>

@@ +49,5 @@
> +      mBuffer(Move(aData))
> +  {
> +    mClonedObjects.SwapElements(aClonedObjects);
> +  }
> +

DispatchDOMEvent should be private.

@@ +51,5 @@
> +    mClonedObjects.SwapElements(aClonedObjects);
> +  }
> +
> +  bool DispatchDOMEvent(JSContext* aCx, nsGlobalWindow* aTargetWindow)
> +  {

AssertIsOnMainThread();

@@ +59,5 @@
> +    clonedObjects.SwapElements(mClonedObjects);
> +
> +    JS::Rooted<JS::Value> messageData(aCx);
> +    if (!mBuffer.read(aCx, &messageData,
> +                      WorkerStructuredCloneCallbacks(false))) {

why false? You want to dispatch events to the main-thread so this must be true.
Add a mochitest where you pass a DOMBlob object. You should test also the transferable obj list.

@@ +77,5 @@
> +    if (NS_FAILED(rv)) {
> +      xpc::Throw(aCx, rv);
> +      return false;
> +    }
> +

I think you can just do this:

event->SetTrusted(true);
bool status = false;
aTargetWindow->DispatchEvent(event, &status);
if (!status) { ...

I'll ask smaug a feedback about this.

@@ +112,5 @@
> +};
> +
> +} // anonymous namespace
> +
> +void ServiceWorkerClient::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,

void
ServiceWorkerClient:...
Attachment #8471261 - Flags: review?(bugs)
Attachment #8471261 - Flags: review?(amarchesini)
Attachment #8471261 - Flags: review+
Comment on attachment 8471261 [details] [diff] [review]
Patch 3(v2) - Implement client.postMessage and add tests for getServiced().

>+    nsRefPtr<MessageEvent> event = new MessageEvent(aTargetWindow, nullptr, nullptr);
>+    nsresult rv =
>+      event->InitMessageEvent(NS_LITERAL_STRING("message"),
>+                              false /* non-bubbling */,
>+                              true /* cancelable */,
Is this really supposed to be cancelable? Yet you don't do anything if preventDefault() is actually called

>+    event->SetTrusted(true);
>+    WidgetEvent* internalEvent = event->GetInternalNSEvent();
>+
>+    nsEventStatus status = nsEventStatus_eIgnore;
>+    EventDispatcher::Dispatch(static_cast<nsPIDOMWindow*>(aTargetWindow),
>+                              presContext,
>+                              internalEvent,
>+                              static_cast<dom::Event*>(event.get()),
>+                              &status);
Just do
bool dummy;
aTargetWindow->DispatchEvent(event, &dummy);
Attachment #8471261 - Flags: review?(bugs) → review+
(Assignee)

Comment 30

5 years ago
(In reply to Andrea Marchesini (:baku) from comment #27)
> Comment on attachment 8471259 [details] [diff] [review]
> Patch 2(v2) - Add postMessage to service workers.
> 
> Review of attachment 8471259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this patch should change ServiceWorkerClient and not ServiceWorker.
> This is what the spec says.
> 
> ::: dom/webidl/ServiceWorker.webidl
> @@ +21,5 @@
> >    readonly attribute ServiceWorkerState state;
> >    attribute EventHandler onstatechange;
> > +
> > +  [Throws]
> > +  void postMessage(any message, optional sequence<Transferable> transferable);
> 
> This goes in ServiceWorkerClient... right?

No.
The spec specifies that ServiceWorker inherits from Worker which has the postMessage method. With our current implementation, SW inherits from EventTarget because the inheritance was an open issue [1]. I need serviceWorker.postMessage for running the tests, so I decided to add the definition to the SW interface.

Now that the issue has been decided, we should switch to inheriting from Worker, I guess.
Either way, the implementation should be the same, only the webidl that's exposing postMessage will change.
I will open a separate bug for it, but do you think we can land this and fix the webidl after?

[1] https://github.com/slightlyoff/ServiceWorker/issues/189
Flags: needinfo?(cbadea)
Comment on attachment 8471259 [details] [diff] [review]
Patch 2(v2) - Add postMessage to service workers.

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

Ok. Thanks for your comment. File a bug please. r=me

::: dom/webidl/ServiceWorker.webidl
@@ +21,5 @@
>    readonly attribute ServiceWorkerState state;
>    attribute EventHandler onstatechange;
> +
> +  [Throws]
> +  void postMessage(any message, optional sequence<Transferable> transferable);

File a follow up and write here the bug ID.
Attachment #8471259 - Flags: review+
(Assignee)

Comment 32

5 years ago
Created attachment 8473539 [details] [diff] [review]
Patch 1(v3) - Service Worker: ServiceWorkerClients and Client interfaces with getServiced()
Attachment #8471249 - Attachment is obsolete: true
(Assignee)

Comment 33

5 years ago
Comment on attachment 8473539 [details] [diff] [review]
Patch 1(v3) - Service Worker: ServiceWorkerClients and Client interfaces with getServiced()

Updated with the requested changes, with some minor comments:

(In reply to Andrea Marchesini (:baku) from comment #26)

> ::: dom/workers/ServiceWorkerClients.cpp
> @@ +13,5 @@
> > +
> > +#include "mozilla/dom/Promise.h"
> > +#include "mozilla/dom/ServiceWorkerClientsBinding.h"
> > +
> > +using namespace mozilla;
> 
> I guess this is just for ErrorResult, so just do:
> 
> using mozilla::ErrorResult;
It's also needed for the js (AutoJSContext) context used in dispatching the runnable back to the worker thread.

> @@ +184,5 @@
> > +  nsRefPtr<Promise> promise = Promise::Create(mWorkerScope, aRv);
> > +  if (NS_WARN_IF(aRv.Failed())) {
> > +    return nullptr;
> > +  }
> > +
> 
> you want to add the feature at this point because the worker can be CC/GCed
> before the runnable is executed.
> Create the PromiseHolder obj here.
Well, the PromiseHolder is created in the constructor of GetServicedRunnable, which is called right after the if statement. So, moving the PromiseHolder out won't make any difference. I personally prefer the current code, because it's similar to PromiseWorkerProxy.
Attachment #8473539 - Flags: review?(amarchesini)
(Assignee)

Comment 34

5 years ago
Created attachment 8473545 [details] [diff] [review]
Patch 2(v3) - Add postMessage to service workers.
Attachment #8471259 - Attachment is obsolete: true
(Assignee)

Comment 35

5 years ago
Comment on attachment 8473545 [details] [diff] [review]
Patch 2(v3) - Add postMessage to service workers.

Added comment for follow-up.
Attachment #8473545 - Flags: review?(amarchesini)
(Assignee)

Comment 36

5 years ago
Created attachment 8473547 [details] [diff] [review]
Patch 3(v3) - Implement client.postMessage and add tests for getServiced().
Attachment #8471261 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
Comment on attachment 8473547 [details] [diff] [review]
Patch 3(v3) - Implement client.postMessage and add tests for getServiced().

Made all the requested changes.

(In reply to Olli Pettay [:smaug] from comment #29)
> Comment on attachment 8471261 [details] [diff] [review]
> Patch 3(v2) - Implement client.postMessage and add tests for getServiced().
> 
> >+    nsRefPtr<MessageEvent> event = new MessageEvent(aTargetWindow, nullptr, nullptr);
> >+    nsresult rv =
> >+      event->InitMessageEvent(NS_LITERAL_STRING("message"),
> >+                              false /* non-bubbling */,
> >+                              true /* cancelable */,
> Is this really supposed to be cancelable? Yet you don't do anything if
> preventDefault() is actually called
I checked the spec, and it shouldn't be cancelable - just changed the flag.
Attachment #8473547 - Flags: review?(amarchesini)
(Assignee)

Comment 38

5 years ago
Created attachment 8473548 [details] [diff] [review]
Patch 4 - Extensive testing of postMessage.
(Assignee)

Comment 39

5 years ago
Comment on attachment 8473548 [details] [diff] [review]
Patch 4 - Extensive testing of postMessage.

WIP. I only wrote a test for basic types + blob and file.
Attachment #8473539 - Flags: review?(amarchesini) → review+
Attachment #8473545 - Flags: review?(amarchesini) → review+
Comment on attachment 8473547 [details] [diff] [review]
Patch 3(v3) - Implement client.postMessage and add tests for getServiced().

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

::: dom/workers/ServiceWorkerClient.cpp
@@ +114,5 @@
> +} // anonymous namespace
> +
> +void
> +ServiceWorkerClient::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
> +                         const Optional<Sequence<JS::Value>>& aTransferable,

indentation

::: dom/workers/test/serviceworkers/get_serviced_worker_enumerate.js
@@ +1,4 @@
> +onmessage = function() {
> +  self.clients.getServiced().then(function(result) {
> +    dump("result: " + result.length + "\n\n");
> +    dump("catalinn\n\n");

mm... remove these messages.

::: dom/workers/test/serviceworkers/message_posting_worker.js
@@ +1,4 @@
> +onmessage = function(e) {
> +  self.clients.getServiced().then(function(res) {
> +    if (!res.length) {
> +      dump("ERROR: no clients are currently controlled.\n");

We should report this error properly.
Attachment #8473547 - Flags: review?(amarchesini) → review+
Created attachment 8478030 [details] [diff] [review]
Patch 1 - interdiff from v3

Handle AddFeature failure gracefully.
Add call to getServiced() in onclose.
More thread assertions.
Attachment #8478030 - Attachment is patch: true
Attachment #8478030 - Attachment mime type: application/octet-stream → text/plain
Attachment #8478030 - Flags: review?(amarchesini)
Created attachment 8478118 [details] [diff] [review]
interdiff from Patch 3(v3)

Changes to test. I'm not happy with not using ready. Maybe we should wait for that.
Comment on attachment 8478030 [details] [diff] [review]
Patch 1 - interdiff from v3

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

lgtm

::: dom/workers/ServiceWorkerManager.cpp
@@ +1981,5 @@
>  class MOZ_STACK_CLASS FilterRegistrationData
>  {
>  public:
>    FilterRegistrationData(nsTArray<uint64_t>* aDocuments,
> +                     ServiceWorkerRegistrationInfo* aRegistration)

indentation
Attachment #8478030 - Flags: review?(amarchesini) → review+
Attachment #8478118 - Attachment is patch: true
Attachment #8478118 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 8478118 [details] [diff] [review]
interdiff from Patch 3(v3)

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

::: dom/workers/test/serviceworkers/test_get_serviced_advanced.html
@@ +46,5 @@
>        }
>      });
> +    var w;
> +    setTimeout(function() {
> +      w = window.open("sw_clients/service_worker_controlled.html");

this should be an iframe but it's ok for this patch.
Attachment #8478118 - Flags: review?(amarchesini) → review+
Created attachment 8478643 [details]
interdiff from Patch 4(v3)

The earlier patch passed a nsTArray where the WorkerStructuredCloneCallbacks expect a WorkerStructuredCloneClosure, obviously leading to all sorts of bad things.
Attachment #8478643 - Attachment is patch: true
Attachment #8478643 - Attachment mime type: application/octet-stream → text/plain
Attachment #8478643 - Flags: review?(amarchesini)
Comment on attachment 8478643 [details]
interdiff from Patch 4(v3)

Review of attachment 8478643 [details]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerClient.cpp
@@ +54,5 @@
> +    bool rv = mBuffer.write(aCx, aValue, aTransferredValue, aCallbacks,
> +                            &mClosure);
> +    // This hashtable has to be empty because it could contain MessagePort
> +    // objects that cannot be freed on a different thread.
> +    mClosure.mTransferredPorts.Clear();

This is not required in this runnable, will get rid of it.
Comment on attachment 8478643 [details]
interdiff from Patch 4(v3)

WorkerStructuredCloneClosure is from MessagePort :(
Attachment #8478643 - Attachment is obsolete: true
Attachment #8478643 - Attachment is patch: false
Attachment #8478643 - Flags: review?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #18)
> Comment on attachment 8466486 [details] [diff] [review]
> Patch 1 -  ServiceWorkers: ServiceWorkerClients and Client interfaces with
> getServiced()
> 
> Review of attachment 8466486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ +238,5 @@
> >      'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate',
> >  },
> >  
> > +'Client': {
> > +    'workers': True,
> 
> This is not needed.
> 
> @@ +1073,5 @@
> >      'headerFile': 'mozilla/dom/ServiceWorkerContainer.h',
> >  },
> >  
> > +'ServiceWorkerClients': {
> > +    'workers': True,
> 
> Remove this.
> 
> ::: dom/webidl/Client.webidl
> @@ +6,5 @@
> > + * The origin of this IDL file is
> > + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html
> > + *
> > + */
> > +
> 
> spec is changed. This is called ServiceWorkerClient.
> 
> Use: [Exposed=ServiceWorker] to expose the object to worker.

Andrea, it seems Exposed=ServiceWorker doesn't yet do what we want it to do. test_interfaces.html keeps complaining. Even if I try just Exposed=Worker. I think we don't want to generate main thread bindings at all for now.

What this bug needs is two interfaces ServiceWorkerClient(s) that are solely exposed on ServiceWorkerGlobalScope. AFAIK there is no way to accomplish that right now except using bindings.conf. Boris, is that right?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 54

5 years ago
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #53)
> https://tbpl.mozilla.org/?tree=Try&rev=2a757b8cae02

Nikhil, thank you for taking over. Unfortunately, I won't be able to continue the work on SW until the beginning of September - I'm on a tight schedule with a school project. I'll ping you once I'm done with it.

Assigning the bug to you in the meantime.
Assignee: catalin.badea392 → nsm.nikhil
> AFAIK there is no way to accomplish that right now except using bindings.conf. 

Other way around.  You can't restrict to only ServiceWorkerGlobalScope (but not other worker scopes) using Bindings.conf, since Bindings.conf has no concept of different worker scopes.

What you want to do for now, until bug 1048699 is fixed, is something like [Exposed=Workers,Func=something] that will enforce visibility only in service workers.  Perhaps restoring the ServiceWorkerGlobalScope::Visible function that attachment 8464540 [details] [diff] [review] removed?

Or better yet, just fix bug 1048699.  ;)
Flags: needinfo?(bzbarsky)
Blocks: 982728
I had to back these out for apparently breaking mochitest-4 on various platforms:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d323ede35f89

https://tbpl.mozilla.org/php/getParsedLog.php?id=47265533&tree=Mozilla-Inbound
Flags: needinfo?(nsm.nikhil)
It's very likely the failures were intermittents as both are known bugs. Andrea landed a fix for Bug 1060517. This patch is completely unrelated to the failures. Trying this again.
Flags: needinfo?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #61)
> It's very likely the failures were intermittents as both are known bugs.
> Andrea landed a fix for Bug 1060517. This patch is completely unrelated to
> the failures. Trying this again.

More like perma-fails hitting consistently across all platforms. And guess what, it still is. Backed out again. Please verify that this is green on Try before burning the tree a third time.
https://hg.mozilla.org/integration/mozilla-inbound/rev/185126a2a4e9

https://tbpl.mozilla.org/php/getParsedLog.php?id=47362920&tree=Mozilla-Inbound
It seems like the test_scopes.html test for the toplevel scope leads to some ordering issue that causes failures. I've done a new try push with that specific test disabled (not the entire test_scopes.html). If this goes green, I'll land these patches. It is quite possible that the ordering changes will be fixed by bug 1066822. In either case I'll file a followup to enable that test when landing this.
Flags: needinfo?(amarchesini)
Pushed to maple for now. Disabling that test did not work, seems like something more insidious. I'm going to work on bug 1066822 and see if that helps.

Comment 78

5 years ago
This should help fix the b2g build bustages that resulted from the patches in comment 76.

https://hg.mozilla.org/projects/maple/rev/a040c47e5db9
Blocks: 1065216
(Assignee)

Comment 79

4 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5f35227e58e

Besides the build failing on Windows, it looks fine.
(Assignee)

Comment 80

4 years ago
Created attachment 8500163 [details] [diff] [review]
Patch 1.1(v4) - Service Worker: ServiceWorkerClients and Client interfaces with getServiced().
Attachment #8473539 - Attachment is obsolete: true
Attachment #8478030 - Attachment is obsolete: true
(Assignee)

Comment 81

4 years ago
Created attachment 8500164 [details] [diff] [review]
Part 1.2: Break the cyclic dependency between WorkerScope.h and ServiceWorkerClients.h, and introduce a ToSupports overload for WorkerGlobalScope in order to fix the b2g builds
(Assignee)

Comment 82

4 years ago
Created attachment 8500165 [details] [diff] [review]
Patch 2 - Add postMessage to service worker.

Rebased and fixed windows build bustage.
Attachment #8473545 - Attachment is obsolete: true
(Assignee)

Comment 83

4 years ago
Created attachment 8500166 [details] [diff] [review]
Patch 3(v4) - Implement client.postMessage and add tests for getServiced().
Attachment #8473547 - Attachment is obsolete: true
Attachment #8478118 - Attachment is obsolete: true
(Assignee)

Comment 84

4 years ago
Created attachment 8500167 [details] [diff] [review]
Patch 4(v2) - Extensive testing of postMessage.
Attachment #8473548 - Attachment is obsolete: true
Assignee: nsm.nikhil → catalin.badea392
(Assignee)

Comment 86

4 years ago
Created attachment 8512138 [details] [diff] [review]
Patch 1.1 - Service Worker: ServiceWorkerClients and Client interfaces with getServiced().
Attachment #8500163 - Attachment is obsolete: true
(Assignee)

Comment 87

4 years ago
Created attachment 8512139 [details] [diff] [review]
Part 1.2: Break the cyclic dependency between WorkerScope.h and ServiceWorkerClients.h, and introduce a ToSupports overload for WorkerGlobalScope in order to fix the b2g builds
Attachment #8500164 - Attachment is obsolete: true
(Assignee)

Comment 88

4 years ago
Created attachment 8512141 [details] [diff] [review]
Part 1.3: Fix crash caused by a race in PromiseHolder.
(Assignee)

Comment 89

4 years ago
Comment on attachment 8512141 [details] [diff] [review]
Part 1.3: Fix crash caused by a race in PromiseHolder.

I think we can try to land 1.1, 1.2 and 1.3

Here are the try runs:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=55a9b2deec3e
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b570fa5d25a2
Attachment #8512141 - Flags: review?(nsm.nikhil)
Comment on attachment 8512141 [details] [diff] [review]
Part 1.3: Fix crash caused by a race in PromiseHolder.

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

Looks good. Please land all relevant patches on m-c, and this fix on maple.

::: dom/promise/Promise.cpp
@@ +1375,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    MutexAutoLock lock(mCleanUpLock);
> +

This file doesn't need to change.

::: dom/workers/ServiceWorkerClients.cpp
@@ +53,5 @@
>    PromiseHolder(WorkerPrivate* aWorkerPrivate,
>                  Promise* aPromise)
>      : mWorkerPrivate(aWorkerPrivate),
>        mPromise(aPromise),
> +      mCleanUpLock("cleanUpLock"),

name this "PromiseHolder cleanUpLock" or similar.

@@ +219,5 @@
>    }
>  
> +  nsRefPtr<PromiseHolder> promiseHolder = new PromiseHolder(workerPrivate,
> +                                                            promise);
> +  if (!promiseHolder->Get()) {

Please rename Get() to GetPromise().
Attachment #8512141 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 91

4 years ago
Created attachment 8512252 [details] [diff] [review]
Patch 1.3 - Fix crash caused by a race condition in PromiseHolder.
Attachment #8512141 - Attachment is obsolete: true
(Assignee)

Comment 92

4 years ago
Created attachment 8512256 [details] [diff] [review]
Patch 1.3 - Fix crash caused by a race condition in PromiseHolder.
Attachment #8512252 - Attachment is obsolete: true
(Assignee)

Comment 93

4 years ago
Please land patches 1.1, 1.2 and 1.3. The patches have all been previously r+-ed and contain only small changes or have been rebased.
Keywords: checkin-needed
sorry had to back this out since i guess this caused https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3350702&repo=mozilla-inbound
Flags: needinfo?(catalin.badea392)
(Assignee)

Comment 96

4 years ago
Created attachment 8512818 [details] [diff] [review]
Patch 1.3 - Fix crash caused by a race condition in PromiseHolder and remove onclose from test.
Attachment #8512256 - Attachment is obsolete: true
(Assignee)

Comment 97

4 years ago
(In reply to Carsten Book [:Tomcat] from comment #95)
> sorry had to back this out since i guess this caused
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3350702&repo=mozilla-inbound

Yes, this was causing the failures.

I want to give it another go. I've updated patch 1.3 to remove a deprecated feature from the test which I think was the reason for the assert.

Try results (I've queued 2 full runs because the assert happens intermittently):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2674a2ae79a0
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ab05c20ba1d9

Please land 1.1, 1.2 and 1.3 again.
Flags: needinfo?(catalin.badea392)
Keywords: checkin-needed
(Assignee)

Comment 98

4 years ago
Actually, lets wait on this. I believe there is still a race condition and its just less likely to occur.
Keywords: checkin-needed
Depends on: 1090994
(Assignee)

Comment 99

4 years ago
Created attachment 8513793 [details] [diff] [review]
Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder.
Attachment #8513793 - Flags: review?(nsm.nikhil)
Comment on attachment 8513793 [details] [diff] [review]
Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder.

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

This looks good to me.
Andrea, could you take a look at this patch and at patch 1.3.

::: dom/workers/ServiceWorkerClients.cpp
@@ +232,5 @@
> +    // Use a control runnable to release the runnable on the worker thread.
> +    nsRefPtr<ReleasePromiseRunnable> releaseRunnable =
> +      new ReleasePromiseRunnable(mWorkerPrivate, mPromiseHolder);
> +
> +    if (!releaseRunnable->Dispatch(nullptr)) {

Use the same cx you used above.

@@ +233,5 @@
> +    nsRefPtr<ReleasePromiseRunnable> releaseRunnable =
> +      new ReleasePromiseRunnable(mWorkerPrivate, mPromiseHolder);
> +
> +    if (!releaseRunnable->Dispatch(nullptr)) {
> +      NS_RUNTIMEABORT("Failed to dispatch control runnable.");

"PromiseHolder control runnable" or "ReleasePromiseRunnable"
Attachment #8513793 - Flags: review?(nsm.nikhil) → review?(amarchesini)
(Assignee)

Comment 101

4 years ago
Created attachment 8514656 [details] [diff] [review]
Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder.
Attachment #8513793 - Attachment is obsolete: true
Attachment #8513793 - Flags: review?(amarchesini)
Attachment #8514656 - Flags: review?(amarchesini)
(Assignee)

Comment 102

4 years ago
Created attachment 8516119 [details] [diff] [review]
Patch 1.5 - Update getServiced test and fix an assert when the registration has been removed.
Attachment #8516119 - Flags: review?(nsm.nikhil)
Comment on attachment 8516119 [details] [diff] [review]
Patch 1.5 - Update getServiced test and fix an assert when the registration has been removed.

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

::: dom/workers/test/serviceworkers/test_get_serviced.html
@@ +23,3 @@
>    function simpleRegister() {
> +    return navigator.serviceWorker.register("get_serviced_worker.js",
> +                                            { scope: "./sw_clients/" }).then((swr) => registration = swr);

Nit: Could you move the .then() to a new line.
Attachment #8516119 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8514656 [details] [diff] [review]
Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder.

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

Looks ok but I want to ask a quick feedback to bent when he is online. I'll write a comment in a few hours.

::: dom/workers/ServiceWorkerClients.cpp
@@ +222,5 @@
>      swm->GetServicedClients(mScope, result);
>      nsRefPtr<ResolvePromiseWorkerRunnable> r =
>        new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result);
>  
>      AutoSafeJSContext cx;

We should use AutoJSAPI instead. But it's not an issue for this bug.
Comment on attachment 8514656 [details] [diff] [review]
Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder.

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

::: dom/workers/ServiceWorkerClients.cpp
@@ +222,5 @@
>      swm->GetServicedClients(mScope, result);
>      nsRefPtr<ResolvePromiseWorkerRunnable> r =
>        new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result);
>  
>      AutoSafeJSContext cx;

We should use AutoJSAPI instead. But it's not an issue for this bug.
Attachment #8514656 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 107

4 years ago
Created attachment 8516934 [details] [diff] [review]
Patch 1.5 - Update getServiced test and fix an assert when the registration has been removed.
Attachment #8516119 - Attachment is obsolete: true
(Assignee)

Comment 108

4 years ago
Please land patches 1.1 through 1.5.
Keywords: checkin-needed
Reopening since the postMessage bits aren't landed yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Catalin, any updates on this? Any patches we could start on review would be good.
Flags: needinfo?(catalin.badea392)
(Assignee)

Comment 113

4 years ago
Created attachment 8559533 [details] [diff] [review]
Patch 2 - Add postMessage to service worker.
Attachment #8500165 - Attachment is obsolete: true
Attachment #8559533 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 114

4 years ago
Created attachment 8559535 [details] [diff] [review]
Patch 3 - Implement client.postMessage.

This includes the client.postMessage implementation from Patch 3 and Patch 4.
Attachment #8500166 - Attachment is obsolete: true
Attachment #8500167 - Attachment is obsolete: true
Attachment #8559535 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 115

4 years ago
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #112)
> Catalin, any updates on this? Any patches we could start on review would be
> good.

Tryrun for the two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=342cf7afd5cc

I will follow up with a patch that stubs the new API and then patches for individual features.

getServiced/getAll/get is still missing tests on m-i, I will add those along with the updated implementation.

Cheers!
Flags: needinfo?(catalin.badea392)
(Assignee)

Updated

4 years ago
Attachment #8559535 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 116

4 years ago
Created attachment 8560140 [details] [diff] [review]
Patch 4 - Remove prefix for SWClients. Rename getServiced to getAll. Add stub query options for GetAll
(Assignee)

Comment 117

4 years ago
Created attachment 8560141 [details] [diff] [review]
Patch 3 - Implement client.postMessage.
Attachment #8559535 - Attachment is obsolete: true
Attachment #8560141 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 118

4 years ago
Created attachment 8560142 [details] [diff] [review]
Patch 4 - Remove prefix for SWClients. Rename getServiced to getAll. Add stub query options for GetAll

This patch is a bit cumbersome to read.
It does the following:
1. s/getServiced/getAll, s/ServiceWorkerClient(s)/Client(s)
2. add stubbed options parameter to getAll and return error for anything other
   than the default options.
(Assignee)

Comment 119

4 years ago
Try run for Patch 2 and Patch 3 with correct tests for patch 3:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37cae710ea5d
Comment on attachment 8559533 [details] [diff] [review]
Patch 2 - Add postMessage to service worker.

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

::: dom/workers/ServiceWorker.h
@@ +62,5 @@
>    }
>  
> +#ifdef PostMessage
> +#undef PostMessage
> +#endif

WorkerPrivate.cpp seems to do this only on WIN_XP. Could you do that here too?
Attachment #8559533 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8560141 [details] [diff] [review]
Patch 3 - Implement client.postMessage.

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

::: dom/workers/ServiceWorkerClient.h
@@ +11,5 @@
>  #include "nsWrapperCache.h"
>  
>  namespace mozilla {
> +
> +class ErrorResult;

include ErrorResult

@@ +17,5 @@
>  namespace dom {
>  
>  class Promise;
> +template<typename T> class Optional;
> +template<typename T> class Sequence;

Just import BindingDeclarations.h
Attachment #8560141 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 123

4 years ago
Created attachment 8561588 [details] [diff] [review]
Patch 2 - Add postMessage to service worker.
Attachment #8559533 - Attachment is obsolete: true
(Assignee)

Comment 124

4 years ago
Created attachment 8561589 [details] [diff] [review]
Patch 3 - Implement client.postMessage.
Attachment #8560141 - Attachment is obsolete: true
(Assignee)

Comment 126

4 years ago
Comment on attachment 8561588 [details] [diff] [review]
Patch 2 - Add postMessage to service worker.

The webidl change was reviewed by :amarchesini in a prevous patch.
Attachment #8561588 - Flags: checkin?
(Assignee)

Updated

4 years ago
Attachment #8561589 - Flags: checkin?
Comment on attachment 8559533 [details] [diff] [review]
Patch 2 - Add postMessage to service worker.

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

::: dom/workers/ServiceWorker.cpp
@@ +77,5 @@
> +{
> +  WorkerPrivate* workerPrivate = GetWorkerPrivate();
> +  MOZ_ASSERT(workerPrivate);
> +
> +  workerPrivate->PostMessage(aCx, aMessage, aTransferable, aRv);

The spec says this one is should throw InvalidAccessError if state is redundant. Please attach a follow up patch.
(Assignee)

Comment 128

4 years ago
Created attachment 8561778 [details] [diff] [review]
Patch 3.1 Throw InvalidStateError when ServiceWorkerState == Redundant.
Attachment #8561778 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 129

4 years ago
Created attachment 8561780 [details] [diff] [review]
Patch 3.1 Throw InvalidStateError when ServiceWorkerState == Redundant.
Attachment #8561778 - Attachment is obsolete: true
Attachment #8561778 - Flags: review?(nsm.nikhil)
Attachment #8561780 - Flags: review?(nsm.nikhil)
Target Milestone: mozilla36 → ---
Attachment #8560140 - Attachment is obsolete: true
Attachment #8512138 - Flags: checkin+
Attachment #8512139 - Flags: checkin+
Attachment #8512818 - Flags: checkin+
Attachment #8514656 - Flags: checkin+
Attachment #8516934 - Flags: checkin+
Attachment #8561588 - Flags: checkin? → checkin+
Attachment #8561589 - Flags: checkin? → checkin+
Comment on attachment 8561588 [details] [diff] [review]
Patch 2 - Add postMessage to service worker.

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

::: dom/webidl/ServiceWorker.webidl
@@ +18,5 @@
>    readonly attribute ServiceWorkerState state;
>  
>    attribute EventHandler onstatechange;
> +
> +  // FIXME(catalinb): Should inherit this from Worker.

Do we have a bug for that? Write the bug id here.
Attachment #8561588 - Flags: review+
Attachment #8561780 - Flags: review?(nsm.nikhil) → review+
(In reply to Andrea Marchesini (:baku) from comment #131)
> Comment on attachment 8561588 [details] [diff] [review]
> Patch 2 - Add postMessage to service worker.
> 
> Review of attachment 8561588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/ServiceWorker.webidl
> @@ +18,5 @@
> >    readonly attribute ServiceWorkerState state;
> >  
> >    attribute EventHandler onstatechange;
> > +
> > +  // FIXME(catalinb): Should inherit this from Worker.
> 
> Do we have a bug for that? Write the bug id here.

Bug 1131347
(Assignee)

Comment 134

4 years ago
Comment on attachment 8561780 [details] [diff] [review]
Patch 3.1 Throw InvalidStateError when ServiceWorkerState == Redundant.

https://hg.mozilla.org/integration/mozilla-inbound/rev/408f0108ff53
Attachment #8561780 - Flags: checkin+
(Assignee)

Comment 136

4 years ago
Created attachment 8576014 [details] [diff] [review]
Test the number of clients returned by matchAll.
Attachment #8576014 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

4 years ago
Keywords: leave-open
Attachment #8576014 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Updated

4 years ago
Attachment #8576014 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/38fe830e9bde
https://hg.mozilla.org/mozilla-central/rev/6983a75e87c9
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.