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

      No description provided.

	Cătălin Badea (:catalinb) Assignee
	Comment 2 • 10 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?

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

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.

	Cătălin Badea (:catalinb) Assignee
	Comment 6 • 10 years ago

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

	Cătălin Badea (:catalinb) Assignee
	Comment 7 • 10 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

	Cătălin Badea (:catalinb) Assignee
	Comment 10 • 10 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.

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

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)

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

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.

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

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 14 • 10 years ago

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.

	Andrea Marchesini [:baku]
	Comment 18 • 10 years ago

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

	Andrea Marchesini [:baku]
	Comment 19 • 10 years ago

I'll wait for a new version of patch 1 before reviewing patch 2 and 3. Is it ok?

	Cătălin Badea (:catalinb) Assignee
	Comment 20 • 10 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

	Cătălin Badea (:catalinb) Assignee
	Comment 23 • 10 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.

	Andrea Marchesini [:baku]
	Comment 26 • 10 years ago

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;

	Andrea Marchesini [:baku]
	Comment 27 • 10 years ago

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?

	Andrea Marchesini [:baku]
	Comment 28 • 10 years ago

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

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

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);

	Cătălin Badea (:catalinb) Assignee
	Comment 30 • 10 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

	Andrea Marchesini [:baku]
	Comment 31 • 10 years ago

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.

	Cătălin Badea (:catalinb) Assignee
	Comment 33 • 10 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.

	Cătălin Badea (:catalinb) Assignee
	Comment 35 • 10 years ago

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

Added comment for follow-up.

	Cătălin Badea (:catalinb) Assignee
	Comment 37 • 10 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.

	Cătălin Badea (:catalinb) Assignee
	Comment 39 • 10 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.

	Andrea Marchesini [:baku]
	Comment 40 • 10 years ago

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.

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

Handle AddFeature failure gracefully.
Add call to getServiced() in onclose.
More thread assertions.

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

Changes to test. I'm not happy with not using ready. Maybe we should wait for that.

	Andrea Marchesini [:baku]
	Comment 43 • 10 years ago

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

	Andrea Marchesini [:baku]
	Comment 44 • 10 years ago

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.

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

The earlier patch passed a nsTArray where the WorkerStructuredCloneCallbacks expect a WorkerStructuredCloneClosure, obviously leading to all sorts of bad things.

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

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.

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

https://tbpl.mozilla.org/?tree=Try&rev=8df8092d3375

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

Comment on attachment 8478643 [details]
interdiff from Patch 4(v3)

WorkerStructuredCloneClosure is from MessagePort :(

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

https://tbpl.mozilla.org/?tree=Try&rev=93ec073b4be2

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

https://tbpl.mozilla.org/?tree=Try&rev=167a630c59a2

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

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

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

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

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

https://tbpl.mozilla.org/?tree=Try&rev=2a757b8cae02

	Cătălin Badea (:catalinb) Assignee
	Comment 54 • 10 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.

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 10 years ago

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

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

https://tbpl.mozilla.org/?tree=Try&rev=8cd8314611c8

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

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

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

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9244a16c3258
https://hg.mozilla.org/integration/mozilla-inbound/rev/05acfcc7a341
https://hg.mozilla.org/integration/mozilla-inbound/rev/095ca68e5d38
https://hg.mozilla.org/integration/mozilla-inbound/rev/547076d4a357

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 60 • 10 years ago

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

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

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.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2771520aa1b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe7add5dd11
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d397edb8e9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9674f68df2e5

	Ryan VanderMeulen [:RyanVM]
	Comment 63 • 10 years ago

(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

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

https://tbpl.mozilla.org/?tree=Try&rev=27ff7dc24a53

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

https://tbpl.mozilla.org/?tree=Try&rev=6f4a75f76b2b

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

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

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

https://tbpl.mozilla.org/?tree=Try&rev=20adbf8ca975

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

https://tbpl.mozilla.org/?tree=Try&rev=5ba14959743f

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

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

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

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

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

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

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

https://tbpl.mozilla.org/?tree=Try&rev=08810f7ebb2f

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

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

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

https://tbpl.mozilla.org/?tree=Try&rev=9a8ba0da16e4

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

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.

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

https://hg.mozilla.org/projects/maple/rev/20bbe32b1e3e
https://hg.mozilla.org/projects/maple/rev/2462a26c50ff
https://hg.mozilla.org/projects/maple/rev/2ad50f15b2d8
https://hg.mozilla.org/projects/maple/rev/79327fd9e5a4

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

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.

	(no longer active)
	Comment 78 • 10 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

	Cătălin Badea (:catalinb) Assignee
	Comment 79 • 10 years ago

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

Besides the build failing on Windows, it looks fine.

	Cătălin Badea (:catalinb) Assignee
	Comment 82 • 10 years ago

Rebased and fixed windows build bustage.

	Cătălin Badea (:catalinb) Assignee
	Comment 85 • 10 years ago

I've rebased the patch set and fixed the build from breaking on windows.
Patch 1.2 is ehsan's fix from maple: https://hg.mozilla.org/projects/maple/rev/a040c47e5db9.

patch 1.1, 1.2: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a9b20d24e95b
patch 2: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69bee59e97ef
whole patch set: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ceec68e1820a

	Cătălin Badea (:catalinb) Assignee
	Comment 89 • 10 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

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

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

	Cătălin Badea (:catalinb) Assignee
	Comment 93 • 10 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.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/062c0a4db943
https://hg.mozilla.org/integration/mozilla-inbound/rev/26cd857611b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae1b3474b22

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

sorry had to back this out since i guess this caused https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3350702&repo=mozilla-inbound

	Cătălin Badea (:catalinb) Assignee
	Comment 97 • 10 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.

	Cătălin Badea (:catalinb) Assignee
	Comment 98 • 10 years ago

Actually, lets wait on this. I believe there is still a race condition and its just less likely to occur.

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

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"

	Cătălin Badea (:catalinb) Assignee
	Comment 103 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e6f42e8edff0

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

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.

	Andrea Marchesini [:baku]
	Comment 105 • 10 years ago

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.

	Andrea Marchesini [:baku]
	Comment 106 • 10 years ago

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.

	Cătălin Badea (:catalinb) Assignee
	Comment 108 • 10 years ago

Please land patches 1.1 through 1.5.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed211a407dde
https://hg.mozilla.org/integration/mozilla-inbound/rev/736c29dc81ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fabdf3714a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f914e69229c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a0c1573df1

	Ryan VanderMeulen [:RyanVM]
	Comment 110 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/ed211a407dde
https://hg.mozilla.org/mozilla-central/rev/736c29dc81ce
https://hg.mozilla.org/mozilla-central/rev/7fabdf3714a5
https://hg.mozilla.org/mozilla-central/rev/5f914e69229c
https://hg.mozilla.org/mozilla-central/rev/a1a0c1573df1

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

Reopening since the postMessage bits aren't landed yet.

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

Catalin, any updates on this? Any patches we could start on review would be good.

	Cătălin Badea (:catalinb) Assignee
	Comment 114 • 9 years ago

This includes the client.postMessage implementation from Patch 3 and Patch 4.

	Cătălin Badea (:catalinb) Assignee
	Comment 115 • 9 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!

	Cătălin Badea (:catalinb) Assignee
	Comment 118 • 9 years ago

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.

	Cătălin Badea (:catalinb) Assignee
	Comment 119 • 9 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

	Cătălin Badea (:catalinb) Assignee
	Comment 120 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=caa4e6d559ed

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

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?

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

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

	Cătălin Badea (:catalinb) Assignee
	Comment 125 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f02999143c39

	Cătălin Badea (:catalinb) Assignee
	Comment 126 • 9 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.

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

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 130 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b105df291221
https://hg.mozilla.org/integration/mozilla-inbound/rev/bec72ec0f5ad

	Andrea Marchesini [:baku]
	Comment 131 • 9 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 132 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b105df291221
https://hg.mozilla.org/mozilla-central/rev/bec72ec0f5ad

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

(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

	Cătălin Badea (:catalinb) Assignee
	Comment 134 • 9 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

	Ryan VanderMeulen [:RyanVM]
	Comment 135 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/408f0108ff53

	Cătălin Badea (:catalinb) Assignee
	Comment 137 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=38fe830e9bde

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 138 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/38fe830e9bde
https://hg.mozilla.org/mozilla-central/rev/6983a75e87c9