949325 - C++ wrapper to support DataStore API on the worker

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Gene Lian (I already quit Mozilla)]

1. implement the DataStore interface in workers
2. any request should be proxied to the main thread.

An example about how to do this proxy in a sync way is this:

https://mxr.mozilla.org/mozilla-central/source/dom/workers/URL.cpp#65

If you decide to follow what URL does, maybe would be nice do make the URLRunnable a "public" class and rename it WorkerSimpleSyncRunnable (or something better).

Comment 1

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #0)
> If you decide to follow what URL does, maybe would be nice do make the
> URLRunnable a "public" class and rename it WorkerSimpleSyncRunnable (or
> something better).

jdm has already done this as part of 916199.

Comment 2

[Gene Lian (I already quit Mozilla)]

I don't see any WIP patches in that bug. Do you mean jdm has already done that in local?

Comment 3

[Gene Lian (I already quit Mozilla)]

Just for my records, indexedDB on worker is at:

https://bugzilla.mozilla.org/show_bug.cgi?id=701634

Comment 4

[Josh Matthews [:jdm]]

Yeah, I haven't attached it to Bugzilla yet, but you can see it at https://gist.github.com/jdm/7920929 .

Comment 5

[Gene Lian (I already quit Mozilla)]

Hi Josh, may I have your patch for TCPSocketStub to refer? :) I guess it's aimed for the proxy. Right?

Comment 6

[Gene Lian (I already quit Mozilla)]

Depending on bug 916199 where Josh made a generic SyncWorkerRunnable.

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

This WIP patch is aimed to add C++ stubs for DataStore and DataStoreCursor, which will still call the original JS implementations but will provide possibilities to dispatch tasks to the main thread for the workers.

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

Polished.

Plan:

0. Hook up the WebIDL to the C++ stubs instead of the original JS implementation.

1. Provide a new internal interface for the public attributes/functions of DataStore.jsm.

2. In the DataStoreService.js, |new aWindow.DataStore(...)| will use the new WebIDL to create the instance and let the C++ stubs call the internal interface to run the original JS implementations.

Comment 9

[Gene Lian (I already quit Mozilla)]

Hi Andrea, could you please take a look at comment #8? May I have your opinion about the overall plan? Thanks!

Comment 10

[Andrea Marchesini [:baku]]

I don't see why this helps this task. In the end you will have a C++ wrapper in workers calling a C++ stub on main-thread, calling a JS implementation. Why do you think this is better than: a C++ wrapper in workers running runnables on main-thread calling the JS implementation?

Comment 11

[Andrea Marchesini [:baku]]

Actually I think this code can be useful. In the end we want to rewrite DataStore in C++ so yes, stubbing this goes in the right direction.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> I don't see why this helps this task. In the end you will have a C++ wrapper
> in workers calling a C++ stub on main-thread, calling a JS implementation.
> Why do you think this is better than: a C++ wrapper in workers running
> runnables on main-thread calling the JS implementation?

Thanks for the reply! TBH, I don't understand the differences. If we want to dispatch runnables to the main thread, we must have some tasks to run and those tasks are actually "the C++ stubs calling JS implementations", where the C++ stubs are the C++ wrapper we're talking about.

My plan is going to be very similar to bug 916199 made by jdm. For example,

void
TCPSocketStub::GetHost(JSContext *aCx, nsAString& aHost)
{
  if (NS_IsMainThread()) {
    GetHost(aHost);
    return;
  }

  WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
  nsRefPtr<RunnableMethodStringArg> event =
      new RunnableMethodStringArg(workerPrivate, this, &TCPSocketStub::GetHost, aHost);
  event->Dispatch(aCx);
}

void
TCPSocketStub::GetHost(nsAString& aHost)
{
  MOZ_ASSERT(NS_IsMainThread());
  mSocket->GetHost(aHost);  
}

You can see how it dispatches the TCPSocketStub::GetHost as runnable to the main thread, where the TCPSocketStub::GetHost is the C++ stubs calling the JS implementation (i.e. mSocket->GetHost(aHost)).

Does that sound reasonable to you?

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

Hook up the C++ stubs to the JS implementation (WIP).

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

Fixed some TABs.

Comment 15

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

Polished.

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8350527 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

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

looks good so far but I have this comments:

1. What you "could" do is to rename:
- the JS implementation to DataStoreImpl (or a better name).
- DataStoreStub -> DataStore

2. Don't use IDL. Use WebIDL:
- DataStoreImpl.webidl and DataStore.idl should be good names.

I'm saying that because when we'll need to rewrite the JS implementation in C++, then we can just remove the DataStoreImpl.webidl.

::: dom/datastore/DataStore.jsm
@@ +369,5 @@
>    get readOnly() {
>      return this._readOnly;
>    },
>  
> +  getById: function(aId) {

why these?

::: dom/datastore/DataStoreStub.cpp
@@ +74,5 @@
> +
> +already_AddRefed<Promise> DataStoreStub::Get(uint32_t aId)
> +{
> +  nsCOMPtr<nsISupports> promise;
> +  nsresult rv = mStore->GetById(aId, getter_AddRefs(promise));

why do you need GetById?

@@ +83,5 @@
> +already_AddRefed<Promise> DataStoreStub::Get(const Sequence<uint32_t>& aIds)
> +{
> +  nsCOMPtr<nsISupports> promise;
> +
> +  // TODO: how to convert Sequence<uint32_t> into an JS array?

JS::Rooted<JSObject*> array(cx, JS_NewArrayObject(cx, aIds.Length(), nullptr));
for (uint32_t i = 0; i < aIds.Length(); ++i) {
  JS::Rooted<JS::Value> value(cx, INT_TO_JSVAL(aIds[i]));
  if (!JS_SetElement(cx, array, i, &value)) {
    // Some exception here
    return nullptr;
  }
}

something like this. If you need 'cx' you can add 'implicitJSContext' : [ 'get' ] in dom/bindings/Bindings.conf

Comment 18

[Gene Lian (I already quit Mozilla)]

(In reply to Andrea Marchesini (:baku) from comment #17)
> why do you need GetById?

Thanks for your feedback, Andrea.

Why I separate GetById and GetByIds is just because the XPCOM IDL cannot contain the same function names. Replacing it by WebIDL can solve this problem as well. ;)

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

Fixed comment #17 and polished the cycle collections.

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8351023 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor (WIP)

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

I guess you have to rename DataStore.jsm in DataStoreImpl.jsm (or at least the internal DataStore object just be renamed).

::: dom/datastore/DataStore.h
@@ +18,5 @@
> +class DataStoreImpl;
> +class StringOrUnsignedLong;
> +class OwningStringOrUnsignedLong;
> +
> +class DataStore : public nsDOMEventTargetHelper

MOZ_FINAL

@@ +26,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(DataStore,
> +                                                         nsDOMEventTargetHelper)
> +  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)
> +
> +  bool GetReadOnly(ErrorResult& aRv);

This is just about style, but I prefer

bool
GetReadOnly(ErrorResult& aRv);

the return type on a separated line here and everywhere.

::: dom/webidl/DataStore.webidl
@@ +23,4 @@
>    readonly attribute boolean readOnly;
>  
>    // Promise<any>
> +  [Throws]

This doesn't throw. At least the promise will be rejected. Is it?
The same for the other "Promise" return values.

Comment 21

[Gene Lian (I already quit Mozilla)]

Try for part-1 patch looks good: https://tbpl.mozilla.org/?tree=Try&rev=8cb8e857939d

Will fix comment #20 to ask for a formal review later.

(In reply to Andrea Marchesini (:baku) from comment #20)
> >    // Promise<any>
> > +  [Throws]
> 
> This doesn't throw. At least the promise will be rejected. Is it?
> The same for the other "Promise" return values.

Regarding this one. Please see DataStore.cpp/DataStoreCursor.cpp, you'll find the functions generated by DataStoreImpl.webidl will carry |ErrorResult& aRv|s as out parameters. To handle and report them properly, I think we'd better carry on the results. IMO, it doesn't harm. Right?

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor

Fixed comment #20 and re-run the try at the same time:

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

Comment 23

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, disptach tasks on the worker to the main thread (WIP)

Comment 24

[Gene Lian (I already quit Mozilla)]

(In reply to Andrea Marchesini (:baku) from comment #20)
> I guess you have to rename DataStore.jsm in DataStoreImpl.jsm (or at least
> the internal DataStore object just be renamed).

I might misunderstand. Please let me know if you want me to rename the internal object names as well. I only renamed the file names, which cannot be properly displayed in the bugzilla reviewer.

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 8351183 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor

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

Looks good! I want to see the second patch before giving a r+

::: dom/bindings/Bindings.conf
@@ +293,5 @@
>  'DataContainerEvent': {
>      'nativeType': 'nsDOMDataContainerEvent',
>  },
>  
> +'DataStore': {

remove all of this. I think it's better to install datastore headers in mozilla/dom/ and nativeType default value is ok.

::: dom/datastore/DataStore.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/datastore/DataStore.h"
> +#include "mozilla/dom/datastore/DataStoreCursor.h"

probably you don't need this.

@@ +6,5 @@
> +#include "mozilla/dom/datastore/DataStoreCursor.h"
> +#include "mozilla/dom/DataStoreBinding.h"
> +#include "mozilla/dom/DataStoreImplBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "nsComponentManagerUtils.h"

move this after ErrorResult.h

@@ +80,5 @@
> +  aOwner.Assign(owner);
> +}
> +
> +already_AddRefed<Promise>
> +DataStore::Get(const AutoSequence<OwningStringOrUnsignedLong>& aId,

This should be without ErrorResult and:

ErrorResult rv;
nsRefPtr<Promise> promise = mStore->Get(aId, rv);

if (rv.Failed() {
  promise = new Promise::Reject(); // or something like this...
  ...
}

return promise.forget();

the same approach should be used in Put()/Add()/Remove()...

::: dom/datastore/DataStore.h
@@ +74,5 @@
> +  virtual JSObject*
> +  WrapObject(JSContext *aCx,
> +             JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  DataStore(nsPIDOMWindow* aWindow, DataStoreImpl& aStore, ErrorResult& aRv);

can you move the constructor and distructor at the top of this file?
Something like:

public:
  DataStore(..);
  ~DataStore();

  // WebIDL

  Constructor(..)

  the rest

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

remove this line

::: dom/datastore/DataStoreCursor.cpp
@@ +9,5 @@
> +#include "mozilla/dom/Promise.h"
> +#include "nsContentUtils.h"
> +#include "nsPIDOMWindow.h"
> +#include "nsComponentManagerUtils.h"
> +#include "mozilla/ErrorResult.h"

Move this above.

@@ +47,5 @@
> +}
> +
> +nsPIDOMWindow*
> +DataStoreCursor::GetParentObject() const
> +{ 

additional space.

::: dom/datastore/moz.build
@@ +9,5 @@
>  ]
>  
>  XPIDL_MODULE = 'dom_datastore'
>  
> +EXPORTS.mozilla.dom.datastore += [

EXPORTS.mozilla.dom

::: dom/tests/mochitest/general/test_interfaces.html
@@ +189,5 @@
>      {name: "DataErrorEvent", b2g: true, pref: "dom.mobileconnection.enabled"},
>      {name: "DataStore", b2g: true, release: false},
>      {name: "DataStoreChangeEvent", b2g: true, release: false},
>      {name: "DataStoreCursor", b2g: true, release: false},
> +    {name: "DataStoreCursorImpl", b2g: true, release: false},

File a bug for rewrite DataStore in C++ (if it's not already there) and write a // TODO Bug <bugId>

::: dom/webidl/DataStore.webidl
@@ +23,4 @@
>    readonly attribute boolean readOnly;
>  
>    // Promise<any>
> +  [Throws]

These [Throws] should go away.

@@ +27,4 @@
>    Promise get(DataStoreKey... id);
>  
>    // Promise<void>
> +  [Throws]

ditto

@@ +31,4 @@
>    Promise put(any obj, DataStoreKey id);
>  
>    // Promise<DataStoreKey>
> +  [Throws]

ditto

@@ +35,4 @@
>    Promise add(any obj, optional DataStoreKey id);
>  
>    // Promise<boolean>
> +  [Throws]

ditto

@@ +39,4 @@
>    Promise remove(DataStoreKey id);
>  
>    // Promise<void>
> +  [Throws]

ditto

@@ +47,5 @@
>  
>    attribute EventHandler onchange;
>  
>    // Promise<unsigned long>
> +  [Throws]

ditto

@@ +52,3 @@
>    Promise getLength();
>  
> +  [Throws]

ditto

@@ +64,4 @@
>    readonly attribute DataStore store;
>  
>    // Promise<DataStoreTask>
> +  [Throws]

ditto

@@ +68,3 @@
>    Promise next();
>  
> +  [Throws]

ditto

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8351183 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor

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

What I think we should do is:

the Navigator::GetDataStores() should call the DataStoreService.jsm. This creates the DataStoreImpl and then a DataStore object from the DataStoreImpl.
The same for the DataStoreImpl.

We should not expose DataStoreImpl at all.

::: dom/datastore/DataStore.cpp
@@ +48,5 @@
> +{
> +}
> +
> +already_AddRefed<DataStore>
> +DataStore::Constructor(GlobalObject& aGlobal,

Remove this constructor.

::: dom/webidl/DataStore.webidl
@@ +5,5 @@
>   */
>  
>  typedef (DOMString or unsigned long) DataStoreKey;
>  
> +[Constructor(DataStoreImpl store),

Actually, the constructor should not exist.
You should create a DataStoreImpl in the ::DataStore()

@@ +55,4 @@
>    DataStoreCursor sync(optional DOMString revisionId = "");
>  };
>  
> +[Constructor(DataStoreCursorImpl cursor),

Also this constructor should not exist. Remove it.

Comment 27

[Josh Matthews [:jdm]]

Comment on attachment 8351183 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor

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

::: dom/tests/mochitest/general/test_interfaces.html
@@ +190,5 @@
>      {name: "DataStore", b2g: true, release: false},
>      {name: "DataStoreChangeEvent", b2g: true, release: false},
>      {name: "DataStoreCursor", b2g: true, release: false},
> +    {name: "DataStoreCursorImpl", b2g: true, release: false},
> +    {name: "DataStoreImpl", b2g: true, release: false},

These changes require a DOM peer's review, and don't look correct to me in any case. If you require a webIDL file for the implementation (and it is not clear to me that this is true), it should be [NoInterfaceObject].

Comment 28

[Gene Lian (I already quit Mozilla)]

> the Navigator::GetDataStores() should call the DataStoreService.jsm. This
> creates the DataStoreImpl and then a DataStore object from the DataStoreImpl.

As mentioned in the off-line email, I'd like to clarify:

1. If we don't want to provide a DataStoreImpl.webidl, the C++ codes for implementing the DataStore.webidl has to do lots of JS APIs to pass around parameters into JS_CallFunctionValue(...), which isn't worth it and can be over-engineering, because we're going to make DataStore C++'ed sooner or later.

2. I'd prefer at least provide a XPCOM IDL to wrap the DataStoreImpl JS object to solve #1. However, if we want to use XPCOM IDL, why not just use WebIDL? which is a newer IDL technology to use. Also, IMO, I'd pretty much prefer WebIDL, because both the generated C++ codes of DataStore.webidl and DataStoreImpl.webidl are in the WebIDL format, making the parameters easier to pass around.

3. To avoid expose unnecessary WebIDL out of Chrome codes, I'd like to use [Func=] to protect it so that we can much safely use the WebIDLs for internal uses. Please see my proposed patch later.

> This should be without ErrorResult and:
> 
> ErrorResult rv;
> nsRefPtr<Promise> promise = mStore->Get(aId, rv);
> 
> if (rv.Failed() {
>   promise = new Promise::Reject(); // or something like this...
>   ...
> }
> 
> return promise.forget();
> 
> the same approach should be used in Put()/Add()/Remove()...

Regarding this, I'd still have some concerns and questions:

1. It sounds weird to me that we have to return |Promise::Reject()| for |rv.Failed()| which can be actually an internal error returned by another WebIDL implementation due to any unknown reasons (e.g., failed to get a certain service or something). However, |Promise::Reject()| should be returned when we're sure this API is returning something *negative* that is defined by the API behaviour. I still prefer passing around |rv| as it is. Please correct me if I'm wrong.

2. Another question is an implementation issue I encountered:

   static already_AddRefed<Promise>
   Reject(const GlobalObject& aGlobal, JSContext* aCx,
          const Optional<JS::Handle<JS::Value>>& aValue, ErrorResult& aRv);

which has to eat GlobalObject and it can only be available in the Constructor or static functions. I don't how to get one in the normal APIs. Could you please give some hints? Thanks!

Comment 29

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V2

Hi Andrea,

Please see my concerns and questions at comment #28. Personally, I'd prefer keeping DataStoreImpl.webidl because it's easier to be removed when we'll make DataStore APIs C++'ed in the future.

If you insist on getting rid of the DataStoreImpl.webidl, I'd appreciate if you could please give me some hints about how to call the DataStoreImpl JS object from C++ codes without using XPCOM IDL or WebIDL. Thanks a lot! :)

Comment 30

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8351356 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V2

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

::: dom/datastore/DataStoreImpl.jsm
@@ +477,5 @@
> +    let cursorImpl = this._window.DataStoreCursorImpl.
> +                                  _create(this._window, this._cursor);
> +
> +    let exposedCursor = new this._window.DataStoreCursor();
> +    exposedCursor.setDataStoreCursorImpl(cursorImpl);    

Self-review:

1. Remove additional blanks.
2. return exposedCursor;

Comment 31

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, disptach tasks on the worker to the main thread (WIP)

Comment 32

[Gene Lian (I already quit Mozilla)]

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

Comment 33

[Andrea Marchesini [:baku]]

Comment on attachment 8351356 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V2

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

wow!!

::: dom/datastore/DataStore.cpp
@@ +37,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mStore)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +DataStore::DataStore(nsPIDOMWindow* aWindow,
> +                     ErrorResult& aRv)

remove this ErrorResult

@@ +83,5 @@
> +}
> +
> +void
> +DataStore::SetDataStoreImpl(DataStoreImpl& aStore, ErrorResult& aRv)
> +{

MOZ_ASSERT(!mStore)

@@ +90,5 @@
> +}
> +
> +bool
> +DataStore::GetReadOnly(ErrorResult& aRv)
> +{

I would add: MOZ_ASSERT(mStore) here and everywhere

::: dom/datastore/DataStoreCursor.cpp
@@ +72,5 @@
> +}
> +
> +void
> +DataStoreCursor::SetDataStoreCursorImpl(DataStoreCursorImpl& aCursor)
> +{

MOZ_ASSERT(!mCursor)

@@ +77,5 @@
> +  mCursor = &aCursor;
> +}
> +
> +already_AddRefed<DataStore>
> +DataStoreCursor::GetStore(ErrorResult& aRv)

MOZ_ASSERT(mCursor)

@@ +85,5 @@
> +}
> +
> +already_AddRefed<Promise>
> +DataStoreCursor::Next(ErrorResult& aRv)
> +{

MOZ_ASSERT

@@ +92,5 @@
> +}
> +
> +void
> +DataStoreCursor::Close(ErrorResult& aRv)
> +{

MOZ_ASSERT

Comment 34

[Andrea Marchesini [:baku]]

Comment on attachment 8351356 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V2

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

::: dom/datastore/DataStoreImpl.jsm
@@ +477,5 @@
> +    let cursorImpl = this._window.DataStoreCursorImpl.
> +                                  _create(this._window, this._cursor);
> +
> +    let exposedCursor = new this._window.DataStoreCursor();
> +    exposedCursor.setDataStoreCursorImpl(cursorImpl);    

return is missing

Comment 35

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, disptach tasks on the worker to the main thread (WIP)

Comment 36

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V3

Note that in test_interfaces.html, I don't think we should expose DataStore and DataStoreCursor either, because only chrome codes will create instances by the constructors.

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=47d82e8c1bc2

Comment 37

[Andrea Marchesini [:baku]]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #36)
> Created attachment 8356092 [details] [diff] [review]
> Part 1, C++ stubs for DataStore and DataStoreCursor, V3
> 
> Note that in test_interfaces.html, I don't think we should expose DataStore
> and DataStoreCursor either, because only chrome codes will create instances
> by the constructors.

Sure. but it's important to have it because otherwise it's not possible to check the instanceof a value.

Comment 38

[(no longer active)]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #36)
> Created attachment 8356092 [details] [diff] [review]
> Part 1, C++ stubs for DataStore and DataStoreCursor, V3
> 
> Note that in test_interfaces.html, I don't think we should expose DataStore
> and DataStoreCursor either, because only chrome codes will create instances
> by the constructors.

Hmm, this is puzzling to me.  You only want to disable the constructor, not the visibility of the interface object, right?  AFAIK, the Func extended WebIDL attribute does the latter not the former.  (And your test_interfaces.html changes are wrong, but they're necessary because you're removing the interface object!)

Comment 39

[Gene Lian (I already quit Mozilla)]

Thanks Andrea and Ehsan for the suggestions. I'll correct it in my next patch.

Comment 40

[Andrea Marchesini [:baku]]

Comment on attachment 8356092 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V3

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

looks good! ehsan can you review this code too?

::: dom/datastore/DataStore.cpp
@@ +8,5 @@
> +#include "mozilla/dom/DataStoreImplBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"
> +#include "nsComponentManagerUtils.h"

do you need this?

@@ +55,5 @@
> +  return store.forget();
> +}
> +
> +nsPIDOMWindow*
> +DataStore::GetParentObject() const

remove this.

::: dom/datastore/DataStore.h
@@ +36,5 @@
> +  static already_AddRefed<DataStore>
> +  Constructor(GlobalObject& aGlobal, ErrorResult& aRv);
> +
> +  nsPIDOMWindow*
> +  GetParentObject() const;

remove this. nsDOMEventTargetHelper should provide this method.

::: dom/datastore/DataStoreCursor.cpp
@@ +8,5 @@
> +#include "mozilla/dom/DataStoreImplBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"
> +#include "nsContentUtils.h"

do you need this?

@@ +10,5 @@
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"
> +#include "nsContentUtils.h"
> +#include "nsPIDOMWindow.h"
> +#include "nsComponentManagerUtils.h"

this?

::: dom/tests/mochitest/general/test_interfaces.html
@@ -186,5 @@
>      "CustomEvent",
>      "DataChannel",
>      "DataContainerEvent",
>      {name: "DataErrorEvent", b2g: true, pref: "dom.mobileconnection.enabled"},
> -    {name: "DataStore", b2g: true, release: false},

you should reintroduce these 2 interfaces.

::: dom/webidl/DataStore.webidl
@@ +5,5 @@
>   */
>  
>  typedef (DOMString or unsigned long) DataStoreKey;
>  
> +[Constructor,

Write a comment here saying that this constructor will be removed once DataStore is fully rewritten in C++

@@ +58,4 @@
>    DataStoreCursor sync(optional DOMString revisionId = "");
>  };
>  
> +[Constructor,

The same comment should go here.

Comment 41

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8356092 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V3

I'll fix the above-mentioned issues and then ask for Ehsan's review in a couple of hours later.

Comment 42

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V4, r=baku

Fixed comment #38 and comment #40.

Comment 43

[Gene Lian (I already quit Mozilla)]

Full try for part-1 patch: https://tbpl.mozilla.org/?tree=Try&rev=c9f1c9744b34

Comment 44

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, disptach tasks on the worker to the main thread (WIP)

Comment 45

[Andrea Marchesini [:baku]]

Comment on attachment 8356555 [details] [diff] [review]
Part 2, disptach tasks on the worker to the main thread (WIP)

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

great!

::: dom/datastore/DataStore.cpp
@@ +98,3 @@
>  {
>    MOZ_ASSERT(mStore);
> +

I would add:

MOZ_ASSERT(aCx || NS_IsMainThread())

just because you call it with nullptr in the runnables.

@@ +321,3 @@
>  }
>  
> +// TODO How to handle the event?

Let me think about this. Maybe smaug can have good ideas about.

::: dom/datastore/DataStoreWorkerUtils.h
@@ +16,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +using namespace workers;
> + 

extra space :)

@@ +18,5 @@
> +
> +using namespace workers;
> + 
> +// Base class for the {DataStore,DataStoreCursor}Runnable.
> +class BaseRunnable : public nsRunnable

We should really have this class somewhere. URL and some other piece of code use something similar.

Comment 46

[(no longer active)]

Comment on attachment 8356554 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V4, r=baku

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

Minusing for the issue of constructors.  Please note that I have not yet reviewed most of the parts of the patch -- just want to make sure that we get the basic API right first.

::: dom/datastore/DataStore.cpp
@@ +73,5 @@
> +}
> +
> +/*static*/ bool
> +DataStore::EnabledForScope(JSContext* /* unused */,
> +                           JS::Handle<JSObject*> aObj)

Note that this is now dead code.

::: dom/webidl/DataStore.webidl
@@ +10,5 @@
> +//                   removed once the DataStore API is fully rewritten in C++,
> +//                   which currently plays a role of C++ proxy directing to the
> +//                   JS codes implemented by the DataStoreImpl WebIDL.
> +
> +[PrefControlled, Constructor]

Two questions:

1. Why did you remove the name of the pref from the WebIDL?  The DataStore::PrefEnabled() function just returns true if the pref is set, so I don't understand why you're using [PrefControlled] here.

2. With this patch, the following code will start to work in content, is that right?

  var ds = new DataStore();

That's bad, since it means that we'll expose an API which we don't want.  Or am I missing something?

The thing is, what you need here is the ability to expose a constructor at runtime based on a function (similar to [Func] for the visibility of the interface itself.)  AFAIK currently the WebIDL codegen doesn't support that, but I don't think you can avoid it.  I think you need to modify the WebIDL codegen to support that first.

@@ +15,3 @@
>  interface DataStore : EventTarget {
> +  [ChromeOnly, Throws]
> +  void setDataStoreImpl(DataStoreImpl store);

Nit: please remove this method from here and move it down to a partial interface DataStore section, so that the main definition of the interface remains similar to what appears in the spec.

@@ +19,2 @@
>    // Returns the label of the DataSource.
> +  [GetterThrows]

Why are all of these needed?  The DataStoreImpl interface doesn't throw on these attributs/methods.

@@ +67,5 @@
> +//                   removed once the DataStore API is fully rewritten in C++,
> +//                   which currently plays a role of C++ proxy directing to the
> +//                   JS codes implemented by the DataStoreCursorImpl WebIDL.
> +
> +[PrefControlled, Constructor]

Same comments as above.

Comment 47

[Gene Lian (I already quit Mozilla)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #46)
> Comment on attachment 8356554 [details] [diff] [review]
> Part 1, C++ stubs for DataStore and DataStoreCursor, V4, r=baku
> 
> Review of attachment 8356554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minusing for the issue of constructors.  Please note that I have not yet
> reviewed most of the parts of the patch -- just want to make sure that we
> get the basic API right first.

Thanks Ehsan very much for the review. Please see my clarification below.

> 
> ::: dom/datastore/DataStore.cpp
> @@ +73,5 @@
> > +}
> > +
> > +/*static*/ bool
> > +DataStore::EnabledForScope(JSContext* /* unused */,
> > +                           JS::Handle<JSObject*> aObj)
> 
> Note that this is now dead code.

No, EnabledForScope(...) is still used by DataStoreImpl/DataStoreCursorImpl WebIDLs, which makes these two interface invisible to the content but can still be use by internal chrome codes.

> 
> ::: dom/webidl/DataStore.webidl
> @@ +10,5 @@
> > +//                   removed once the DataStore API is fully rewritten in C++,
> > +//                   which currently plays a role of C++ proxy directing to the
> > +//                   JS codes implemented by the DataStoreImpl WebIDL.
> > +
> > +[PrefControlled, Constructor]
> 
> Two questions:
> 
> 1. Why did you remove the name of the pref from the WebIDL?  The
> DataStore::PrefEnabled() function just returns true if the pref is set, so I
> don't understand why you're using [PrefControlled] here.

Indeed, we can still keep the |Pref="dom.datastore.enabled"|. I intentionally did this just because I hope to refactor out a DataStore::PrefEnabled() function to be reused by EnabledForScope(...). I'll restore that if you prefer. :)

> 2. With this patch, the following code will start to work in content, is
> that right?
> 
>   var ds = new DataStore();
> 
> That's bad, since it means that we'll expose an API which we don't want.  Or
> am I missing something?

No, the constructor won't work. Please see what I did for the implementation of DataStore/DataStoreCursor constructors:

DataStore::Constructor(GlobalObject& aGlobal,
                       ErrorResult& aRv)
{
  // Only the chrome codes in Gecko can use the constructor.
  if (xpc::AccessCheck::isChrome(aGlobal.Get())) {
    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
    return nullptr;
  }

  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
  nsRefPtr<DataStore> store = new DataStore(window);
  return store.forget();
}

which is inspired by WebSocket::Constructor(...).

> @@ +15,3 @@
> >  interface DataStore : EventTarget {
> > +  [ChromeOnly, Throws]
> > +  void setDataStoreImpl(DataStoreImpl store);
> 
> Nit: please remove this method from here and move it down to a partial
> interface DataStore section, so that the main definition of the interface
> remains similar to what appears in the spec.

Sounds good. I'll try that.

> 
> @@ +19,2 @@
> >    // Returns the label of the DataSource.
> > +  [GetterThrows]
> 
> Why are all of these needed?  The DataStoreImpl interface doesn't throw on
> these attributs/methods.

Please see the implementation in DataStore.cpp. For example:

void
DataStore::GetName(nsAString& aName, ErrorResult& aRv)
{
  MOZ_ASSERT(mStore);
  nsAutoString name;
  mStore->GetName(name, aRv);
  aName.Assign(name);
}

You can see the |mStore->GetName(...)| generated by the DataStoreImpl WebIDL still pop up the |aRv| although it's based on a JS implementation, so I'd prefer carry on that to ensure consistency. Also, since DataStore WebIDL is based on a C++ implementation, throwing exceptions makes sense in any way no matter how we implement it in details.

Comment 48

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V4.1, r=baku

Hi Ehsan, could you please see my clarifications in comment #47? I directly asked for the review again and please let me know if it doesn't make sense to you. Thanks!

Comment 49

[Andrea Marchesini [:baku]]

Comment on attachment 8356945 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V4.1, r=baku

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

::: dom/datastore/DataStore.cpp
@@ +70,5 @@
> +DataStore::EnabledForScope(JSContext* /* unused */,
> +                           JS::Handle<JSObject*> aObj)
> +{
> +  // Only expose the interface when it is:
> +  // 1. enabled by the preference and 

extra space

::: dom/datastore/DataStoreCursor.cpp
@@ +67,5 @@
> +DataStoreCursor::EnabledForScope(JSContext* /* unused */,
> +                                 JS::Handle<JSObject*> aObj)
> +{
> +  // Only expose the interface when it is:
> +  // 1. enabled by the preference and 

extra space

Comment 50

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases for running workers

Hi Andrea,

I just worked out a very quick patch to show my idea. To be honest, I'm not very familiar with the use of worker. Could you please take a look to see if I'm in the right direction?

I made two files:

  file_basic_worker.html
  file_basic_worker.js

As you can see the file_basic_worker.html launches a worker (file_basic_worker.js) which will run the original test cases for basic DataStore APIs.

In the worker, I replaced alert() by postMessage(), so the file_basic_worker.html will catch the worker's messages via onmessage and call alert() to notify test_basic.html which handles the tests.

Does that make sense to you overall?

Comment 51

[(no longer active)]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #47)
> > ::: dom/datastore/DataStore.cpp
> > @@ +73,5 @@
> > > +}
> > > +
> > > +/*static*/ bool
> > > +DataStore::EnabledForScope(JSContext* /* unused */,
> > > +                           JS::Handle<JSObject*> aObj)
> > 
> > Note that this is now dead code.
> 
> No, EnabledForScope(...) is still used by DataStoreImpl/DataStoreCursorImpl
> WebIDLs, which makes these two interface invisible to the content but can
> still be use by internal chrome codes.

Oh, right.  I missed that.  Sorry!

> > ::: dom/webidl/DataStore.webidl
> > @@ +10,5 @@
> > > +//                   removed once the DataStore API is fully rewritten in C++,
> > > +//                   which currently plays a role of C++ proxy directing to the
> > > +//                   JS codes implemented by the DataStoreImpl WebIDL.
> > > +
> > > +[PrefControlled, Constructor]
> > 
> > Two questions:
> > 
> > 1. Why did you remove the name of the pref from the WebIDL?  The
> > DataStore::PrefEnabled() function just returns true if the pref is set, so I
> > don't understand why you're using [PrefControlled] here.
> 
> Indeed, we can still keep the |Pref="dom.datastore.enabled"|. I
> intentionally did this just because I hope to refactor out a
> DataStore::PrefEnabled() function to be reused by EnabledForScope(...). I'll
> restore that if you prefer. :)

I think it's preferred to drop in the pref name in the IDL to make it more obvious how the interface is exposed.

> > 2. With this patch, the following code will start to work in content, is
> > that right?
> > 
> >   var ds = new DataStore();
> > 
> > That's bad, since it means that we'll expose an API which we don't want.  Or
> > am I missing something?
> 
> No, the constructor won't work. Please see what I did for the implementation
> of DataStore/DataStoreCursor constructors:
> 
> DataStore::Constructor(GlobalObject& aGlobal,
>                        ErrorResult& aRv)
> {
>   // Only the chrome codes in Gecko can use the constructor.
>   if (xpc::AccessCheck::isChrome(aGlobal.Get())) {
>     aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
>     return nullptr;
>   }
> 
>   nsCOMPtr<nsPIDOMWindow> window =
> do_QueryInterface(aGlobal.GetAsSupports());
>   nsRefPtr<DataStore> store = new DataStore(window);
>   return store.forget();
> }
> 
> which is inspired by WebSocket::Constructor(...).

Yeah I saw that.  What I'm suggesting is that maybe we should consider doing that in the codegen.  The thing is that as things stand, anybody who reads the IDL will assume that the constructor is exposed to content.

If you don't agree, please ask Boris, and I'm fine with whatever he suggests.

> > @@ +19,2 @@
> > >    // Returns the label of the DataSource.
> > > +  [GetterThrows]
> > 
> > Why are all of these needed?  The DataStoreImpl interface doesn't throw on
> > these attributs/methods.
> 
> Please see the implementation in DataStore.cpp. For example:
> 
> void
> DataStore::GetName(nsAString& aName, ErrorResult& aRv)
> {
>   MOZ_ASSERT(mStore);
>   nsAutoString name;
>   mStore->GetName(name, aRv);
>   aName.Assign(name);
> }
> 
> You can see the |mStore->GetName(...)| generated by the DataStoreImpl WebIDL
> still pop up the |aRv| although it's based on a JS implementation, so I'd
> prefer carry on that to ensure consistency. Also, since DataStore WebIDL is
> based on a C++ implementation, throwing exceptions makes sense in any way no
> matter how we implement it in details.

Oh, so this is necessary in order to catch JS exceptions, right?  I'm not sure if that's the right thing to do.  Boris?

(I'll review the rest of the patch once we settle on the issue of the constructor here.)

Comment 52

[Boris Zbarsky [:bzbarsky]]

> If you don't agree, please ask Boris, and I'm fine with whatever he suggests.

If we want content to see a DataStore interface object but not be able to use the constructor, we should use ChromeConstructor() instead of Constructor().  My apologies that the fix for bug 938544 didn't get documented.

If we don't want content to see the interface object, we should be flagging the interface appropriately.

In either case, you shouldn't need to do manual security checks here.

> Oh, so this is necessary in order to catch JS exceptions, right? 

It's necessary to not silently drop exceptions on the floor.  The issue is that DataStoreImpl is JS-implemented, and we assume that all methods/getters/setters of JS-implemented interfaces can throw, because it's actually pretty very difficult to write JS code that never throws (you have to watch out for out-of-stack, out-of-memory, etc, etc).  So all JS-implemented methods are assumed fallible.

You could have DataStore use an on-stack ErrorResult and assert that an exception has never been thrown, but all that would do is fail the assert (and leak) if an exception ever does get thrown.  Better to go ahead and propagate it through to the bindings code, which will then go ahead and report it correctly, etc, which is what this patch is doing.

Comment 53

[Boris Zbarsky [:bzbarsky]]

One other note.  For the worker patch, you could just get the worker JSContext directly using GetCurrentThreadJSContext() no?  Is having the binding code provide it just an optimization and does it really save enough to need the extra complexity?

Alternately, if you're just after the worker private, you could GetCurrentThreadWorkerPrivate(), and not need to deal with JSContext at all.

Comment 54

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #52)
> > If you don't agree, please ask Boris, and I'm fine with whatever he suggests.
> 
> If we want content to see a DataStore interface object but not be able to
> use the constructor, we should use ChromeConstructor() instead of
> Constructor().  My apologies that the fix for bug 938544 didn't get
> documented.
> 
> If we don't want content to see the interface object, we should be flagging
> the interface appropriately.
> 
> In either case, you shouldn't need to do manual security checks here.

Ah, ChromeConstructor is exactly what we need here!

> > Oh, so this is necessary in order to catch JS exceptions, right? 
> 
> It's necessary to not silently drop exceptions on the floor.  The issue is
> that DataStoreImpl is JS-implemented, and we assume that all
> methods/getters/setters of JS-implemented interfaces can throw, because it's
> actually pretty very difficult to write JS code that never throws (you have
> to watch out for out-of-stack, out-of-memory, etc, etc).  So all
> JS-implemented methods are assumed fallible.

Yeah that makes sense.

> You could have DataStore use an on-stack ErrorResult and assert that an
> exception has never been thrown, but all that would do is fail the assert
> (and leak) if an exception ever does get thrown.  Better to go ahead and
> propagate it through to the bindings code, which will then go ahead and
> report it correctly, etc, which is what this patch is doing.

No I don't think we should do that.  The Throws annotations are fine for now as they are, until we port this whole thing to C++.

Comment 55

[(no longer active)]

Gene, I'm not going to get to do a proper review of this patch tonight, so I'll just let the flag remain set so that this stays in my queue for tomorrow.  If you get to make the ChromeConstructor change in the mean time please upload a new patch, otherwise I'll review this tomorrow assuming that you will be using a ChromeConstructor.  Sorry for the back and forth here.  :-)

Comment 56

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V5, r=baku

Use ChromeConstructor.

Thanks Ehsan and Boris for the suggestions!

Full try: https://tbpl.mozilla.org/?tree=Try&rev=63a7bc51c7e7

Comment 57

[Gene Lian (I already quit Mozilla)]

Hi Andrea,

I encountered when trying to run the DataStore on the worker. Hope you can give me some directions if you have a chance. Thanks! Please refer to the following code segment in Navigator.cpp:

  already_AddRefed<Promise>
  Navigator::GetDataStores(const nsAString& aName, ErrorResult& aRv)
  {
    if (!mWindow || !mWindow->GetDocShell()) {
      aRv.Throw(NS_ERROR_UNEXPECTED);
      return nullptr;
    }

    nsCOMPtr<nsIDataStoreService> service =
      do_GetService("@mozilla.org/datastore-service;1");
    if (!service) {
      aRv.Throw(NS_ERROR_FAILURE);
      return nullptr;
    }

    nsCOMPtr<nsISupports> promise;
    aRv = service->GetDataStores(mWindow, aName, getter_AddRefs(promise));

    nsRefPtr<Promise> p = static_cast<Promise*>(promise.get());
    return p.forget();
  }

Question 1: it seems the test case stops at the (!mWindow || !mWindow->GetDocShell()) check, where |nWindow| can be null because a worker has no window. How can we solve that?

Question 2: do we need to dispatch the Navigator::GetDataStores() to the main thread just like other APIs do? I used to heard my colleague said do_GetService() doesn't work on the non-main-thread if its XPCOM service is a JS implementation, which seems to force us to do that?

Comment 58

[Gene Lian (I already quit Mozilla)]

s/encountered/encountered problems/

Comment 59

[(no longer active)]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #57)
> Question 2: do we need to dispatch the Navigator::GetDataStores() to the
> main thread just like other APIs do? I used to heard my colleague said
> do_GetService() doesn't work on the non-main-thread if its XPCOM service is
> a JS implementation, which seems to force us to do that?

Yes, that should be proxied to the main thread.

Comment 60

[(no longer active)]

Comment on attachment 8357539 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V5, r=baku

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

::: dom/datastore/DataStore.cpp
@@ +48,5 @@
> +already_AddRefed<DataStore>
> +DataStore::Constructor(GlobalObject& aGlobal,
> +                       ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());

You should handle window being null here.

@@ +67,5 @@
> +  // Only expose the interface when it is:
> +  // 1. enabled by the preference and
> +  // 2. accessed by the chrome codes in Gecko.
> +  return (Preferences::GetBool("dom.datastore.enabled", false) &&
> +          xpc::AccessCheck::isChrome(aObj));

Hmm, I think you can remove this method, and just use a [Pref=...] and [ChromeOnly] on the WebIDL.

@@ +75,5 @@
> +DataStore::SetDataStoreImpl(DataStoreImpl& aStore, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(!mStore);
> +  mStore = &aStore;
> +  mStore->SetEventTarget(*this, aRv);

Please document this clearly somewhere (perhaps in the header)?  It took me several minutes to figure out why the event target handling bits in this patch are correct!

@@ +108,5 @@
> +DataStore::Get(const AutoSequence<OwningStringOrUnsignedLong>& aId,
> +               ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(mStore);
> +  nsRefPtr<Promise> promise = mStore->Get(aId, aRv);

For these methods which return an already_AddRefed<Promise>, you should be able to just do something similar to:

return mStore->Get(aId, aRv);

::: dom/datastore/DataStore.h
@@ +27,5 @@
> +                                                         nsDOMEventTargetHelper)
> +  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)
> +
> +public:
> +  DataStore(nsPIDOMWindow* aWindow);

Nit: please mark this as explicit.

@@ +86,5 @@
> +
> +  already_AddRefed<DataStoreCursor>
> +  Sync(const nsAString& aRevisionId, ErrorResult& aRv);
> +
> +  IMPL_EVENT_HANDLER(change)

I think you should use EventTargetProxy class that Josh has implemented in bug 916199 in the worker parts of this patch.

::: dom/datastore/DataStoreCursor.cpp
@@ +39,5 @@
> +already_AddRefed<DataStoreCursor>
> +DataStoreCursor::Constructor(GlobalObject& aGlobal,
> +                             ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());

Same here.

@@ +64,5 @@
> +  // Only expose the interface when it is:
> +  // 1. enabled by the preference and
> +  // 2. accessed by the chrome codes in Gecko.
> +  return (Preferences::GetBool("dom.datastore.enabled", false) &&
> +          xpc::AccessCheck::isChrome(aObj));

Same here.

@@ +78,5 @@
> +already_AddRefed<DataStore>
> +DataStoreCursor::GetStore(ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(mCursor);
> +  nsRefPtr<DataStore> store = mCursor->GetStore(aRv);

Same here and in Next()!

::: dom/datastore/DataStoreCursor.h
@@ +21,5 @@
> +class GlobalObject;
> +class DataStoreCursorImpl;
> +
> +class DataStoreCursor MOZ_FINAL : public nsISupports,
> +                                  public nsWrapperCache

So, the only way to get your hands onto a DataStoreCursor from content script is by calling sync().  Now, sync always returns a new object, so once you lose your JS reference to this DataStoreCursor object, there is no way to regain a new reference to the same object, so this doesn't need to be wrapper-cached.

Which reminds me, we should mark the sync method as [NewObject] in the WebIDL, both in our code and in the spec!

@@ +28,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(DataStoreCursor)
> +
> +public:
> +  DataStoreCursor(nsPIDOMWindow* aOwner);

Nit: explicit.

::: dom/webidl/DataStore.webidl
@@ +51,3 @@
>    readonly attribute DOMString revisionId;
>  
>    attribute EventHandler onchange;

The reason why this is not marked as [Throws] is that it's not properly forwarded to the JS implementation (see my comment in DataStore.h)

Comment 61

[(no longer active)]

Actually, Boris told me that you can't use both [Pref] and [ChromeOnly], so forget about that part of my comments!

Comment 62

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

Comment on attachment 8357539 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V5, r=baku

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

::: dom/datastore/DataStore.cpp
@@ +66,5 @@
> +{
> +  // Only expose the interface when it is:
> +  // 1. enabled by the preference and
> +  // 2. accessed by the chrome codes in Gecko.
> +  return (Preferences::GetBool("dom.datastore.enabled", false) &&

On the worker thread you'll have to use the worker preferences system and add WorkerPrivate::DataStoreEnabled(). Preferences is not thread safe.

Comment 63

[Gene Lian (I already quit Mozilla)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)

Thanks for the review!

> @@ +86,5 @@
> > +
> > +  already_AddRefed<DataStoreCursor>
> > +  Sync(const nsAString& aRevisionId, ErrorResult& aRv);
> > +
> > +  IMPL_EVENT_HANDLER(change)
> 
> I think you should use EventTargetProxy class that Josh has implemented in
> bug 916199 in the worker parts of this patch.

I'll fix this in my part-2 patch which really proxies the tasks from the worker to the main thread. In this patch, I'll still keep it as it is. Does that sound reasonable to you?

The rest looks good to me. I'll try to fix them. Thanks!

Comment 64

[Gene Lian (I already quit Mozilla)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #62)
> On the worker thread you'll have to use the worker preferences system and
> add WorkerPrivate::DataStoreEnabled(). Preferences is not thread safe.

Hi, Nikhil, thanks for the comment! But I don't really get this. Could you please point out where the "worker preferences system" is?

Anyway, I'll try to solve this in my part-2 patch which really takes the worker case into consideration.

Comment 65

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6, r=baku

Fixed comment #60. Note that to make DataStoreCursor no wrapperCache, I did:

1. Add 'wrapperCache': False for it in Bindings.conf.
2. Add [NewObject] for DataStore.sync() and DataStoreImpl.sync().
3. DataStoreCursor don't need to inherit nsWrapperCache.
4. Remove DataStoreCursor::GetParentObject() and thus don't need to keep |mOwner|.
5. Don't need to call SetIsDOMBinding() in the DataStoreCursor::DataStoreCursor().

Comment 66

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

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #64)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #62)
> > On the worker thread you'll have to use the worker preferences system and
> > add WorkerPrivate::DataStoreEnabled(). Preferences is not thread safe.
> 
> Hi, Nikhil, thanks for the comment! But I don't really get this. Could you
> please point out where the "worker preferences system" is?
> 
> Anyway, I'll try to solve this in my part-2 patch which really takes the
> worker case into consideration.

https://mxr.mozilla.org/mozilla-central/source/dom/workers/Workers.h#166
https://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#1702 (and corresponding unregister around line 1867)
https://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2354
http://mxr.mozilla.org/mozilla-central/source/dom/workers/RegisterBindings.cpp#67
and http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.h#987

Comment 67

[Gene Lian (I already quit Mozilla)]

Thanks Nikhil!

Comment 68

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #57)
> Question 1: it seems the test case stops at the (!mWindow ||
> !mWindow->GetDocShell()) check, where |nWindow| can be null because a worker
> has no window. How can we solve that?

Respond to myself: we can still use mWorkerPrivate->GetWindow() after the task is dispatched to the main thread.

Please correct me if I'm wrong. Thanks!

Comment 69

[Gene Lian (I already quit Mozilla)]

Full try for patch-1: https://tbpl.mozilla.org/?tree=Try&rev=4229475bb419

Comment 70

[Gene Lian (I already quit Mozilla)]

Hi Ehsan,

I encountered an issue. Maybe you can help me with that. When the test tries to call the following API in the worker thread:

  navigator.getDataStores(...)

It shows the the |navigator| is a type of [object WorkerNavigator] and |navigator.getDataStores| is undefined, which blocks me using the Data Store API from the beginning even if I want to proxy all the logic from the main thread to the worker thread.

That is, it won't go into the function defined in the Navigator.cpp:

  already_AddRefed<Promise>
  Navigator::GetDataStores(const nsAString& aName, ErrorResult& aRv)
  {
    ...
  }

It seems that *WorkerNavigator* is different from the normal Navigator. Do you know how to enable the getDataStores(...) under the WorkerNavigator? Or could you please introduce me to someone for help? Thanks a lot!

Comment 71

[Andrea Marchesini [:baku]]

> Please correct me if I'm wrong. Thanks!

    // Walk up to our containing page
    WorkerPrivate* wp = mWorkerPrivate;
    while (wp->GetParent()) {
      wp = wp->GetParent();
    }

    nsPIDOMWindow* window = wp->GetWindow();

Comment 72

[Andrea Marchesini [:baku]]

> It seems that *WorkerNavigator* is different from the normal Navigator. Do
> you know how to enable the getDataStores(...) under the WorkerNavigator? Or
> could you please introduce me to someone for help? Thanks a lot!

you can probably just add:

WorkerNavigator implements NavigatorDataStore;

in WorkerNavigator.webidl and then implement GetdataStores() in dom/workers/Navigator.cpp

Comment 73

[Gene Lian (I already quit Mozilla)]

Awesome! Thanks Andrea! It's very helpful. I'll try that. :)

Comment 74

[Boris Zbarsky [:bzbarsky]]

>    // Walk up to our containing page

We've been doing that in several places.  Can we just add a helper method for it?

Comment 75

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, disptach tasks on the worker to the main thread (WIP)

Fixed comment #45, comment #71 and comment #72.

TODO:

1. Refactorize the common part with URLRunnable.
2. comment #53
3. comment #62
4. comment #74

Comment 76

[(no longer active)]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #63)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #60)
> 
> Thanks for the review!
> 
> > @@ +86,5 @@
> > > +
> > > +  already_AddRefed<DataStoreCursor>
> > > +  Sync(const nsAString& aRevisionId, ErrorResult& aRv);
> > > +
> > > +  IMPL_EVENT_HANDLER(change)
> > 
> > I think you should use EventTargetProxy class that Josh has implemented in
> > bug 916199 in the worker parts of this patch.
> 
> I'll fix this in my part-2 patch which really proxies the tasks from the
> worker to the main thread. In this patch, I'll still keep it as it is. Does
> that sound reasonable to you?

Yes, absolutely!

Comment 77

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases for running workers (WIP)

Full try: https://tbpl.mozilla.org/?tree=Try&rev=9bfcf899c60e

Comment 78

[(no longer active)]

Comment on attachment 8358201 [details] [diff] [review]
Part 1, C++ stubs for DataStore and DataStoreCursor, V6, r=baku

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

r=me with the below fixed.

::: dom/datastore/DataStore.h
@@ +33,5 @@
> +
> +  // WebIDL (internal functions)
> +
> +  static already_AddRefed<DataStore>
> +  Constructor(GlobalObject& aGlobal, ErrorResult& aRv);

General style nits:

1. Please put the return type on the same line as the function name.  The return type only goes on its own line in the function definition.
2. Please wrap the lines on 80 characters, not less.

::: dom/datastore/DataStoreCursor.cpp
@@ +7,5 @@
> +#include "mozilla/dom/DataStoreBinding.h"
> +#include "mozilla/dom/DataStoreImplBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"

This can go away if EnabledForScope is removed.

@@ +8,5 @@
> +#include "mozilla/dom/DataStoreImplBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"
> +#include "nsContentUtils.h"

This is not used.

@@ +9,5 @@
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"
> +#include "nsContentUtils.h"
> +#include "nsPIDOMWindow.h"

This is not used.

@@ +10,5 @@
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"
> +#include "nsContentUtils.h"
> +#include "nsPIDOMWindow.h"
> +#include "AccessCheck.h"

This can go away if EnabledForScope is removed.

::: dom/datastore/DataStoreCursor.h
@@ +4,5 @@
> +
> +#ifndef mozilla_dom_DataStoreCursor_h
> +#define mozilla_dom_DataStoreCursor_h
> +
> +#include "nsWrapperCache.h"

Please remove this as you're not using it.

@@ +7,5 @@
> +
> +#include "nsWrapperCache.h"
> +#include "nsCOMPtr.h"
> +
> +class nsPIDOMWindow;

Please remove this, as you're not using it.

@@ +20,5 @@
> +class DataStore;
> +class GlobalObject;
> +class DataStoreCursorImpl;
> +
> +class DataStoreCursor MOZ_FINAL : public nsISupports

I don't think you need to derive from nsISupports here, as you're not doing anything special in the QueryInterface implementation.  Please drop this, and just add NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING to get an AddRef/Release method.

@@ +24,5 @@
> +class DataStoreCursor MOZ_FINAL : public nsISupports
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(DataStoreCursor)

Hmm, your trace implementation is empty, so you don't need to use a script holder CC macro.  Just use NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS.

@@ +27,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(DataStoreCursor)
> +
> +public:
> +  explicit DataStoreCursor();

explicit is only useful in ctors which take only one non-optional argument.  Please drop this.

Also, you can just remove both the ctor and the dtor here since they're both empty!

@@ +41,5 @@
> +  WrapObject(JSContext *aCx,
> +             JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  static bool
> +  EnabledForScope(JSContext* /* unused */, JS::Handle<JSObject*> aObj);

Hmm, can't you just use the DataStore::EnabledForScope method and drop this one?  They're both doing the same thing...

Comment 79

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, disptach tasks on the worker to the main thread (WIP)

Polished. Still to do:

1. Refactorize the common part with URLRunnable.
2. EventTargetProxy for dispatching the onchange event.
3. comment #53
4. comment #62
5. comment #74

Comment 80

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases for running workers (WIP)

Comment 81

[Gene Lian (I already quit Mozilla)]

Hi Andrea,

I've been facing a difficulty and stucked for a while. I'd like to have your suggestion. Please refer to my part-2 patch (attachment #8359197 [details] [diff] [review]) for how I dispatch the getDataStores(...) to the main thread:

  already_AddRefed<Promise>
  WorkerNavigator::GetDataStores(JSContext* aCx,
                                 const nsAString& aName,
                                 ErrorResult& aRv)
  {
    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
    nsRefPtr<WorkerNavigatorGetDataStoresRunnable> runnable =
      new WorkerNavigatorGetDataStoresRunnable(workerPrivate, aName, aRv);
    runnable->Dispatch(aCx);

    return runnable->mPromise.forget();
  }

Please refer to https://tbpl.mozilla.org/?tree=Try&rev=bd18cef5ba53 to find the issue. It seems that the returned promise has never been resolved even if the following aResolve(...) has already been called:

  obj.retrieveRevisionId(
    function() {
      --callbackPending;
      if (!callbackPending) {
        aResolve(results);
      }
    }
  );

Looking at the test case (Linux Debug), it seems to assert by the last line of WorkerNavigator::GetDataStores(...):

  return runnable->mPromise.forget();

-----
Question 1: I wonder that's because the Promise is created and returned by the main thread, but it's going to be used by the worker thread. Is that the root cause?

Question 2: If yes, do we have any possible solution in general, when the worker thread wants to access variables that are assigned by the main thread? May I have your hints?

Looking forward to your feedback. Thanks!

Comment 82

[Andrea Marchesini [:baku]]

> Question 1: I wonder that's because the Promise is created and returned by
> the main thread, but it's going to be used by the worker thread. Is that the
> root cause?

Yep. I guess this is the issue: Promise uses "heavily" the event loop.
when it's created on the main thread, the callback is dispatched to the main-thread event loop.
Plus, Promise are not thread-safed (as far as I remember).

What I would like to do is:

1. Create a PromiseNativeHandler and call promise->AppendNativeHandler(). This runs on the main-thread.
2. On the worker thread, create a new Promise,attach it to the PromiseNativeHandler and return it.
3. when the PromiseNativeHandler on main-thread is resolved, jump back to the worker-thread; create new DataStores (in workers) and resolve/reject the new Promise.

> Question 2: If yes, do we have any possible solution in general, when the
> worker thread wants to access variables that are assigned by the main
> thread? May I have your hints?

The runnables that you used are good for this. Check how URL.cpp works.

Comment 83

[Gene Lian (I already quit Mozilla)]

Thanks Andrea for the feedback!

(In reply to Andrea Marchesini (:baku) from comment #82)
> > Question 1: I wonder that's because the Promise is created and returned by
> > the main thread, but it's going to be used by the worker thread. Is that the
> > root cause?
> 
> Yep. I guess this is the issue: Promise uses "heavily" the event loop.
> when it's created on the main thread, the callback is dispatched to the
> main-thread event loop.
> Plus, Promise are not thread-safed (as far as I remember).
> 
> What I would like to do is:
> 
> 1. Create a PromiseNativeHandler and call promise->AppendNativeHandler().
> This runs on the main-thread.
> 2. On the worker thread, create a new Promise,attach it to the
> PromiseNativeHandler and return it.
> 3. when the PromiseNativeHandler on main-thread is resolved, jump back to the worker-thread;
                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is something I didn't understand. How to do it in practice? Does that mean I need to create another runnable carrying the returned results and dispatch it to the worker thread?

> create new DataStores (in workers) and resolve/reject the new Promise.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

My understanding is since the DataStores have been created by the main thread, so does "create new DataStores (in workers)" you mentioned here mean calling DataStore::WrapObject() to wrap the DataStores on the worker?

Comment 84

[Josh Matthews [:jdm]]

No, you can't wrap JS values between runtimes. What it means in practice is that if you have a payload on the main thread, you need to dispatch a runnable to the worker thread that can recreate that payload from scratch. This will involve structured clones if it's an arbitrary JS value, otherwise strings and numeric values can be sent with relative ease.

Comment 85

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2.X, provide a proxy to resolve/reject Promise on workers (WIP)

Hi Andrea,

I worked out a quick patch to show what I'm planning to do based on comment #82. We can apply this class to all the APIs that need to return Promise on workers.

Does that sound reasonable to you? Thanks a lot for you feedback again.

Comment 86

[Andrea Marchesini [:baku]]

Comment on attachment 8359682 [details] [diff] [review]
Part 2.X, provide a proxy to resolve/reject Promise on workers (WIP)

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

yep. This is what I meant. well done. The missing steps are:

1. when the promise is resolved/rejected, you have to go back to the worker-thread (with a runnable).
2. there you have  use the DataStore Worker implementation (probably)

::: dom/datastore/DataStoreWorkerUtils.h
@@ +77,5 @@
>    MainThreadRun() = 0;
>  };
>  
> +class GetDataStoresPromiseProxy : public PromiseNativeHandler
> +{

I think this class should have a thread-safe refcounting.

@@ +84,5 @@
> +
> +public:
> +  GetDataStoresPromiseProxy(Promise* aWorkerPromise)
> +    : mWorkerPromise(aWorkerPromise)
> +  {}

Just to be 100% sure, add a MOZ_ASSERT checking that this is created on the worker thread.

@@ +89,5 @@
> +
> +protected:
> +  virtual void
> +  ResolvedCallback(JS::Handle<JS::Value> aValue)
> +  {

here you are on the main-thread. Jump back to the worker thread and then call mWorkerPromise->MaybeResolve().
And add MOZ_ASSERT(NS_IsMainThread())

@@ +115,5 @@
> +  }
> +
> +  virtual void
> +  RejectedCallback(JS::Handle<JS::Value> aValue)
> +  {

ditto.

@@ +157,5 @@
>      : BaseRunnable(aWorkerPrivate)
>      , mName(aName)
>      , mRv(aRv)
> +  {
> +    mPromseProxy = new GetDataStoresPromiseProxy(aWorkerPromise);

s/Promse/Promise/g

::: dom/workers/Navigator.cpp
@@ +50,5 @@
>    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
> +  WorkerPrivate* wp = workerPrivate;
> +  while (wp->GetParent()) {
> +    wp = wp->GetParent();
> +  }

Promise can be created in the worker thread. In this case, the window param is null.
Just do:

nsRefPtr<Promise> promise = new Promise(nullptr);

Comment 87

[Andrea Marchesini [:baku]]

I guess the full work-flow should be something like this:

1. WorkerNavigator::GetDataStores() creates a runnable and dispatch it to the main-thread. A Promise (A) is created and returned. - you already have done this.

2. the runnable calls Navigator::GetDataStores() and it "attaches" a PromiseNativeHandler to the new promise (B). - already done.

3. when the promise (B) is resolved/rejected, PromiseNativeHandler is called on the main-thread. It creates the list of DataStores and creates a runnable for the worker thread.

4. the runnable is dispatch on the worker thread and it creates a new list of DataStores (on the worker thread) for each the main-thread DataStore. So I assume we will have a WorkerDataStore that has a mDataStore pointing to the main-thread DataStore.

5. These WorkerDataStores use runnables to call methods on mDataStore on the main thread. - you have something similar in patch 2.

6. Any Promise return value should use the same approach of this workflow.

tell me if this makes sense.

Comment 88

[Gene Lian (I already quit Mozilla)]

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

Thanks Andrea for the advice! This really helps a lot.

> 4. the runnable is dispatch on the worker thread and it creates a new list
> of DataStores (on the worker thread) for each the main-thread DataStore. So
> I assume we will have a WorkerDataStore that has a mDataStore pointing to
> the main-thread DataStore.

To support a WorkerDataStore. I'd like to add |'workers': True| in the bindings.conf to generate:

  dom/DataStore
  dom/workers/DataStore

which plays the same trick like URL.

Comment 89

[Gene Lian (I already quit Mozilla)]

Pseudo codes will be:

workers::DataStore holds base::DataStore:
==================================================

class workers::DataStore {
public:
  workers::DataStore(base::DataStore* aDataStore)
    : mDataStore(aDataStore)
  {}

  already_AddRefed<Promise>
  Foo(JSContext* aCx) {
     WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);

     nsRefPtr<Promise> promise = new Promise(window);

     nsRefPtr<DataStoreFooRunnable> runnable =
       new DataStoreFooRunnable(workerPrivate, promise);
     runnable->Dispatch(aCx);

     return promise.forget();
  }

private:
  nsRefPtr<base::DataStore> mDataStore;
}

class base::DataStore {
public:
  Foo() {
    MOZ_ASSERT(NS_IsMainThread());
    return mStore->Foo();
  }

private:
  nsRefPtr<DataStoreImpl> mStore;
}


Dispatch API tasks from the worker to the main thread:
==================================================

class DataStoreFooRunnable {
public:
  DataStoreFooRunnable(WorkerPrivate* aWorkerPrivate,
                       Promise* aWorkerPromise) {
    mPromseProxy = new PromiseProxy(aWorkerPrivate,
                                    aWorkerPromise);
  }

  virtual void
  MainThreadRun() {
    // WorkerPrivate* wp = mWorkerPrivate;
    while (wp->GetParent()) {
      wp = wp->GetParent();
    }
    nsPIDOMWindow* window = wp->GetWindow();

    // Get the naviagor of the top window of the worker.
    nsCOMPtr<nsIDOMNavigator> navigator;
    window->GetNavigator(getter_AddRefs(navigator));

    nsRefPtr<Promise> promise = navigator->Foo();

    promise->AppendNativeHandler(mPromseProxy);
  }
}


A proxy to catch the resolved Promise from the main thread:
==================================================

class PromiseProxy : public PromiseNativeHandler
{
public:
  PromiseProxy(WorkerPrivate *aWorkerPrivate,
               Promise* aWorkerPromise)
    : mWorkerPrivate(aWorkerPrivate)
    , mWorkerPromise(aWorkerPromise)
  {}

protected:
  virtual void
  ResolvedCallback(JS::Handle<JS::Value> aValue)
  {
    nsRefPtr<PromiseWorkerRunnable> workerRunnable =
      new PromiseWorkerRunnable(mWorkerPrivate, mWorkerPromise, aValue);

    mWorkerPrivate->Dispatch(workerRunnable);
  }

private:
  nsRefPtr<WorkerPrivate> mWorkerPrivate;
  nsRefPtr<Promise> mWorkerPromise;
};


A WorkerRunnable to solve the Promise on the worker thread:
==================================================

class PromiseWorkerRunnable : public WorkerRunnable
{
public:
  PromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
                        Promise* aWorkerPromise,
                        JS::Handle<JS::Value> aValue)
    : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
    , mWorkerPromise(aWorkerPromise)
    , mValue(aValue)
  {}

  bool
  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
  {
    // Need to recreate new JS::Value on the worker based on the |mValue|
    // which is originally created by the main thread.
    JS::Value newValue = XXX(mValue);

    mWorkerPromise->MaybeResolve(aCx, newValue);
    return true;
  }

private:
  nsRefPtr<Promise> mWorkerPromise;
  JS::Heap<JS::Value> mValue;
};

Comment 90

[Gene Lian (I already quit Mozilla)]

> A WorkerRunnable to solve the Promise on the worker thread:
> ==================================================
> 
> class PromiseWorkerRunnable : public WorkerRunnable
> {
> public:
>   PromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
>                         Promise* aWorkerPromise,
>                         JS::Handle<JS::Value> aValue)
>     : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
>     , mWorkerPromise(aWorkerPromise)
>     , mValue(aValue)
>   {}
> 
>   bool
>   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
>   {
>     // Need to recreate new JS::Value on the worker based on the |mValue|
>     // which is originally created by the main thread.
>     JS::Value newValue = XXX(mValue);

Questions 1: we need to do this for:

interface DataStore : EventTarget {
  // Promise<any>
  [Throws]
  Promise get(DataStoreKey... id);

  // Promise<DataStoreKey>
  [Throws]
  Promise add(any obj, optional DataStoreKey id);
};

interface DataStoreCursor {
  // Promise<DataStoreTask>
  [Throws]
  Promise next();
};

But NOT for some primitive types or void:

interface DataStore : EventTarget {
  // Promise<void>
  [Throws]
  Promise put(any obj, DataStoreKey id);

  // Promise<boolean>
  [Throws]
  Promise remove(DataStoreKey id);

  // Promise<void>
  [Throws]
  Promise clear();

  // Promise<unsigned long>
  [Throws]
  Promise getLength();
};

Correct?

> 
>     mWorkerPromise->MaybeResolve(aCx, newValue);
>     return true;
>   }
> 
> private:
>   nsRefPtr<Promise> mWorkerPromise;
>   JS::Heap<JS::Value> mValue;
> };

Comment 91

[Gene Lian (I already quit Mozilla)]

Question 2: for this one:

interface DataStore : EventTarget {
  [NewObject, Throws]
  DataStoreCursor sync(optional DOMString revisionId = "");
}

We don't need to care about the Promise. But we still need to do the similar thing like WorkerNavigator::getDataStores():

Dispatch API tasks from the worker to the main thread:
==================================================

class DataStoreSyncRunnable {
public:
  DataStoreSyncRunnable(WorkerPrivate* aWorkerPrivate)
    : mWorkerPrivate(aWorkerPrivate)
  {}

  virtual void
  MainThreadRun() {
    // WorkerPrivate* wp = mWorkerPrivate;
    while (wp->GetParent()) {
      wp = wp->GetParent();
    }
    nsPIDOMWindow* window = wp->GetWindow();

    // Get the naviagor of the top window of the worker.
    nsCOMPtr<nsIDOMNavigator> navigator;
    window->GetNavigator(getter_AddRefs(navigator));

    nsRefPtr<base::DataStoreCursor> cursor = navigator->Sync();
    mCursor = new worker::DataStoreCursor(cursor);
  }

private:
  nsRefPtr<worker::DataStoreCursor> mCursor;
}


and then:

already_AddRefed<DataStoreCursor>
worker::DataStore::Sync(JSContext* aCx, const nsAString& aRevisionId, ErrorResult& aRv)
{
  WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
  nsRefPtr<DataStoreSyncRunnable> runnable =
    new DataStoreSyncRunnable(workerPrivate, aRevisionId, aRv);
  runnable->Dispatch(aCx);

  return runnable->mCursor.forget();
}


That is, we need to |new worker::DataStoreCursor()| for the worker. Right?

If yes, do we need to do this by dispatching the task to the worker thread?

Comment 92

[Gene Lian (I already quit Mozilla)]

Could you please clarify me for comment #90 and comment #91? Thanks!

Comment 93

[Gene Lian (I already quit Mozilla)]

Respond to myself: don't need to find the navigator of the top window of the worker to call APIs, because we already had worker::DataStore pointing to base::DataStore to call the APIs. We only need to do this for WorkerNavigator::getDataStores(). Please correct me if I'm wrong. Thanks!

Comment 94

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.1, r=baku,ehsan

Fixed comment #78, except for:

1. I'd prefer line up all the function parameters if it exceeds 80 chars (this convention is also encouraged by some reviewers).

2. After some thoughts, I'd like to keep a ref to the owner window in DataStoreCursor, which is going to be useful in my later patches, so I'd have:

  explicit DataStore(nsPIDOMWindow* aWindow);
  virtual ~DataStore();

  and 

  nsPIDOMWindow* GetOwner() const { return mOwnerWindow; }

Please let me know if you disagree. The rest like removing nsISupports or using native class CC is all fixed. Thanks!

-----
Full try: https://tbpl.mozilla.org/?tree=Try&rev=1d78dec39e16

Comment 95

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, disptach tasks on the worker to the main thread (WIP)

Use |'workers': True| in bindings.conf to separate:

  DataStore and workers::DataStore
  DataStoreCursor and workers::DataStoreCursor

TODO:

1. Refactorize the common part with URLRunnable.
2. EventTargetProxy for dispatching the onchange event.
3. comment #53
4. comment #62
5. comment #74
6. comment #89

-----
Full try: https://tbpl.mozilla.org/?tree=Try&rev=837c927ff0d1

Comment 96

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, provide a proxy to resolve/reject Promise on workers

Comment 97

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, provide a proxy to resolve/reject Promise on workers, V1.1

Polished the alignment.

Comment 98

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8361601 [details] [diff] [review]
Part 2-1, provide a proxy to resolve/reject Promise on workers, V1.1

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

::: dom/promise/PromiseWorkerProxy.h
@@ +31,5 @@
> +
> +  virtual ~PromiseWorkerProxy() {}
> +
> +protected:
> +  virtual void ResolvedCallback(JS::Handle<JS::Value> aValue);  

Self-review: extra spaces.

@@ +38,5 @@
> +
> +public:
> +  virtual JS::Value CreateValueOnWorker(JSContext* aCx,
> +                    workers::WorkerPrivate* aWorkerPrivate,
> +                    JS::Handle<JS::Value> aValue) = 0;

Self-review: need to align params.

Comment 99

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-2, refactorize SyncWorkerRunnable from workers/URL.cpp

This is actually stolen from Bug 916199 by Josh.

Comment 100

[Andrea Marchesini [:baku]]

Comment on attachment 8361601 [details] [diff] [review]
Part 2-1, provide a proxy to resolve/reject Promise on workers, V1.1

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

r- because I want to see the structured clone algorithm fully implemented.

::: dom/promise/Promise.cpp
@@ +665,5 @@
> +                                       Promise* aWorkerPromise)
> +  : mWorkerPrivate(aWorkerPrivate)
> +  , mWorkerPromise(aWorkerPromise)
> +{
> +  MOZ_ASSERT(aWorkerPrivate->AssertIsOnWorkerThread());

The ordering is wrong:

MOZ_ASSERT(mWorkerPromise);
MOZ_ASSERT(mWorkerPromise->AssertIsOnWorkerThread());
MOZ_ASSERT(mPromiseWorkerProxy);

@@ +674,5 @@
> +void
> +PromiseWorkerProxy::ResolvedCallback(JS::Handle<JS::Value> aValue)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mWorkerPrivate);

remove this.

@@ +690,5 @@
> +void
> +PromiseWorkerProxy::RejectedCallback(JS::Handle<JS::Value> aValue)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mWorkerPrivate);

remove this.

@@ +724,5 @@
> +  MOZ_ASSERT(mPromiseWorkerProxy);
> +  MOZ_ASSERT(mWorkerPromise);
> +
> +  JS::Value value =
> +    mPromiseWorkerProxy->CreateValueOnWorker(aCx, aWorkerPrivate,

You don't need this. You can use the structured clone algorithm. On the main-thread do this:

    if (!mValue.write(cx, aValue, nullptr, nullptr)) {
      JS_ClearPendingException(cx);
      return something...;
    }

where mValue is: JSAutoStructuredCloneBuffer mValue;

then, on the worker thread, just do:

    JS::Rooted<JS::Value> value(cx);
    if (!mValue.read(cx, &value, nullptr, nullptr)) {
      return something..;
    }

::: dom/promise/PromiseWorkerProxy.h
@@ +36,5 @@
> +
> +  virtual void RejectedCallback(JS::Handle<JS::Value> aValue);
> +
> +public:
> +  virtual JS::Value CreateValueOnWorker(JSContext* aCx,

You don't need this.

Comment 101

[Andrea Marchesini [:baku]]

Comment on attachment 8361609 [details] [diff] [review]
Part 2-2, refactorize SyncWorkerRunnable from workers/URL.cpp

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

bent, I think you should take a look.

Comment 102

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, disptach tasks on the worker to the main thread (WIP)

This patch is still implementing CreateValueOnWorker(...). Will try Andrea's solution next week.

TODO:

1. EventTargetProxy for dispatching the onchange event.
2. comment #53
3. comment #74
4. comment #100

Comment 103

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

Comment on attachment 8361601 [details] [diff] [review]
Part 2-1, provide a proxy to resolve/reject Promise on workers, V1.1

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

::: dom/promise/PromiseWorkerProxy.h
@@ +41,5 @@
> +                    workers::WorkerPrivate* aWorkerPrivate,
> +                    JS::Handle<JS::Value> aValue) = 0;
> +
> +private:
> +  nsRefPtr<workers::WorkerPrivate> mWorkerPrivate;

I don't see a need to hold a RefPtr in this use case, based on:
1) Since the worker proxy is used through a SyncRunnable, if the WorkerPrivate was alive when this runnable was dispatched, it will stay alive.
2) You create a subclass of this on the worker thread, then use this in the main thread and trigger all sorts of assertions about AddRef/Release on th wrong thread.
3) Keeping WorkerPrivate's alive like this is generally a bad idea outside of the RuntimeService and a few other things. You want to keep it alive (which due to (1) you don't have to worry about), you'll want to use the AddFeature() system.

@@ +42,5 @@
> +                    JS::Handle<JS::Value> aValue) = 0;
> +
> +private:
> +  nsRefPtr<workers::WorkerPrivate> mWorkerPrivate;
> +  nsRefPtr<Promise> mWorkerPromise;

This class needs to be cycle collected since it holds on to Promise and is strongly held by Promises that are visible to JS.
But since it is using threadsafe refcounting, you will have to break the cycles manually. Best to discuss with smaug/mccr8/khuey.

On further reading, this is also being touched on both worker thread and main thread. I have a feeling that due to SyncRunnable this can also be held as just a weakref (with the JS wrapper holding the Promise alive), but I wouldn't bet on it. You may want to use nsWorkerThreadPtrHolder (Bug 947499).

@@ +48,5 @@
> +
> +
> +// A WorkerRunnable to resolve/reject the Promise on the worker thread.
> +
> +class PromiseWorkerRunnable : public workers::WorkerRunnable

If this class isn't meant to be used by anyone other than PromiseWorkerProxy, please move it to Promise.cpp.
It also seems like this class is redundant if you switch to structured clone. Just use WorkerPromiseResolverTask. (you may have to remove the owning thread assertions.)

Comment 104

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

Comment on attachment 8361634 [details] [diff] [review]
Part 2-3, disptach tasks on the worker to the main thread (WIP)

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

Fly by comments.

::: dom/workers/DataStoreWorkerUtils.h
@@ +79,5 @@
> +    WorkerPrivate* wp = mWorkerPrivate;
> +    while (wp->GetParent()) {
> +      wp = wp->GetParent();
> +    }
> +    nsPIDOMWindow* window = wp->GetWindow();

SharedWorkers have a null window. You should use the principal/origin to get the right datastore. See comment below.

@@ +89,5 @@
> +      return;
> +    }
> +
> +    nsRefPtr<Promise> promise = static_cast<Navigator*>
> +      (navigator.get())->GetDataStores(window, mName, mRv);

Refactor this to not call navigator's GetDataStores. I guess since DataStore is JS implemented, you'll have to do some stubbing here.

@@ +91,5 @@
> +
> +    nsRefPtr<Promise> promise = static_cast<Navigator*>
> +      (navigator.get())->GetDataStores(window, mName, mRv);
> +
> +    promise->AppendNativeHandler(mPromiseWorkerProxy);

mPromiseWorkerProxy will be addrefed on the wrong (main) thread.

Comment 105

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Just FYI, worker code makes *heavy* use of assertions, so please make sure that you're testing with debug builds!

Comment 106

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8361609 [details] [diff] [review]
Part 2-2, refactorize SyncWorkerRunnable from workers/URL.cpp

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

Don't add a new header for this.  Just stick it in WorkerRunnable.h.  Also, it needs a better name.  We already have a WorkerSyncRunnable, adding SyncWorkerRunnable is very confusing.

Comment 107

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, provide a proxy to resolve/reject Promise on workers, V1.2

Thanks Andrea and Nikhil for the feedback. Fixed comment #100 and comment #103.

Comment 108

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, provide a proxy to resolve/reject Promise on workers, V2

Fixed the last comment at comment #104. PromiseWorkerProxy is only allowed being crated on the main thread.

Comment 109

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-2, refactor WorkerMainThreadRunnable, V2, r=baku

Fixed comment #106. WorkerMainThreadRunnable is the best naming I could think of.

Comment 110

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, disptach tasks on the worker to the main thread (WIP)

Hi Andrea,

Could you please take a look to see if I'm in the right direction? Thanks!

Comment 111

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8362473 [details] [diff] [review]
Part 2-3, disptach tasks on the worker to the main thread (WIP)

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

::: dom/workers/DataStore.cpp
@@ +256,5 @@
> +
> +void
> +DataStore::SetDataStore(mozilla::dom::DataStore* aStore)
> +{
> +  mStore = aStore;

Self review:

aStore is created on the main thread. If on the worker thread we use nsRefPtr<mozilla::dom::DataStore> mStore to catch the aStore, AddRef() will cause error according to Nikhil's comments. I'm wondering we need to dispatch this task on the worker thread.

However, this would make the whole process across threads quite often:

1. Get dom::DataStore on the main thread (by DataStoreCursorGetStoreRunnable).
2. Create workers::DataStore on the worker thread (by CreateDataStoreWorkerRunnable).
3. Set workers::DataStore point to dom::DataStore on the main thread (here).

Any better thoughts? Need advice.

::: dom/workers/DataStoreCursor.cpp
@@ +95,5 @@
> +
> +void
> +DataStoreCursor::SetDataStoreCursor(mozilla::dom::DataStoreCursor* aCursor)
> +{
> +  mCursor = aCursor;

Ditto.

::: dom/workers/DataStoreWorkerUtils.h
@@ +87,5 @@
> +// mozilla::dom::DataStore.
> +class DataStoreRunnable : public WorkerMainThreadRunnable
> +{
> +protected:
> +  nsRefPtr<mozilla::dom::DataStore> mStore;

Self review:

I think we shouldn't use nsRefPtr. Because mozilla::dom::DataStore is created on the main thread and DataStoreRunnable (or its derived class) is created on the worker thread, assign DataStoreRunnable to nsRefPtr will cause AddRef() error. Please correct me if I'm wrong. Thanks!

@@ +106,5 @@
> +// mozilla::dom::DataStoreCursor.
> +class DataStoreCursorRunnable : public WorkerMainThreadRunnable
> +{
> +protected:
> +  nsRefPtr<mozilla::dom::DataStoreCursor> mCursor;

Ditto.

Comment 112

[Gene Lian (I already quit Mozilla)]

> aStore is created on the main thread. If on the worker thread we use
> nsRefPtr<mozilla::dom::DataStore> mStore to catch the aStore, AddRef() will
> cause error according to Nikhil's comments. I'm wondering we need to
> dispatch this task on the worker thread.
                            ^^^^^^^^^^^^^ main thread

Comment 113

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8362473 [details] [diff] [review]
Part 2-3, disptach tasks on the worker to the main thread (WIP)

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

::: dom/workers/DataStore.cpp
@@ +256,5 @@
> +
> +void
> +DataStore::SetDataStore(mozilla::dom::DataStore* aStore)
> +{
> +  mStore = aStore;

After some thoughts, I think we can new a worker::DataStore and pass it into DataStoreCursorGetStoreRunnable. Then, directly set the dom::DataStore in the DataStoreCursorGetStoreRunnable::MainThreadRun(), so that we don't need to cross worker/main threads back and forth.

already_AddRefed<DataStore>
DataStoreCursor::GetStore(JSContext* aCx, ErrorResult& aRv)
{
  MOZ_ASSERT(mCursor);

  nsRefPtr<worker::DataStore> store = new worker::DataStore();

  WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
  nsRefPtr<DataStoreCursorGetStoreRunnable> runnable =
    new DataStoreCursorGetStoreRunnable(workerPrivate, mCursor, store, aRv);
  runnable->Dispatch(aCx);

  return store.forget();
}

Comment 114

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

Comment on attachment 8362417 [details] [diff] [review]
Part 2-1, provide a proxy to resolve/reject Promise on workers, V2

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

Please add a comment to PromiseWorkerProxy stating it should only be used via a SyncRunnable

Comment 115

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, disptach tasks on the worker to the main thread (WIP)

TODO:

1. Need a way to point workers::DataStore to dom::DataStore, because PromiseWorkerProxy simply uses structured clone.
2. EventTargetProxy for dispatching the onchange event.
3. Properly handle shared worker in addition to normal worker.
4. comment #53 (minor)
5. comment #74 (minor)

Comment 116

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8362473 [details] [diff] [review]
Part 2-3, disptach tasks on the worker to the main thread (WIP)

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

This is not intended to be a comprehensive review.

I think you already realize this, but one thing that sticks out to me is that you need to be more careful about managing the lifetimes of objects living on different threads.  You should also keep in mind that the destructor of a runnable that is AddRef()ed and Release()ed on multiple threads can run on either thread.  There are essentially three types of objects here: 1) those that live solely on the worker thread, 2) those that live solely on the main thread, and 3) those that live on both.  #1 and #2 need to ensure that they obtain objects living on the other thread without AddRef()ing them on their thread (e.g. through sticking them in an nsCOMPtr on the other thread and then using .forget() and already_AddRefed<T> to transfer them) and that they send these objects back to the other thread to be released (e.g. via NS_ProxyRelease).  #3 needs to make sure it releases the objects on the correct thread, since the destructor can run on either thread.  This might also require NS_ProxyRelease, or explicitly clearing nsCOMPtrs in a Run() function.

::: dom/workers/DataStore.cpp
@@ +6,5 @@
> +#include "mozilla/dom/DataStore.h"
> +#include "mozilla/dom/DataStoreBinding.h"
> +#include "mozilla/dom/DataStoreImplBinding.h"
> +#include "mozilla/dom/workers/bindings/DataStore.h"
> +#include "mozilla/dom/workers/bindings/DataStoreCursor.h"

Just DataStore.h and DataStoreCursor.h.  You're in the same directory; you don't need fully qualified paths.

@@ +11,5 @@
> +
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/Preferences.h"
> +#include "AccessCheck.h"

Why do you need Preferences.h and AccessCheck.h?

@@ +54,5 @@
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
> +  if (!window) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }

This does not work at all.  The global object on a worker thread is not an nsPIDOMWindow, it is http://hg.mozilla.org/mozilla-central/annotate/253b45002d73/dom/workers/WorkerScope.h#l26.

::: dom/workers/DataStore.h
@@ +27,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(DataStore,
> +                                                         nsDOMEventTargetHelper)

You don't use the Trace hook, so this does not need to have SCRIPT_HOLDER.

@@ +28,5 @@
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(DataStore,
> +                                                         nsDOMEventTargetHelper)
> +  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)

You are not inheriting from nsIDOMEventTarget again, so I don't think you need this either.

@@ +32,5 @@
> +  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)
> +
> +public:
> +  explicit DataStore(nsPIDOMWindow* aWindow);
> +  virtual ~DataStore();

refcounted objects should have private or protected dtors.

@@ +83,5 @@
> +
> +  // This internal function (ChromeOnly) is aimed to make the DataStore keep a
> +  // reference to the DataStoreImpl which really implements the API's logic in
> +  // JS. We also need to let the DataStoreImpl implementation keep the event
> +  // taget of DataStore, so that it can know where to fire the events.

*target

This comment is at best misleading.  This is not a ChromeOnly function, because it does not appear in the WebIDL at all.

@@ +89,5 @@
> +
> +  void SetDataStore(mozilla::dom::DataStore* aStore);
> +
> +private:
> +  nsRefPtr<mozilla::dom::DataStore> mStore;

This needs a comment about it living on the main thread.  That also means that you need to

::: dom/workers/DataStoreWorkerUtils.h
@@ +22,5 @@
> +BEGIN_WORKERS_NAMESPACE
> +
> +// Sub-class inheriting WorkerMainThreadRunnable to run
> +// WorkerNavigator::GetDataStores(...) on the main thread.
> +class NavigatorGetDataStoresRunnable : public WorkerMainThreadRunnable

These runnables should all go in the .cpp file they are used in, assuming there are none that are used in both DataStore.cpp and DataStoreCursor.cpp

@@ +24,5 @@
> +// Sub-class inheriting WorkerMainThreadRunnable to run
> +// WorkerNavigator::GetDataStores(...) on the main thread.
> +class NavigatorGetDataStoresRunnable : public WorkerMainThreadRunnable
> +{
> +private:

class members are private by default, no need for the label

@@ +25,5 @@
> +// WorkerNavigator::GetDataStores(...) on the main thread.
> +class NavigatorGetDataStoresRunnable : public WorkerMainThreadRunnable
> +{
> +private:
> +  nsRefPtr<Promise> mWorkerPromise;

This needs a comment documenting that it is an object from the worker thread.

@@ +26,5 @@
> +class NavigatorGetDataStoresRunnable : public WorkerMainThreadRunnable
> +{
> +private:
> +  nsRefPtr<Promise> mWorkerPromise;
> +  const nsAString& mName;

Our string classes are essentially smart pointers to string buffers, so there's no reason to hold references to them like this.  If this were a const nsAString (no &) it would just increment the refcount on aName's underlying buffer, so we do that instead.

@@ +27,5 @@
> +{
> +private:
> +  nsRefPtr<Promise> mWorkerPromise;
> +  const nsAString& mName;
> +  ErrorResult& mRv;

This is a reference to an object allocated on the stack of another thread.  While I think this is technically ok, because the sync loop will prevent the worker thread from unwinding its stack, I must admit that it scares me a lot ;-)  I sent bent (the module owner) an email asking what he thinks of this.

@@ +40,5 @@
> +    , mName(aName)
> +    , mRv(aRv)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    MOZ_ASSERT(aWorkerPrivate->AssertIsOnWorkerThread());

You don't need these WorkerPrivate assertions, the base class will handle that for you.

@@ +46,5 @@
> +  }
> +
> +protected:
> +  virtual void
> +  MainThreadRun() {

MOZ_OVERRIDE

@@ +47,5 @@
> +
> +protected:
> +  virtual void
> +  MainThreadRun() {
> +    MOZ_ASSERT(NS_IsMainThread());

AssertIsOnMainThread() is what we use in worker code.  That way if we want to make all worker assertions fatal in opt builds it's easy.

@@ +57,5 @@
> +    }
> +    nsPIDOMWindow* window = wp->GetWindow();
> +
> +    // TODO SharedWorker has null window. Need to handle that as well. How?
> +    if (!window) {

It's not just SharedWorker, workers created from a JS component or JSM will have a null window pointer too.  It's not clear to me that this is something you need to worry about at this point.

@@ +87,5 @@
> +// mozilla::dom::DataStore.
> +class DataStoreRunnable : public WorkerMainThreadRunnable
> +{
> +protected:
> +  nsRefPtr<mozilla::dom::DataStore> mStore;

Yes, that's correct.  This will cause a threadsafety assertion when you try to AddRef the object in the constructor.  What you should do instead is ensure that the object that holds a reference to the DataStore is still alive.  That is, instead of establishing a reference chain that looks like:

DataStoreRunnable -> mStore

You can do

DataStoreRunnable -> WorkerDataStore -> DataStore

subject to the caveats about releasing things on the right thread that I mentioned before.

@@ +279,5 @@
> +class DataStorePutRunnable : public DataStoreRunnable
> +{
> +private:
> +  nsRefPtr<Promise> mWorkerPromise;
> +  JS::Heap<JS::Value> mObj;

This definitely is not ok.  You can't use JS objects across threads like this.  Once we have a moving GC this object could get moved during the middle of the Put() call.  This needs to be structured cloned while you're still on the worker thread.

@@ +325,5 @@
> +class DataStoreAddRunnable : public DataStoreRunnable
> +{
> +private:
> +  nsRefPtr<Promise> mWorkerPromise;
> +  JS::Heap<JS::Value> mObj;

and here

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

no indenting

::: dom/workers/moz.build
@@ +63,5 @@
>      '../base',
>      '../system',
>      '/content/base/src',
>      '/content/events/src',
> +    '/js/xpconnect/wrappers',

You shouldn't need this.

Comment 117

[Gene Lian (I already quit Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #116)

Thanks Kyle! These comments are really helpful! Just having a couple of questions/concerns as below:

> Comment on attachment 8362473 [details] [diff] [review]
> Part 2-3, disptach tasks on the worker to the main thread (WIP)
> 
> Review of attachment 8362473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is not intended to be a comprehensive review.
> 
> I think you already realize this, but one thing that sticks out to me is
> that you need to be more careful about managing the lifetimes of objects
> living on different threads.  You should also keep in mind that the
> destructor of a runnable that is AddRef()ed and Release()ed on multiple
> threads can run on either thread.  There are essentially three types of
> objects here: 1) those that live solely on the worker thread, 2) those that
> live solely on the main thread, and 3) those that live on both.  #1 and #2
> need to ensure that they obtain objects living on the other thread without
> AddRef()ing them on their thread (e.g. through sticking them in an nsCOMPtr
> on the other thread and then using .forget() and already_AddRefed<T> to
> transfer them) and that they send these objects back to the other thread to
> be released (e.g. via NS_ProxyRelease).  #3 needs to make sure it releases
> the objects on the correct thread, since the destructor can run on either
> thread.  This might also require NS_ProxyRelease, or explicitly clearing
> nsCOMPtrs in a Run() function.

For #3, can we just use raw pointers in runnable/workerRunnable to keep objects (if we're sure some other class will keep them alive for sure via nsRefPtr), so that we don't need to worry too much about the AddRef()/Release() issues?

> 
> @@ +32,5 @@
> > +  NS_REALLY_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper)
> > +
> > +public:
> > +  explicit DataStore(nsPIDOMWindow* aWindow);
> > +  virtual ~DataStore();
> 
> refcounted objects should have private or protected dtors.

Wow! Thanks!

> @@ +83,5 @@
> > +
> > +  // This internal function (ChromeOnly) is aimed to make the DataStore keep a
> > +  // reference to the DataStoreImpl which really implements the API's logic in
> > +  // JS. We also need to let the DataStoreImpl implementation keep the event
> > +  // taget of DataStore, so that it can know where to fire the events.
> 
> *target
> 
> This comment is at best misleading.  This is not a ChromeOnly function,
> because it does not appear in the WebIDL at all.

Actually, this is designed for the part-1 patch.

> @@ +57,5 @@
> > +    }
> > +    nsPIDOMWindow* window = wp->GetWindow();
> > +
> > +    // TODO SharedWorker has null window. Need to handle that as well. How?
> > +    if (!window) {
> 
> It's not just SharedWorker, workers created from a JS component or JSM will
> have a null window pointer too.  It's not clear to me that this is something
> you need to worry about at this point.

+1. Will fire a follow-up for that.

> 
> @@ +87,5 @@
> > +// mozilla::dom::DataStore.
> > +class DataStoreRunnable : public WorkerMainThreadRunnable
> > +{
> > +protected:
> > +  nsRefPtr<mozilla::dom::DataStore> mStore;
> 
> Yes, that's correct.  This will cause a threadsafety assertion when you try
> to AddRef the object in the constructor.  What you should do instead is
> ensure that the object that holds a reference to the DataStore is still
> alive.  That is, instead of establishing a reference chain that looks like:

As mentioned above, may I just use raw pointer to keep that? For example,

  class DataStoreRunnable : public WorkerMainThreadRunnable {
  protected:
    mozilla::dom::DataStore* mStore;
  }

since what I want to do is just use it to call mStore->API(). WorkerDataStore will keep a strong ref to DataStore to make sure it's alive.

> 
> DataStoreRunnable -> mStore
> 
> You can do
> 
> DataStoreRunnable -> WorkerDataStore -> DataStore
> 
> subject to the caveats about releasing things on the right thread that I
> mentioned before.
> 
> @@ +279,5 @@
> > +class DataStorePutRunnable : public DataStoreRunnable
> > +{
> > +private:
> > +  nsRefPtr<Promise> mWorkerPromise;
> > +  JS::Heap<JS::Value> mObj;
> 
> This definitely is not ok.  You can't use JS objects across threads like
> this.  Once we have a moving GC this object could get moved during the
> middle of the Put() call.  This needs to be structured cloned while you're
> still on the worker thread.

Yeap! I missed this one. Thanks for pointing this out.

Comment 118

[Gene Lian (I already quit Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #116)

One more question:

> @@ +89,5 @@
> > +
> > +  void SetDataStore(mozilla::dom::DataStore* aStore);
> > +
> > +private:
> > +  nsRefPtr<mozilla::dom::DataStore> mStore;
> 
> This needs a comment about it living on the main thread.  That also means
> that you need to

I guess you want to say: you need to use NS_ProxyReleas() to release it on the main thread, since the WorkerDataStore is created and released on the worker thread. Right? So, I should do:

class DataStore
{
  virtual ~DataStore() {
    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
    if (mainThread) {
      NS_ProxyRelease(mainThread, mStore);
    }
  }

  nsRefPtr<mozilla::dom::DataStore> mStore;
}

Is that accurate?

Comment 119

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8362473 [details] [diff] [review]
Part 2-3, disptach tasks on the worker to the main thread (WIP)

Kyle left comments for the obsolete patch when I attached new patch at the same time. Re-obsolete it. The new patch didn't apply Kyle's comments yet. Will do it tomorrow.

Comment 120

[Andrea Marchesini [:baku]]

Comment on attachment 8362773 [details] [diff] [review]
Part 2-3, disptach tasks on the worker to the main thread (WIP)

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

r- just because I want to see the structured clone algorithm for put/add. and the Proxy for the GetDataStores.

::: dom/workers/DataStore.cpp
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

should this include mozilla/dom/worker/DataStore.h ?

@@ +51,5 @@
> +already_AddRefed<DataStore>
> +DataStore::Constructor(GlobalObject& aGlobal, ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
> +  if (!window) {

does it mean that this object is created on the mainthread?
Probably you don't need the window.

@@ +69,5 @@
> +
> +void
> +DataStore::GetName(JSContext* aCx, nsAString& aName, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(mStore);

MOZ_ASSERT(we are in a worker thread) here and in any webidl method.

::: dom/workers/DataStore.h
@@ +36,5 @@
> +  virtual ~DataStore();
> +
> +  // WebIDL (internal functions)
> +
> +  static already_AddRefed<DataStore> Constructor(GlobalObject& aGlobal,

type
methodName(params)

::: dom/workers/DataStoreWorkerUtils.h
@@ +71,5 @@
> +    }
> +
> +    // TODO I'm afraid we cannot use PromiseWorkerProxy here for GetDataStores()
> +    // because we need to re-create workers::DataStore pointing to dom::DataStore.
> +    // PromiseWorkerProxy will simply structuredly clone the value.

Right. You cannot. But you can create something custom for this method.
When you receive the array of DataStores, for each one, you can create a workers::DataStore having it's dom::DataStore object.

@@ +298,5 @@
> +    , mRv(aRv)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    MOZ_ASSERT(aWorkerPrivate->AssertIsOnWorkerThread());
> +    MOZ_ASSERT(aWorkerPromise);

You should use structured clone algorithm for this aObj without storing it in mObj.
Then read using structured clone algorithm when you are on the main thread.
Same stuff for Add.

::: dom/workers/Navigator.h
@@ +104,5 @@
>      mOnline = aOnline;
>    }
> +
> +  already_AddRefed<Promise> GetDataStores(JSContext* aCx,
> +                                          const nsAString &aName,

& next to the type.

Comment 121

[Andrea Marchesini [:baku]]

Comment on attachment 8362417 [details] [diff] [review]
Part 2-1, provide a proxy to resolve/reject Promise on workers, V2

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

::: dom/promise/Promise.cpp
@@ +662,5 @@
>    RunTask();
>  }
>  
> +PromiseWorkerProxy::PromiseWorkerProxy(WorkerPrivate *aWorkerPrivate,
> +                                       Promise* aWorkerPromise)

I would prefer a separated file for this.

@@ +667,5 @@
> +  : mWorkerPrivate(aWorkerPrivate)
> +  , mWorkerPromise(aWorkerPromise)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aWorkerPrivate);

mWorkerPrivate or aWorkerPromise.

@@ +677,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsRefPtr<WorkerPromiseResolverTask> task =
> +    new WorkerPromiseResolverTask(mWorkerPrivate,

where is this class?

::: dom/promise/PromiseWorkerProxy.h
@@ +49,5 @@
> +  virtual void RejectedCallback(JS::Handle<JS::Value> aValue);
> +
> +private:
> +  workers::WorkerPrivate* mWorkerPrivate;
> +  Promise* mWorkerPromise;

nsRefPtr

Comment 122

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #117)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #116)
> 
> Thanks Kyle! These comments are really helpful! Just having a couple of
> questions/concerns as below:
> 
> > Comment on attachment 8362473 [details] [diff] [review]
> > Part 2-3, disptach tasks on the worker to the main thread (WIP)
> > 
> > Review of attachment 8362473 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is not intended to be a comprehensive review.
> > 
> > I think you already realize this, but one thing that sticks out to me is
> > that you need to be more careful about managing the lifetimes of objects
> > living on different threads.  You should also keep in mind that the
> > destructor of a runnable that is AddRef()ed and Release()ed on multiple
> > threads can run on either thread.  There are essentially three types of
> > objects here: 1) those that live solely on the worker thread, 2) those that
> > live solely on the main thread, and 3) those that live on both.  #1 and #2
> > need to ensure that they obtain objects living on the other thread without
> > AddRef()ing them on their thread (e.g. through sticking them in an nsCOMPtr
> > on the other thread and then using .forget() and already_AddRefed<T> to
> > transfer them) and that they send these objects back to the other thread to
> > be released (e.g. via NS_ProxyRelease).  #3 needs to make sure it releases
> > the objects on the correct thread, since the destructor can run on either
> > thread.  This might also require NS_ProxyRelease, or explicitly clearing
> > nsCOMPtrs in a Run() function.
> 
> For #3, can we just use raw pointers in runnable/workerRunnable to keep
> objects (if we're sure some other class will keep them alive for sure via
> nsRefPtr), so that we don't need to worry too much about the
> AddRef()/Release() issues?

Yes, you can, and we do this extensively in the worker code.  But you absolutely have to be sure that the other class keeps them alive long enough.
 
(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #118)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #116)
> 
> One more question:
> 
> > @@ +89,5 @@
> > > +
> > > +  void SetDataStore(mozilla::dom::DataStore* aStore);
> > > +
> > > +private:
> > > +  nsRefPtr<mozilla::dom::DataStore> mStore;
> > 
> > This needs a comment about it living on the main thread.  That also means
> > that you need to
> 
> I guess you want to say: you need to use NS_ProxyReleas() to release it on
> the main thread, since the WorkerDataStore is created and released on the
> worker thread. Right? So, I should do:
> 
> class DataStore
> {
>   virtual ~DataStore() {
>     nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
>     if (mainThread) {
>       NS_ProxyRelease(mainThread, mStore);
>     }
>   }
> 
>   nsRefPtr<mozilla::dom::DataStore> mStore;
> }
> 
> Is that accurate?

Yeah, something like that.  Although you have to clear the nsRefPtr without Release()ing the pointer.  e.g. http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/AsyncConnectionHelper.cpp#98

I think I found my next thing to use r-value references with!

Comment 123

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) (AFK Jan 22/23) from comment #122)
> I think I found my next thing to use r-value references with!

Although apparently there's already an NS_ProxyRelease overload that takes an nsCOMPtr/nsRefPtr, so it's not really needed.

Comment 124

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.2, r=baku,ehsan

Polished this patch that I learnt from Kyle's comments:

1. refcounted objects should have private or protected dtors
2. Don't need NS_REALLY_FORWARD_NSIDOMEVENTTARGET
3. DataStoreCursor doesn't need to keep a window reference

Comment 125

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V3, f=nsm

Hi Andrea,

This patch fixed comment #121, except for:

I'd prefer keeping PromiseWorkerProxy implementation in the Promise.cpp because WorkerPromiseResolverTask and its parent PromiseResolverMixin are all there. Moving it out of Promise.cpp needs a big change. Please let me know if you insist and I'm glad to do that. :)

Also, please see Nikhil's comment #103 for why I used raw pointer for the mWorkerPromise. Besides, since the Promise is created on the worker, we cannot use mWorkerPromise to catch it on the main thread because it will cause AddRef()/Release() errors.

Comment 126

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread (WIP)

Polished some in the the previous comments.

TODO:

1. Need a way to point workers::DataStore to dom::DataStore, because PromiseWorkerProxy simply uses structured clone.
2. Need to use structured clone for the |mObj|.
3. EventTargetProxy for dispatching the onchange event.
4. comment #53 (minor)
5. comment #74 (minor)

Comment 127

[Andrea Marchesini [:baku]]

Comment on attachment 8363639 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V3, f=nsm

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

::: dom/promise/PromiseWorkerProxy.h
@@ +43,5 @@
> +
> +  virtual ~PromiseWorkerProxy() {}
> +
> +protected:
> +  virtual void ResolvedCallback(JS::Handle<JS::Value> aValue);

MOZ_OVERRIDE

@@ +45,5 @@
> +
> +protected:
> +  virtual void ResolvedCallback(JS::Handle<JS::Value> aValue);
> +
> +  virtual void RejectedCallback(JS::Handle<JS::Value> aValue);

MOZ_OVERRIDE

Comment 128

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8363639 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V3, f=nsm

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

::: dom/promise/Promise.cpp
@@ +666,5 @@
> +                                       Promise* aWorkerPromise)
> +  : mWorkerPrivate(aWorkerPrivate)
> +  , mWorkerPromise(aWorkerPromise)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

Still confused about this. I wonder we cannot construct the PromiseWorkerProxy on the main thread, due to below:

@@ +680,5 @@
> +  nsRefPtr<WorkerPromiseResolverTask> task =
> +    new WorkerPromiseResolverTask(mWorkerPrivate,
> +                                  mWorkerPromise,
> +                                  aValue,
> +                                  Promise::PromiseState::Resolved);

Looking into the parent of WorkerPromiseResolverTask, PromiseResolverMixin's constructor still uses nsRefPtr to catch the promise. Does that mean the promise has to be created on the worker thread in the first place?

Also, when we want to use WorkerPromiseResolverTask, it usually means we need to resolve the promise on the work thread, which means we're usually on the main thread right now. But [1] shows we should be on the worker thread. Why?

Hi Nikhil, WorkerPromiseResolverTask/PromiseResolverMixin seems to be written by you. Could you please clarify me?

Also, what's the correct way to use PromiseResolverTask and WorkerPromiseResolverTask in general?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp#674

Comment 129

[Gene Lian (I already quit Mozilla)]

> Looking into the parent of WorkerPromiseResolverTask, PromiseResolverMixin's
> constructor still uses nsRefPtr to catch the promise. Does that mean the
> promise has to be created on the worker thread in the first place?
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Correct: does that mean the assignment has to be happening on the worker, since the promise was created on the worker thread?

Comment 130

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread (WIP)

Comment 131

[Andrea Marchesini [:baku]]

Comment on attachment 8363639 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V3, f=nsm

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

::: dom/promise/Promise.cpp
@@ +675,5 @@
> +void
> +PromiseWorkerProxy::ResolvedCallback(JS::Handle<JS::Value> aValue)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

Here we have to change something. You should use the structured clone algorithm again the aValue and pass the buffer to WorkerPromiseResolverTask.

Comment 132

[Andrea Marchesini [:baku]]

What about this:

class PromiseWorkerProxy : public PromiseNativeHandler {
public:
  PromiseWorkerProxy(workers::WorkerPrivate* aWorkerPrivate, Promise* aWorkerPromise,
                     JSStructuredCloneCallbacks* aCallbacks)
    : mCallbacks(aCallbacks), mData(nullptr) ...

  JSStructuredCloneCallbacks* mCallbacks;
  void* mData;

  virtual void ResolvedCallback(JS::Handle<JS::Value> aValue) MOZ_OVVERIDE {
    ...

    // the value is cloned and written in a buffer.
    JSAutoStructuredCloneBuffer buffer;
    if (!buffer.write(cx, aValue, mCallbacks, mData)) {
       ...
    }

    // This buffer is send to a proxy runnable.
    nsRefPtr<PromiseWorkerProxyRunnable> task =
      new PromiseWorkerProxyRunnable(mWorkerPrivate, mWorkerPromise, buffer, mData,
		Promise::PromiseState::Resolved);
    mWorkerPrivate->Dispatch(task);
  }

Where PromiseWorkerProxyRunnable is something like:

class PromiseWorkerProxyRunnable : public nsRunnable {
public:
  PromiseWorkerProxyRunnable(..., JSStructuredCloneCallbacks* aCallbacks,
                             JSAutoStructuredCloneBuffer& aBuffer, void* aData, ...)
    : mCallbacks(aCallbacks), mBuffer(aBuffer), mData(aData) ...
  {}

  nsresult Run() {
    // Assert we are on a worker thread.

    // Here we convert the buffer to a JSValue.
    JS::Rooted<JS::Value> value(cx);
    if (!buffer.read(cx, &value, mCallbacks, mData)) {
       ...
    }

    mPromise->MaybeResolve(value); // or reject.
    ....
  }

In this way you can use the same PromiseWorkerProxy everywhere. For any internal methods, just use nullptr for the callback. but for GetDataStores you can use custom callbacks as such as:

JSStructuredCloneCallbacks gGetDataStoresCallbacks = {
  GetDataStoresStructuredCloneCallbacksRead,
  GetDataStoresStructuredCloneCallbacksWrite,
  GetDataStoresStructuredCloneCallbacksError
};
#define WORKER_DS_TAG JS_SCTAG_USER_MIN


GetDataStoresStructuredCloneCallbacksWrite should do:

GetDataStoresStructuredCloneCallbacksWrite(JSContext* aCx,
                                           JSStructuredCloneWriter* aWriter,
                                           JS::Handle<JSObject*> aObj,
                                           void* aClosure) {
    DataStore* store = nullptr;
    nsresult rv = UNWRAP_OBJECT(DataStore, obj, store);
    // some check
    nsRefPtr<workers::DataStore> wStore = new workers::DataStore();
    ... SetDataStore(store);
    return JS_WriteUint32Pair(writer, WORKER_DS_TAG, workerStore);

GetDataStoresStructuredCloneCallbacksRead should do:

GetDataStoresStructuredCloneCallbacksRead(JSContext* aCx,
                                          JSStructuredCloneReader* /* unused */,
                                          uint32_t aTag, uint32_t aData,
                                          void* aClosure) {
    if (aTag != WORKER_DS_TAG) ... error
    nsRefPtr<workers::DataStore> wStore = static_cast<workers::DataStore*>(aData);
    // wrap this object and return it.

mData can be useful to keep the objects alive. But I have to think about how to use it a bit more.

Comment 133

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

Comment on attachment 8363639 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V3, f=nsm

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

Hi, I think the code is wrong, but I'll need to give it an hour or so to read properly. I'll have feedback/review by tonight.

Comment 134

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

Comment on attachment 8364292 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread (WIP)

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

Drive by comments:
So on further reading, WorkerMainThreadRunnable does actually spin the worker sync event loop, and not block it as I originally thought. This changes things a little. GC will run on the worker thread, so you've to ensure that JS exposed objects that you want alive are also held alive by C++.

::: dom/promise/Promise.cpp
@@ +685,5 @@
> +  Maybe<JSAutoCompartment> ac;
> +  EnterCompartment(ac, cx, aValue);
> +
> +  JS::Rooted<JS::Value> newValue(cx);
> +  bool isReWrapped = ReWrapValue(cx, aValue, newValue.address());

I think this is going to blow up somewhere. newValue is created on the main thread, in a main thread runtime and compartment. I don't think you can just send it to WorkerPromiseResolverTask, really needs to be structure cloned properly.

::: dom/workers/DataStore.cpp
@@ +40,5 @@
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +DataStore::~DataStore()
> +{
> +  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();

MOZ_ASSERT(mainThread); If there is no main thread, things have gone really really wrong :)

@@ +48,5 @@
> +}
> +
> +already_AddRefed<DataStore>
> +DataStore::Constructor(GlobalObject& aGlobal, ErrorResult& aRv)
> +{

Thread assertion please.

::: dom/workers/DataStore.h
@@ +22,5 @@
> +namespace workers {
> +
> +class DataStoreCursor;
> +
> +class DataStore MOZ_FINAL : public nsDOMEventTargetHelper

Can you call this WorkerDataStore? Its hard to reason about two classes named DataStore, even though they are in separate namespaces.

@@ +83,5 @@
> +  void SetDataStoreImpl(DataStoreImpl& aStore, ErrorResult& aRv);
> +
> +  // This internal function is aimed to point the workers::DataStore to the
> +  // dom::DataStore.
> +  void SetDataStore(mozilla::dom::DataStore* aStore);

SetBackingDataStore

@@ +91,5 @@
> +
> +private:
> +  // A refrence to the dom::DataStore. Can be set by SetDataStore().
> +  // Note that this has to be living on the main thread.
> +  nsRefPtr<mozilla::dom::DataStore> mStore;

mBackingStore

::: dom/workers/DataStoreWorkerUtils.h
@@ +40,5 @@
> +  virtual bool
> +  ReWrapValue(JSContext* aCx,
> +              JS::Handle<JS::Value> aValue,
> +              JS::Value* aNewValue) MOZ_OVERRIDE
> +  {

Please add assertions if this is to run on a specific thread, otherwise add a comment saying any thread is ok.

@@ +99,5 @@
> +// WorkerNavigator::GetDataStores(...) on the main thread.
> +class NavigatorGetDataStoresRunnable : public WorkerMainThreadRunnable
> +{
> +  // Note that this has to be living on the worker thread.
> +  nsRefPtr<Promise> mWorkerPromise;

You can't guarantee which thread this object is destroyed on. Hold a nsWorkerThreadPtrHandle<Promise> instead

@@ +105,5 @@
> +  ErrorResult& mRv;
> +
> +public:
> +  NavigatorGetDataStoresRunnable(WorkerPrivate* aWorkerPrivate,
> +                                 Promise* aWorkerPromise,

This will be |const nsWorkerThreadPtrHandle<Promise>& aWorkerPromise|

@@ +158,5 @@
> +// mozilla::dom::DataStore.
> +class DataStoreRunnable : public WorkerMainThreadRunnable
> +{
> +protected:
> +  mozilla::dom::DataStore* mStore;

Which thread is mStore from?
I'm assuming worker thread for now. In which case, who has ownership of the DataStore?

@@ +176,5 @@
> +// mozilla::dom::DataStoreCursor.
> +class DataStoreCursorRunnable : public WorkerMainThreadRunnable
> +{
> +protected:
> +  mozilla::dom::DataStoreCursor* mCursor;

ownership again.

@@ +222,5 @@
> +  MainThreadRun() MOZ_OVERRIDE {
> +    AssertIsOnMainThread();
> +
> +    nsAutoString string;
> +    (mStore->*mFunc)(string, mRv);

Is it always safe to call these functions on the main thread for a object held on the worker thread?
If you use WorkerThreadPtrHolder<> you won't be able to do this either, so I'd really like to clarify the ownership

@@ +504,5 @@
> +};
> +
> +// Sub-class inheriting DataStoreRunnable to run
> +// mozilla::dom::DataStore::Sync(...) on the main thread.
> +class DataStoreSyncRunnable : public DataStoreRunnable

SyncRunnable means something completely different in workers. Call this DataStoreSyncStoreRunnable or similar.

::: dom/workers/moz.build
@@ +20,5 @@
>  # Stuff needed for the bindings, not really public though.
>  EXPORTS.mozilla.dom.workers.bindings += [
> +    'DataStore.h',
> +    'DataStoreCursor.h',
> +    'DataStoreWorkerUtils.h',

DataStoreWorkerUtils should be in EXPORTS.mozilla.dom.workers.

::: dom/workers/test/navigator_worker.js
@@ +5,5 @@
>  var supportedProps = [
>    "appCodeName",
>    "appName",
>    "appVersion",
> +  "getDataStores",

This should only be checked on B2G correct?

Comment 135

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

Comment on attachment 8363639 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V3, f=nsm

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

::: dom/promise/Promise.cpp
@@ +680,5 @@
> +  nsRefPtr<WorkerPromiseResolverTask> task =
> +    new WorkerPromiseResolverTask(mWorkerPrivate,
> +                                  mWorkerPromise,
> +                                  aValue,
> +                                  Promise::PromiseState::Resolved);

You are correct, you cannot use WorkerPromiseResolverTask directly.

::: dom/promise/PromiseWorkerProxy.h
@@ +30,5 @@
> +//   3. Create a Promise on the main thread.
> +//   4. Call Promise::AppendNativeHandler(PromiseWorkerProxy*) based on the
> +//      Promise at #3 to bind PromiseWorkerProxy at #2.
> +//   5. Then all the resolved/rejected results will be dispatched from the main
> +//      thread to the worker thread.

What you want to do is:
1. Create a Promise on the worker and return it to content script. create a holder |new WorkerThreadPtrHolder<Promise>| (notice Handle vs Holder)| on the worker thread.

// On worker thread
nsRefPtr<Promise> promise = new Promise(...);
WorkerThreadPtrHandle<Promise> p = new WorkerThreadPtrHolder<Promise>(promise);
Pass `p` around to the utility runnables.
return promise.forget();

2. Utility runnables create a PromiseWorkerProxy on the main thread which holds a WorkerThreadPtrHandle<Promise> to the worker promise holder. This is because the PromiseWorkerProxy will be in the main thread promise's CC chain and so we want to keep it on the main thread. 
3. Create a Promise on the main thread.
4. AppendNativeHandler(PromiseWorkerProxy*)
5. PromiseWorkerProxy's Resolve/Reject handlers create a lightweight runnable similar to WorkerPromiseResolveRunnable which structured clone's the resolution value and take a WorkerThreadPtrHandle<Promise> and in the Run() call MaybeResolveInternal/MaybeRejectInternal with SyncTask on the worker promise (the WorkerThreadPtrHandle<Promise> can be safely dereferenced on the worker thread). NOTE: Please make it very clear via comments whether this system of classes is meant to be used via a SyncRunnable (which blocks the worker event loop, in which case use AsyncTask), or via a WorkerMainThreadRunnable (which spins the worker event loop, in which case SyncTask is fine).

@@ +38,5 @@
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PromiseWorkerProxy)
> +
> +public:
> +  PromiseWorkerProxy(workers::WorkerPrivate* aWorkerPrivate,
> +                     Promise* aWorkerPromise);

const WorkerThreadPtrHandle<Promise>& aWorkerPromise

@@ +49,5 @@
> +  virtual void RejectedCallback(JS::Handle<JS::Value> aValue);
> +
> +private:
> +  workers::WorkerPrivate* mWorkerPrivate;
> +  Promise* mWorkerPromise;

WorkerThreadPtrHandle<Promise> mWorkerPromise;

Comment 136

[Gene Lian (I already quit Mozilla)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #134)
> ::: dom/promise/Promise.cpp
> @@ +685,5 @@
> > +  Maybe<JSAutoCompartment> ac;
> > +  EnterCompartment(ac, cx, aValue);
> > +
> > +  JS::Rooted<JS::Value> newValue(cx);
> > +  bool isReWrapped = ReWrapValue(cx, aValue, newValue.address());
> 
> I think this is going to blow up somewhere. newValue is created on the main
> thread, in a main thread runtime and compartment. I don't think you can just
> send it to WorkerPromiseResolverTask, really needs to be structure cloned
> properly.

I originally thought WorkerPromiseResolverTask would do the structured clone for me, because it sounds something like dispatching WorkerPromiseResolverTask from the main thread to the worker thread. This matches what I discovered at comment #128: it still dispatches WorkerPromiseResolverTask from the worker thread.

I think I'll stop using WorkerPromiseResolverTask and adopt Andrea solution at comment #132.

> ::: dom/workers/DataStore.h
> @@ +22,5 @@
> > +namespace workers {
> > +
> > +class DataStoreCursor;
> > +
> > +class DataStore MOZ_FINAL : public nsDOMEventTargetHelper
> 
> Can you call this WorkerDataStore? Its hard to reason about two classes
> named DataStore, even though they are in separate namespaces.

Bindings.conf generates the same name for both worker and non-worker by default. I think I can customize a WorkerDataStore for the worker. Thanks for the good point.

> @@ +99,5 @@
> > +// WorkerNavigator::GetDataStores(...) on the main thread.
> > +class NavigatorGetDataStoresRunnable : public WorkerMainThreadRunnable
> > +{
> > +  // Note that this has to be living on the worker thread.
> > +  nsRefPtr<Promise> mWorkerPromise;
> 
> You can't guarantee which thread this object is destroyed on. Hold a
> nsWorkerThreadPtrHandle<Promise> instead

nsWorkerThreadPtrHandle is not yet supported. I think I'll still use NS_ProxyRelease() to make sure it's always released on the right main thread. It's OK as long as we're careful.

> @@ +158,5 @@
> > +// mozilla::dom::DataStore.
> > +class DataStoreRunnable : public WorkerMainThreadRunnable
> > +{
> > +protected:
> > +  mozilla::dom::DataStore* mStore;
> 
> Which thread is mStore from?
> I'm assuming worker thread for now. In which case, who has ownership of the
> DataStore?

The WorkerDataStore's |nsRefPtr<mozilla::dom::DataStore> mStore| will hold this, so I just use raw pointer catch that to avoid AddRef() error since DataStoreRunnable is created on the worker thread.

> @@ +222,5 @@
> > +  MainThreadRun() MOZ_OVERRIDE {
> > +    AssertIsOnMainThread();
> > +
> > +    nsAutoString string;
> > +    (mStore->*mFunc)(string, mRv);
> 
> Is it always safe to call these functions on the main thread for a object
> held on the worker thread?

No, as mentioned above. mStore is passed as a raw pointer and is created and run its API on the main thread.

Comment 137

[Gene Lian (I already quit Mozilla)]

After some thoughts, I think Nikhil's nsWorkerThreadPtrHolder should be applied here to solve issue in a cleaner way...

Comment 138

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-0, a handle to safely release resources on workers (stolen from Nikhil at bug 947499), r=nsm

Hi Nikhil,

Hope you don't mind I stole your patch at bug 947499. Thanks to this, we can have less pain.

Comment 139

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V4, r=baku,nsm

Comment 140

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V4.1, r=baku,nsm

Fixed comment #132 and comment #135.

Comment 141

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

Comment on attachment 8365017 [details] [diff] [review]
Part 2-0, a handle to safely release resources on workers (stolen from Nikhil at bug 947499), r=nsm

I can't review my own patch :) Asking khuey since this deals with worker lifetime.

Comment 142

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

Comment on attachment 8365020 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V4.1, r=baku,nsm

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

Seems reasonable, have you tested it?

Me and Andrea will be at the work week next week, so if you want to go over some things, feel free to set up Vidyo or something.

::: dom/promise/Promise.cpp
@@ +671,5 @@
> +                             const WorkerThreadPtrHandle<Promise>& aWorkerPromise,
> +                             JSStructuredCloneCallbacks* aCallbacks,
> +                             JSAutoStructuredCloneBuffer& aBuffer,
> +                             void* aData,
> +                             Promise::PromiseState aState)

Please use PromiseCallback::Task everywhere. State is used to indicate what state a promise is in, while Task is used to indicate what state a Promise should be resolved to.

@@ +676,5 @@
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
> +    , mWorkerPromise(aWorkerPromise)
> +    , mCallbacks(aCallbacks)
> +    , mData(aData)
> +    , mState(aState)

call this mTask

@@ +748,5 @@
> +  EnterCompartment(ac, cx, aValue);
> +
> +  // The value is cloned and written in a buffer.
> +  JSAutoStructuredCloneBuffer buffer;
> +  if (!buffer.write(cx, aValue, mCallbacks, mData)) {

You aren't using the closure (mData) for anything, so you can drop that everywhere, correct?

Similarly for callbacks, drop it and the default ones will be used, unless you want custom behaviour.

Comment 143

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #142)
> Me and Andrea will be at the work week next week, so if you want to go over
> some things, feel free to set up Vidyo or something.

Gene will be off at least some of next week for CNY.

Comment 144

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #136)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #134)
> > ::: dom/promise/Promise.cpp
> > @@ +685,5 @@
> > > +  Maybe<JSAutoCompartment> ac;
> > > +  EnterCompartment(ac, cx, aValue);
> > > +
> > > +  JS::Rooted<JS::Value> newValue(cx);
> > > +  bool isReWrapped = ReWrapValue(cx, aValue, newValue.address());
> > 
> > I think this is going to blow up somewhere. newValue is created on the main
> > thread, in a main thread runtime and compartment. I don't think you can just
> > send it to WorkerPromiseResolverTask, really needs to be structure cloned
> > properly.
> 
> I originally thought WorkerPromiseResolverTask would do the structured clone
> for me, because it sounds something like dispatching
> WorkerPromiseResolverTask from the main thread to the worker thread. This
> matches what I discovered at comment #128: it still dispatches
> WorkerPromiseResolverTask from the worker thread.
> 
> I think I'll stop using WorkerPromiseResolverTask and adopt Andrea solution
> at comment #132.
> 
> > ::: dom/workers/DataStore.h
> > @@ +22,5 @@
> > > +namespace workers {
> > > +
> > > +class DataStoreCursor;
> > > +
> > > +class DataStore MOZ_FINAL : public nsDOMEventTargetHelper
> > 
> > Can you call this WorkerDataStore? Its hard to reason about two classes
> > named DataStore, even though they are in separate namespaces.
> 
> Bindings.conf generates the same name for both worker and non-worker by
> default. I think I can customize a WorkerDataStore for the worker. Thanks
> for the good point.

Use |nativeType| to change the name.


> 
> > @@ +158,5 @@
> > > +// mozilla::dom::DataStore.
> > > +class DataStoreRunnable : public WorkerMainThreadRunnable
> > > +{
> > > +protected:
> > > +  mozilla::dom::DataStore* mStore;
> > 
> > Which thread is mStore from?
> > I'm assuming worker thread for now. In which case, who has ownership of the
> > DataStore?
> 
> The WorkerDataStore's |nsRefPtr<mozilla::dom::DataStore> mStore| will hold
> this, so I just use raw pointer catch that to avoid AddRef() error since
> DataStoreRunnable is created on the worker thread.

Ok, is WorkerDataStore guaranteed to be around when these runnables are on different threads?

Comment 145

[Gene Lian (I already quit Mozilla)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #144)
> > > @@ +158,5 @@
> > > > +// mozilla::dom::DataStore.
> > > > +class DataStoreRunnable : public WorkerMainThreadRunnable
> > > > +{
> > > > +protected:
> > > > +  mozilla::dom::DataStore* mStore;
> > > 
> > > Which thread is mStore from?
> > > I'm assuming worker thread for now. In which case, who has ownership of the
> > > DataStore?
> > 
> > The WorkerDataStore's |nsRefPtr<mozilla::dom::DataStore> mStore| will hold
> > this, so I just use raw pointer catch that to avoid AddRef() error since
> > DataStoreRunnable is created on the worker thread.
> 
> Ok, is WorkerDataStore guaranteed to be around when these runnables are on
> different threads?

OK. I'll to use nsMainThreadPtrHandle to keep that. The original reason for why I used raw pointer because I think WorkerDataStore would keep strong reference to that. For more safety, just pass it around by nsMainThreadPtrHandle.

class DataStoreRunnable : public WorkerMainThreadRunnable
{
protected:
  nsMainThreadPtrHandle<mozilla::dom::DataStore> mStore;
}

Comment 146

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-0, a handle to safely release resources on workers, V1.1, r=khuey

Fixed compiling error when using this helper. We cannot hide a template's implementation in cpp:

http://stackoverflow.com/questions/10632251/undefined-reference-to-template-function

Comment 147

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8364292 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread (WIP)

Will come back with a better one by overriding the structured cloned callbacks.

Comment 148

[Gene Lian (I already quit Mozilla)]

> OK. I'll to use nsMainThreadPtrHandle to keep that. The original reason for
> why I used raw pointer because I think WorkerDataStore would keep strong
> reference to that. For more safety, just pass it around by
> nsMainThreadPtrHandle.
> 
> class DataStoreRunnable : public WorkerMainThreadRunnable
> {
> protected:
>   nsMainThreadPtrHandle<mozilla::dom::DataStore> mStore;
> }

I found this usage is problematic, because mStore is hard to CC-GC'ed. Eventually, I'll take Kyle's approach at comment #116:

  DataStoreRunnable -> WorkerDataStore -> DataStore

Comment 149

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread (WIP)

Hi Andrea and Nikhil,

Fixed comment #132 and comment #134. Could you please take a look about:

1. GetDataStoresStructuredCloneCallbacks{Read/Write}
2. And structured clone in DataStore::Add/Put

Also, I encountered a bottleneck at GetDataStoresStructuredCloneCallbacksRead. Don't know how to assign |store| on the worker thread, where it was created on the main thread:

  workerStore->mBackingStore = store;

Need your feedbacks. Thanks!

Comment 150

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

Comment on attachment 8365886 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread (WIP)

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

::: dom/base/Navigator.cpp
@@ +1117,5 @@
>  
> +already_AddRefed<Promise>
> +Navigator::GetDataStores(const nsAString& aName, ErrorResult& aRv)
> +{
> +  return GetDataStores(mWindow, aName, aRv);

Use GetWindow() here.

::: dom/workers/DataStoreCursor.h
@@ +54,5 @@
> +  virtual ~WorkerDataStoreCursor();
> +
> +public:
> +  // A refrence to the DataStoreCursor, which has to be living on the main thread.
> +  nsRefPtr<DataStoreCursor> mBackingCursor;

nsMainThreadPtrHolder

::: dom/workers/DataStoreWorkerUtils.h
@@ +127,5 @@
> +// Sub-class inheriting WorkerMainThreadRunnable to run
> +// WorkerNavigator::GetDataStores(...) on the main thread.
> +class NavigatorGetDataStoresRunnable : public WorkerMainThreadRunnable
> +{
> +  // Note that this has to be living on the worker thread.

You can remove this comment.

@@ +130,5 @@
> +{
> +  // Note that this has to be living on the worker thread.
> +  WorkerThreadPtrHandle<Promise> mWorkerPromise;
> +  const nsString mName;
> +  ErrorResult& mRv;

Why is this being stored? The promise was already returned, so there is no error handling to be done. ErrorResult cannot be used for a async result. You'll just have to reject the promise. This applies for all uses of mRv in this patch.

For sync getters, use the pattern described below in DataStoreGetStringRunnable.

@@ +143,5 @@
> +    , mName(aName)
> +    , mRv(aRv)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    MOZ_ASSERT(aWorkerPromise);

thread assertion.

@@ +148,5 @@
> +  }
> +
> +protected:
> +  virtual void
> +  MainThreadRun() MOZ_OVERRIDE {

Nit: brace on new line.

@@ +198,5 @@
> +    : WorkerMainThreadRunnable(aWorkerPrivate)
> +    , mWorkerStore(aWorkerStore)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    MOZ_ASSERT(mWorkerStore);

thread assertion.

@@ +216,5 @@
> +    : WorkerMainThreadRunnable(aWorkerPrivate)
> +    , mWorkerCursor(aWorkerCursor)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    MOZ_ASSERT(mWorkerCursor);

thread assertion.

@@ +231,5 @@
> +  typedef void
> +  (DataStore::*FuncType)(nsAString&, ErrorResult&);
> +
> +  FuncType mFunc;
> +  nsAString& mString;

nsString mString

and then you'd use the following form in the caller.

    nsRefPtr<DataStoreGetStringRunnable> runnable =
      new DataStoreGetStringRunnable(workerPrivate,
                                     this,
                                     &DataStore::GetName);
    runnable->Dispatch(aCx);
    aRv = runnable->GetStringValue(aString);

where GetStringValue() would copy mString into aString, and set aRv based on what happened on the main thread.

@@ +232,5 @@
> +  (DataStore::*FuncType)(nsAString&, ErrorResult&);
> +
> +  FuncType mFunc;
> +  nsAString& mString;
> +  ErrorResult& mRv;

I don't think the use here is thread safe, even in a blocking call. ErrorResult does various things on different kinds of errors, so I'd use a separate ErrorResult in each thread and copy values.

@@ +260,5 @@
> +};
> +
> +// Sub-class inheriting DataStoreRunnable to run:
> +//   - DataStore::Clear(...)
> +//   - DataStore::GetLength(...)

Comment would be "For DataStore methods that return Promises"

@@ +262,5 @@
> +// Sub-class inheriting DataStoreRunnable to run:
> +//   - DataStore::Clear(...)
> +//   - DataStore::GetLength(...)
> +// on the main thread.
> +class DataStoreReturnPromiseRunnable : public DataStoreRunnable

Just DataStorePromiseRunnable? You aren't really returning a Promise, just linking them up.

@@ +336,5 @@
> +class DataStoreGetRunnable : public DataStoreRunnable
> +{
> +  // Note that this has to be living on the worker thread.
> +  WorkerThreadPtrHandle<Promise> mWorkerPromise;
> +  const Sequence<OwningStringOrUnsignedLong>& mId;

Don't hold a reference, use SwapElements instead.

@@ +421,5 @@
> +                                       : nsContentUtils::GetSafeJSContext());
> +    MOZ_ASSERT(cx);
> +
> +    JS::Rooted<JS::Value> value(cx);
> +    if (!mObjBuffer.read(cx, &value, nullptr, nullptr)) {

you can skip writing nullptr

@@ +473,5 @@
> +  MainThreadRun() MOZ_OVERRIDE {
> +    AssertIsOnMainThread();
> +
> +    nsRefPtr<PromiseWorkerProxy> proxy =
> +      new PromiseWorkerProxy(mWorkerPrivate, mWorkerPromise);

I think the pattern of:

  // create proxy

  // do other stuff

  // AppendNativeHandler

is repeated everywhere. Can you refactor that?

::: dom/workers/Navigator.cpp
@@ +43,5 @@
> +already_AddRefed<Promise>
> +WorkerNavigator::GetDataStores(JSContext* aCx,
> +                               const nsAString& aName,
> +                               ErrorResult& aRv)
> +{

Nit: thread assertion here just to clarify for readers.

::: dom/workers/test/navigator_worker.js
@@ +5,5 @@
>  var supportedProps = [
>    "appCodeName",
>    "appName",
>    "appVersion",
> +  "getDataStores",

Nit from before, b2g only check.

Comment 151

[Gene Lian (I already quit Mozilla)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #150)
> Comment on attachment 8365886 [details] [diff] [review]
> Part 2-3, dispatch tasks on the worker to the main thread (WIP)
> 
> Review of attachment 8365886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Navigator.cpp
> @@ +1117,5 @@
> >  
> > +already_AddRefed<Promise>
> > +Navigator::GetDataStores(const nsAString& aName, ErrorResult& aRv)
> > +{
> > +  return GetDataStores(mWindow, aName, aRv);
> 
> Use GetWindow() here.

Not really get this. Do you mean creating a GetWindow() to return |mWindow|? But it's just for internal use so far.

> @@ +231,5 @@
> > +  typedef void
> > +  (DataStore::*FuncType)(nsAString&, ErrorResult&);
> > +
> > +  FuncType mFunc;
> > +  nsAString& mString;
> 
> nsString mString
> 
> and then you'd use the following form in the caller.
> 
>     nsRefPtr<DataStoreGetStringRunnable> runnable =
>       new DataStoreGetStringRunnable(workerPrivate,
>                                      this,
>                                      &DataStore::GetName);
>     runnable->Dispatch(aCx);
>     aRv = runnable->GetStringValue(aString);
> 
> where GetStringValue() would copy mString into aString, and set aRv based on
> what happened on the main thread.

I think this is fine. Please see workers/URL.cpp:

1. In SetterRunnable for how it uses |ErrorResult& mRv|
2. In GetterRunnable for how it sets |nsString& mValue|.

> 
> @@ +232,5 @@
> > +  (DataStore::*FuncType)(nsAString&, ErrorResult&);
> > +
> > +  FuncType mFunc;
> > +  nsAString& mString;
> > +  ErrorResult& mRv;
> 
> I don't think the use here is thread safe, even in a blocking call.
> ErrorResult does various things on different kinds of errors, so I'd use a
> separate ErrorResult in each thread and copy values.

Ditto.

The rest looks great to me! Thanks Nikhil for the feedback!

Comment 152

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8365805 [details] [diff] [review]
Part 2-0, a handle to safely release resources on workers, V1.1, r=khuey

I don't understand what you are trying to do here.  As you point out, just using WorkerThreadPtrHolder is not sufficient because you still have to keep the worker from shutting down (via AddFeature, or a sync loop, or something).  If you just need to hold things alive across a synchronous call to the main thread you can do that with nsCOM/RefPtr on the stack with the sync loop and then just use raw pointers on the main thread.  Are you trying to do something more complicated than that?

(The idea of optionally enforcing threading invariants is pretty terrifying too.)

Comment 153

[Gene Lian (I already quit Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #152)
> Comment on attachment 8365805 [details] [diff] [review]
> Part 2-0, a handle to safely release resources on workers, V1.1, r=khuey
> 
> I don't understand what you are trying to do here.  As you point out, just
> using WorkerThreadPtrHolder is not sufficient because you still have to keep
> the worker from shutting down (via AddFeature, or a sync loop, or
> something).  If you just need to hold things alive across a synchronous call
> to the main thread you can do that with nsCOM/RefPtr on the stack with the
> sync loop and then just use raw pointers on the main thread.  Are you trying
> to do something more complicated than that?

Yes, the most complicated part is we have to do something in the part-2-1 and part-2-3 patches. Could you please take a look if you have a chance? Thanks!

//   1. Create a Promise on the worker and return it to the content script and
//      create a holder |WorkerThreadPtrHolder<Promise>| on the worker thread.
//      For example,
//
//        nsRefPtr<Promise> promise = new Promise(nullptr);
//        WorkerThreadPtrHandle<Promise> p =
//          new WorkerThreadPtrHolder<Promise>(promise);
//        // Pass |p| around to the WorkerMainThreadRunnable
//        return promise.forget();
//
//   2. In WorkerMainThreadRunnable::MainThreadRun(), create a PromiseWorkerProxy
//      which holds a WorkerThreadPtrHandle<Promise> to the one at #1.
//
//   3. Create a Promise on the main thread.
//
//   4. Call Promise::AppendNativeHandler(PromiseWorkerProxy*) based on the
//      Promise at #3 to bind the PromiseWorkerProxy at #2.
//
//   5. Then the Promise results returned by ResolvedCallback/RejectedCallback
//      will be dispatched to the worker thread to resolve/reject.

From #1 to #2 we have to pass around Promise from the worker to the main thread. Waiting for the callback's result on the main thread. Then, from #4 to #5 we have to pass around Promise from the main thread to the worker. Eventually, solve/reject a Promise (which was originally created on the worker) on the worker thread.

With WorkerThreadPtrHandle<Promise>, we can easily pass around Promise across different threads and make sure it will be safely released on the right (worker) thread finally. And most importantly, we have to make sure Promise won't be released during all the async processes.

Comment 154

[Gene Lian (I already quit Mozilla)]

Ping Kyle. Although I'm on CNY holidays, I can still be stick to this. Could you please give me some advices/hints about how to pass around the Promise back and forth between the main thread and the worker thread? I don't think we can simply pass raw pointer for that because we'd never know if the content script still holds the promise or not.

(WorkerThreadPtrHandle still seems to be the cleanest solution to me if we want to keep Promise alive in the async process of resolving/rejecting it)

Comment 155

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

(In reply to Gene Lian [:gene] (CNY Jan. 29 ~ Feb. 5) from comment #154)
> Ping Kyle. Although I'm on CNY holidays, I can still be stick to this. Could
> you please give me some advices/hints about how to pass around the Promise
> back and forth between the main thread and the worker thread? I don't think
> we can simply pass raw pointer for that because we'd never know if the
> content script still holds the promise or not.
> 
> (WorkerThreadPtrHandle still seems to be the cleanest solution to me if we
> want to keep Promise alive in the async process of resolving/rejecting it)

If you are using the worker sync loop, the WorkerThreadPtrHandle is sufficient.

Comment 156

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V5

Comment 157

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread (WIP)

Comment 158

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Gene and I talked about this in person.  I'm wary of having a generic template for this because of the need to keep the worker alive to release the object.  We sketched out a plan to have the PromiseWorkerProxy handle all of the lifetime management.

Comment 159

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V6

Hi Kyle, I just uploaded a new patch for PromiseWorkerProxy plus AddFeature/RemoveFeature. Could you please take a look to see if I'm in the right direction? I'm testing them at the same time.

Comment 160

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread (WIP)

Comment 161

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8373887 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V6

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

::: dom/promise/Promise.cpp
@@ +1152,5 @@
> +}
> +
> +PromiseWorkerProxy::~PromiseWorkerProxy()
> +{
> +  MOZ_ASSERT(mCleanedUp);

We should also assert !mWorkerPromise

::: dom/promise/PromiseWorkerProxy.h
@@ +54,5 @@
> +
> +  workers::WorkerPrivate* mWorkerPrivate;
> +
> +  // This has to be living on the worker thread.
> +  nsRefPtr<Promise> mWorkerPromise;

mWorkerPrivate and mWorkerPromise should be private.

@@ +58,5 @@
> +  nsRefPtr<Promise> mWorkerPromise;
> +
> +  bool StoreISupports(nsISupports* aSupports);
> +
> +  void cleanUp(JSContext* aCx);

CleanUp (capitalize every word is Gecko style)

@@ +67,5 @@
> +  virtual void RejectedCallback(JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;
> +
> +  virtual bool Notify(JSContext* aCx, workers::Status aStatus) MOZ_OVERRIDE;
> +
> +private:  

nit: whitespace at EOL

@@ +74,5 @@
> +  JSStructuredCloneCallbacks* mCallbacks;
> +
> +  // Aimed to keep objects alive when doing the structured-clone read/write,
> +  // which can be added by calling StoreISupports() on the main thread.
> +  nsTArray<nsMainThreadPtrHandle<nsISupports> > mSupportsArray;

nit: all our compilers can handle T<U<V>>

Comment 162

[Gene Lian (I already quit Mozilla)]

After testing, I realized my structured clone read/write algorithm is not correct (should consider it as an array of DataStores). Fixing them.

Comment 163

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread (WIP)

Fixed the structured-clone read/write callbacks for getDataStores().

Comment 164

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-4, should create array based on the window to return

Please see the off-line e-mail discussion with bz.

Comment 165

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch event on workers

Comment 166

[(no longer active)]

Comment on attachment 8375451 [details] [diff] [review]
Part 2-5, a proxy to dispatch event on workers

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

I don't feel very confident providing feedback on this patch, as I'm not very familiar with the worker code.  Please ask someone who is more familiar with this code.  Thanks!

::: dom/datastore/DataStore.h
@@ +92,5 @@
>    // JS. We also need to let the DataStoreImpl implementation keep the event
>    // target of DataStore, so that it can know where to fire the events.
>    void SetDataStoreImpl(DataStoreImpl& aStore, ErrorResult& aRv);
>  
> +  nsRefPtr<DataStoreImpl> GetDataStoreImpl();

Nit: Please return a raw pointer.  Also, make the method const.

Comment 167

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V6.1

Comment 168

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8374770 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread (WIP)

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

Hi Kyle, I have already passed most of the test cases except for doing the structure-cloned write for the JS string (due to the wrong compartment matching), so hope to ask for feedback for now. I'll keep fixing it.

Comment 169

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8375451 [details] [diff] [review]
Part 2-5, a proxy to dispatch event on workers

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

Hi Kyle,

I did a very similar thing for dispatching the event from the main thread to the worker thread. Could you please give some feedbacks at this moment? Thanks!

Comment 170

[Andrea Marchesini [:baku]]

Comment on attachment 8376149 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V6.1

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

looks good to me.

::: dom/promise/Promise.cpp
@@ +1196,5 @@
> +
> +  JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
> +
> +  // Do we need this?
> +  Maybe<AutoCxPusher> pusher;

Some dom/js peer can review this.

Comment 171

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8376149 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V6.1

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

::: dom/promise/Promise.cpp
@@ +1196,5 @@
> +
> +  JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
> +
> +  // Do we need this?
> +  Maybe<AutoCxPusher> pusher;

Eventually, we found out we need this and the following EnterCompartment(...). Otherwise, the aValue (if it's an object) will not match the compartment when doing the structured-clone write.

Comment 172

[Boris Zbarsky [:bzbarsky]]

> Eventually, we found out we need this 

We need to push the cx and enter the right compartment.  However:

1)  Just use AutoSafeJSContext cx instead of manually reimplementing it.
2)  I suspect that if aValue is a string the code here asserts fatally and dies, no?

Comment 173

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8376149 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V6.1

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

We're getting closer.

The main problem with this patch is that it's possible to race on mCleanedUp.  That is, it is written to on the worker thread (in CleanUp()) but read on the main thread (in Resolved/RejectedCallback).  If the CPU chooses to switch threads between reading mCleanedUp and dispatching the runnable, and switch to and run CleanUp on the worker thread, we'll be dispatching to a WorkerPrivate that may no longer be alive.

There are two ways to fix this.  One is that in CleanUp instead of immediately calling RemoveFeature we could send a message (runnable) to the main thread to set mCleanedUp there (so that it's only ever touched on the main thread) and then have the main thread send back a message saying it's now safe to call RemoveFeature.  The other is that we could just use a lock to protect mCleanedUp.  I think the second is the way to go here (requires writing much less code).

::: dom/promise/Promise.cpp
@@ +1077,5 @@
> +public:
> +  PromiseWorkerProxyRunnable(PromiseWorkerProxy* aPromiseWorkerProxy,
> +                             JSStructuredCloneCallbacks* aCallbacks,
> +                             JSAutoStructuredCloneBuffer&& aBuffer,
> +                             PromiseCallback::Task aTask)

For bonus points you could use pointer to member here instead of this Task variable.

@@ +1097,5 @@
> +    aWorkerPrivate->AssertIsOnWorkerThread();
> +
> +    MOZ_ASSERT(mPromiseWorkerProxy);
> +    nsRefPtr<Promise> workerPromise = mPromiseWorkerProxy->GetWorkerPromise();
> +    MOZ_ASSERT(workerPromise);

Assert that the Proxy's WorkerPrivate matches aWorkerPrivate still.

@@ +1103,5 @@
> +    // Here we convert the buffer to a JS::Value.
> +    JS::Rooted<JS::Value> value(aCx);
> +    if (!mBuffer.read(aCx, &value, mCallbacks, mPromiseWorkerProxy)) {
> +      JS_ClearPendingException(aCx);
> +      MOZ_ASSERT(false, "cannot read the JSAutoStructuredCloneBuffer!");

This could in theory come from an out of memory condition, so instead of asserting you should just return.

@@ +1113,5 @@
> +                                     Promise::PromiseTaskSync::SyncTask);
> +    } else if (mTask == PromiseCallback::Task::Reject) {
> +      workerPromise->RejectInternal(aCx,
> +                                    value,
> +                                    Promise::PromiseTaskSync::SyncTask);

Then you wouldn't need these conditionals (you would just call the function pointer).

@@ +1116,5 @@
> +                                    value,
> +                                    Promise::PromiseTaskSync::SyncTask);
> +    } else {
> +      NS_NOTREACHED("should never get here!");
> +      return false;

And you definitely wouldn't need this.

@@ +1164,5 @@
> +WorkerPrivate*
> +PromiseWorkerProxy::GetWorkerPrivate()
> +{
> +  return mWorkerPrivate;
> +}

After you add the lock, this function should, *in DEBUG builds only*, grab the lock and assert that mCleanedUp is false (because it is not safe to use the WorkerPrivate after it is true).

@@ +1216,5 @@
> +  nsRefPtr<PromiseWorkerProxyRunnable> runnable =
> +    new PromiseWorkerProxyRunnable(this,
> +                                   mCallbacks,
> +                                   Move(buffer),
> +                                   PromiseCallback::Task::Resolve);

ResolvedCallback and RejectedCallback are the same function except for this Task value.  Combine them into a single function and make the Resolve/Reject Task value (or function pointers to the Resolve/Reject functions) an argument to it.

So something like

void
PromiseWorkerProxy::ResolvedCallback(JS::Handle<JS::Value> aValue) {
  ResultCallback(aValue, ResolveFunctionPointer);
}

void
PromiseWorkerProxy::RejectedCallback(JS::Handle<JS::Value> aValue) {
  ResultCallback(aValue, RejectedFunctionPointer);
}

void
PromiseWorkerProxy::ResultCallback(JS::Handle<JS::Value> aValue, FunctionPointer* aPtr) {
  // do all the work
}

etc

@@ +1280,5 @@
> +  MOZ_ASSERT(mWorkerPrivate);
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  MOZ_ASSERT(mWorkerPrivate->GetJSContext() == aCx);
> +
> +  if (mCleanedUp == true) {

This function will need to grab the lock and hold it until it calls RemoveFeature.

::: dom/promise/PromiseWorkerProxy.h
@@ +28,5 @@
> +//        nsRefPtr<Promise> promise = new Promise(nullptr);
> +//        // Pass |promise| around to the WorkerMainThreadRunnable
> +//        return promise.forget();
> +//
> +//   2. In WorkerMainThreadRunnable's contructor, create a PromiseWorkerProxy

In your WorkerMainThreadRunnable's

@@ +31,5 @@
> +//
> +//   2. In WorkerMainThreadRunnable's contructor, create a PromiseWorkerProxy
> +//      which holds a nsRefPtr<Promise> to the Promise created at #1.
> +//
> +//   3. In WorkerMainThreadRunnable::MainThreadRun(), obtain a Promise on the

your

@@ +49,5 @@
> +  PromiseWorkerProxy(workers::WorkerPrivate* aWorkerPrivate,
> +                     Promise* aWorkerPromise,
> +                     JSStructuredCloneCallbacks* aCallbacks = nullptr);
> +
> +  virtual ~PromiseWorkerProxy();

dtor should be private

@@ +51,5 @@
> +                     JSStructuredCloneCallbacks* aCallbacks = nullptr);
> +
> +  virtual ~PromiseWorkerProxy();
> +
> +  workers::WorkerPrivate* GetWorkerPrivate();

This should just be WorkerPrivate().  This will never return null.

@@ +53,5 @@
> +  virtual ~PromiseWorkerProxy();
> +
> +  workers::WorkerPrivate* GetWorkerPrivate();
> +
> +  nsRefPtr<Promise> GetWorkerPromise();

If you need to return an AddRefed pointer this should be already_AddRefed<Promise>.  If not, just Promise*.  There is almost never a good reason to return a smart pointer.

Comment 174

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8362438 [details] [diff] [review]
Part 2-2, refactor WorkerMainThreadRunnable, V2, r=baku

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

For bonus points you could make the newly added ConsoleRunnable subclass this.  If you don't, please file a followup so we remember to.

This looks good, I just want to see it with the corrections made.

::: dom/workers/WorkerRunnable.cpp
@@ +479,5 @@
>  
>  NS_IMPL_ISUPPORTS_INHERITED0(WorkerControlRunnable, WorkerRunnable)
> +
> +
> +WorkerMainThreadRunnable::WorkerMainThreadRunnable(WorkerPrivate* aWorkerPrivate)

nit: only one \n above

@@ +507,5 @@
> +WorkerMainThreadRunnable::Run()
> +{
> +  AssertIsOnMainThread();
> +
> +  MainThreadRun();

make this return bool

@@ +512,5 @@
> +
> +  nsRefPtr<MainThreadStopSyncLoopRunnable> response =
> +    new MainThreadStopSyncLoopRunnable(mWorkerPrivate,
> +                                       mSyncLoopTarget.forget(),
> +                                       true);

and then use the return value instead of bool

@@ +515,5 @@
> +                                       mSyncLoopTarget.forget(),
> +                                       true);
> +  if (!response->Dispatch(nullptr)) {
> +    NS_WARNING("Failed to dispatch response!");
> +  }

This should never happen, so assert.  e.g.

MOZ_ALWAYS_TRUE(response->Dispatch(nullptr));

::: dom/workers/WorkerRunnable.h
@@ +316,5 @@
>  };
>  
> +// Base class for the runnable objects, which is aimed to dispatch the tasks
> +// from the worker thread to the main thread. The derived class needs to
> +// override the MainThreadRun();

"The derived class must override MainThreadRun."  Also note that this makes a synchronous call from the worker thread to the main thread.

@@ +331,5 @@
> +public:
> +  bool Dispatch(JSContext* aCx);
> +
> +private:
> +  NS_IMETHOD Run();

MOZ_OVERRIDE

@@ +334,5 @@
> +private:
> +  NS_IMETHOD Run();
> +
> +protected:
> +  virtual void MainThreadRun() = 0;

use a bool return type here.

Comment 175

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8375451 [details] [diff] [review]
Part 2-5, a proxy to dispatch event on workers

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

This has the same CleanUp locking problem that the PromiseWorkerProxy class did.  Maybe we should create a subclass that handles the CleanUp logic?

There's a lot of extraneous whitespace at the end of lines in this patch.

::: dom/workers/DataStoreWorkerUtils.h
@@ +29,5 @@
>  
>  BEGIN_WORKERS_NAMESPACE
>  
> +class EventTargetProxy : public EventTarget
> +                       , public WorkerFeature

I'm not sure exactly what you're trying to do here, but having something inherit EventTarget (and hence nsWrapperCache) and have threadsafe refcounting is not correct.

Comment 176

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8374770 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread (WIP)

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

::: dom/base/Navigator.h
@@ +155,5 @@
>    battery::BatteryManager* GetBattery(ErrorResult& aRv);
> +
> +  already_AddRefed<Promise> GetDataStores(nsCOMPtr<nsPIDOMWindow> aWindow,
> +                                          const nsAString& aName,
> +                                          ErrorResult& aRv);

This should be static.

::: dom/datastore/DataStore.h
@@ +26,5 @@
> +//
> +// Guessing this is due to the three-layer reference:
> +// WorkerDataStore -> DataStore -> DataStoreImpl
> +//
> +// class DataStoreImpl;

Moving the destructor out of line will fix this.

::: dom/datastore/DataStoreCursor.cpp
@@ +30,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DataStoreCursor)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mCursor)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END

Revert this all to NS_IMPL_CYCLE_COLLECTION_1

::: dom/datastore/DataStoreCursor.h
@@ +31,5 @@
> +//
> +// Guessing this is due to the three-layer reference:
> +// WorkerDataStoreCursor -> DataStoreCursor -> DataStoreCursorImpl
> +//
> +// class DataStoreCursorImpl;

Moving DataStoreCursor's destructor out of line would probably fix that.

@@ +39,5 @@
> +// in the WorkerDataStoreCursor.
> +//
> +// Compile error:
> +// nsProxyRelease.h:47: error: cannot convert 'mozilla::dom::DataStoreCursor*'
> +// to 'nsISupports*' for argument '2' to 'nsresult NS_ProxyRelease(nsIEventTarget*, nsISupports*, bool)'

You can remove this comment.  This is only useful information to your reviewer ;)

@@ +45,4 @@
>  {
>  public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(DataStoreCursor)

Remove the SCRIPT_HOLDER part.  You're not using the Trace hook, so there's no need for it.

::: dom/webidl/WorkerNavigator.webidl
@@ +7,5 @@
>  
>  WorkerNavigator implements NavigatorID;
>  WorkerNavigator implements NavigatorOnLine;
> +
> +WorkerNavigator implements NavigatorDataStore;

No \n above.

::: dom/workers/DataStore.cpp
@@ +34,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(WorkerDataStore,
> +                                                nsDOMEventTargetHelper)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END

There's no need to declare a cycle collection class and redeclare nsISupports if you don't add anything here.

@@ +44,5 @@
> +
> +already_AddRefed<WorkerDataStore>
> +WorkerDataStore::Constructor(GlobalObject& aGlobal, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());

You should get the cx from the GlobalObject, and the WorkerPrivate from that, and assert that we're on the right thread.

::: dom/workers/DataStore.h
@@ +38,5 @@
> +
> +  // WebIDL (internal functions)
> +
> +  static already_AddRefed<WorkerDataStore> Constructor(GlobalObject& aGlobal,
> +                                                 ErrorResult& aRv);

return type on previous line.

throughout this file.

::: dom/workers/DataStoreWorkerUtils.h
@@ +22,5 @@
> +#include "WorkerPrivate.h"
> +#include "WorkerRunnable.h"
> +
> +
> +// TODO These runnables should all go in the .cpp file they are used in.

Yes, they really should.  It would make this a lot easier to review ...

@@ +164,5 @@
> +      return;
> +    }
> +
> +    nsRefPtr<Promise> promise = static_cast<Navigator*>
> +      (navigator.get())->GetDataStores(window, mName, mRv);

If you make this function static this will just be Navigator::GetDataStores(args), and you won't need the GetNavigator bit above.

::: dom/workers/Navigator.h
@@ +9,5 @@
>  #include "Workers.h"
>  #include "nsString.h"
>  #include "nsWrapperCache.h"
>  
> +// Need this to hoop up to Navigator::HasDataStoreSupport()

I think you mean hook, not hoop?  I would just say "need this to use .."

@@ +106,5 @@
>    {
>      mOnline = aOnline;
>    }
> +
> +  already_AddRefed<Promise> GetDataStores(JSContext* aCx,

return type on the previous line.

Comment 177

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8374770 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread (WIP)

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

::: dom/workers/DataStoreWorkerUtils.h
@@ +242,5 @@
> +  {
> +    AssertIsOnMainThread();
> +
> +    nsAutoString string;
> +    (mBackingStore.get()->*mFunc)(string, mRv);

If this .get() is really necessary to compile, please go add an operator->* to nsMainThreadPtrHandle.

Comment 178

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.3

Just rebased.

Comment 179

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V7

Fixed comment #173 except for:

1. I cannot add lock in GetWorkerPrivate() because this is called in the main thread and has been locked in RunCallback() which constructs PromiseWorkerProxyRunnable() which uses GetWorkerPrivate().

2. I cannot not s/GetWorkerPrivate()/WorkerPrivate() because |WorkerPrivate| is already a existing class type.

3. I don't think I can leverage bug 974120 at this moment.

Thanks for the review!

Comment 180

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-2, refactor WorkerMainThreadRunnable, V3

Fixed comment #174.

Comment 181

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.4

Don't need to use SCRIPT_HOLDER since we don't want to trace.

Comment 182

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.5

Apply the recent change on Navigator::HasDataStoreSupport().

Comment 183

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-0, pass around global for creating Promise value (by baku)

Thanks Andrea's awesome patch which fix the structured-clone write for string value.

Comment 184

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V8

Rebased on the part 2-0 patch.

Comment 185

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread

Fixed comment #176 except for the coding style. I was told by Ehsan at comment #78: please put the return type on the same line as the function name.  The return type only goes on its own line in the function definition.

Please let me know if you insist on making return type separate line both in headers and cpp.

Comment 186

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8378197 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V8

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

Almost there.  I think the next iteration will be the last.

::: dom/promise/Promise.cpp
@@ +8,5 @@
>  
>  #include "jsfriendapi.h"
>  #include "mozilla/dom/OwningNonNull.h"
>  #include "mozilla/dom/PromiseBinding.h"
> +#include "mozilla/Monitor.h"

Monitor is just a Mutex combined with a CondVar.  But you're not using the CondVar part, so you can just use Mutex instead of Monitor, MutexAutoLock instead of MonitorAutoLock, etc.

@@ +1104,5 @@
> +      JS_ClearPendingException(aCx);
> +      return false;
> +    }
> +
> +    (workerPromise.get()->*mFunc)(aCx,

Is this .get() required to compile?  If so, please file a followup on fixing nsRefPtr's operator->*.

@@ +1154,5 @@
> +  MOZ_ASSERT(!mWorkerPromise);
> +}
> +
> +WorkerPrivate*
> +PromiseWorkerProxy::GetWorkerPrivate()

This function should be const.

@@ +1156,5 @@
> +
> +WorkerPrivate*
> +PromiseWorkerProxy::GetWorkerPrivate()
> +{
> +  MOZ_ASSERT(!mCleanedUp);

Please add a comment stating that it's ok to race on mCleanedUp, because it will never cause us to fire the assertion when we should not.

@@ +1162,5 @@
> +  return mWorkerPrivate;
> +}
> +
> +Promise*
> +PromiseWorkerProxy::GetWorkerPromise()

This one too.

@@ +1168,5 @@
> +  return mWorkerPromise;
> +}
> +
> +bool
> +PromiseWorkerProxy::StoreISupports(nsISupports* aSupports)

make this function return void, since it always returns true.

@@ +1173,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  mSupportsArray.AppendElement(
> +    new nsMainThreadPtrHolder<nsISupports>(aSupports));

This leaks, because you create a new nsMainThreadPtrHolder on the heap but it never gets deleted.  Rewrite this as:

nsMainThreadPtrHolder<nsISupports> supports(aSupports);
mSupportsArray.AppendElement(supports);

@@ +1181,5 @@
> +
> +void
> +PromiseWorkerProxy::RunCallback(JS::Handle<JSObject*> aGlobal,
> +                                JS::Handle<JS::Value> aValue,
> +                                RunCallbackFunc aFunc) {

brace on the next line.

@@ +1182,5 @@
> +void
> +PromiseWorkerProxy::RunCallback(JS::Handle<JSObject*> aGlobal,
> +                                JS::Handle<JS::Value> aValue,
> +                                RunCallbackFunc aFunc) {
> +  MonitorAutoLock monitor(mLockCleanUpMonitor);

We usually call these RAII variables 'lock'.

@@ +1184,5 @@
> +                                JS::Handle<JS::Value> aValue,
> +                                RunCallbackFunc aFunc) {
> +  MonitorAutoLock monitor(mLockCleanUpMonitor);
> +
> +  MOZ_ASSERT(NS_IsMainThread());

Threading assertions at the top of the function.

@@ +1186,5 @@
> +  MonitorAutoLock monitor(mLockCleanUpMonitor);
> +
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  // If the worker thread's been cancelled don't need to resolve the Promise.

... cancelled we don't ...

@@ +1194,5 @@
> +
> +  JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
> +
> +  Maybe<AutoCxPusher> pusher;
> +  pusher.construct(cx);

Replace this with | AutoJSContext cx; |

@@ +1213,5 @@
> +                                   mCallbacks,
> +                                   Move(buffer),
> +                                   aFunc);
> +
> +  mWorkerPrivate->Dispatch(runnable);

Do | runnable->Dispatch(cx); | instead.

@@ +1255,5 @@
> +  MOZ_ASSERT(mWorkerPrivate->GetJSContext() == aCx);
> +
> +  if (mCleanedUp == true) {
> +    return;
> +  }

You need to check mCleanedUp before the assertions, because mWorkerPrivate might not be safe to use anymore if we have already cleaned up and RemoveFeature()d.  Please add a comment about this.

@@ +1259,5 @@
> +  }
> +
> +  // Release the Promise and remove the PromiseWorkerProxy from the features of
> +  // the worker thread since the Promise has been resolved/rejected or the
> +  // worker thread has been cancelled earlier.

delete the word 'earlier'.

::: dom/promise/PromiseWorkerProxy.h
@@ +23,5 @@
> +// and resolve/reject that on the worker thread eventually.
> +//
> +// How to use:
> +//
> +//   1. Create a Promise on the worker and return it to the content script:

worker thread.

@@ +45,5 @@
> +                           public workers::WorkerFeature
> +{
> +  friend class PromiseWorkerProxyRunnable;
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PromiseWorkerProxy)

Please add a comment that this overrides the non-threadsafe refcounting in PromiseNativeHandler.

@@ +54,5 @@
> +                     JSStructuredCloneCallbacks* aCallbacks = nullptr);
> +
> +  workers::WorkerPrivate* GetWorkerPrivate();
> +
> +  Promise* GetWorkerPromise();

As I mentioned earlier, these both should be const.

@@ +56,5 @@
> +  workers::WorkerPrivate* GetWorkerPrivate();
> +
> +  Promise* GetWorkerPromise();
> +
> +  bool StoreISupports(nsISupports* aSupports);

and this should return void.

@@ +82,5 @@
> +                   JS::Handle<JS::Value> aValue, RunCallbackFunc aFunc);
> +
> +  workers::WorkerPrivate* mWorkerPrivate;
> +
> +  // This has to be living on the worker thread.

Just "this lives on the worker thread"

Comment 187

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8378110 [details] [diff] [review]
Part 2-2, refactor WorkerMainThreadRunnable, V3

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

r=me with comments addressed.

::: dom/workers/URL.cpp
@@ +225,5 @@
>      nsresult rv;
>      nsCOMPtr<nsIIOService> ioService(do_GetService(NS_IOSERVICE_CONTRACTID, &rv));
>      if (NS_FAILED(rv)) {
>        mRv.Throw(rv);
> +      return false;

The boolean return value controls whether we should throw a special exception when exiting the sync loop.  Since we're already thrown on mRv here there's no need to do that, and we can return true here.

@@ +235,5 @@
>        rv = ioService->NewURI(NS_ConvertUTF16toUTF8(mBase), nullptr, nullptr,
>                               getter_AddRefs(baseURL));
>        if (NS_FAILED(rv)) {
>          mRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +        return false;

same here.

@@ +246,5 @@
>      rv = ioService->NewURI(NS_ConvertUTF16toUTF8(mURL), nullptr, baseURL,
>                             getter_AddRefs(url));
>      if (NS_FAILED(rv)) {
>        mRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +      return false;

same here.

::: dom/workers/WorkerRunnable.h
@@ +363,5 @@
> +private:
> +  NS_IMETHOD Run() MOZ_OVERRIDE;
> +
> +protected:
> +  virtual bool MainThreadRun() = 0;

Rearrange the members of this class so all the protected are together.

Comment 188

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8378205 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread

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

I still want you to break up DataStoreWorkerUtils.h/cpp to make this easier to review.

::: dom/base/Navigator.h
@@ +153,5 @@
>    // The XPCOM GetDoNotTrack is ok
>    Geolocation* GetGeolocation(ErrorResult& aRv);
>    battery::BatteryManager* GetBattery(ErrorResult& aRv);
> +
> +  static already_AddRefed<Promise> GetDataStores(nsCOMPtr<nsPIDOMWindow> aWindow,

Just nsPIDOMWindow*.

::: dom/datastore/DataStoreCursor.h
@@ +22,5 @@
>  class GlobalObject;
>  class DataStoreCursorImpl;
>  
> +// We have to make this inherited from nsISupports, because we're using
> +// nsMainThreadPtrHandle<DataStoreCursor> in the WorkerDataStoreCursor.

You can remove this comment.

::: dom/workers/DataStore.h
@@ +28,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +public:

No need to mark this public again.

@@ +84,5 @@
> +protected:
> +  virtual ~WorkerDataStore() {}
> +
> +public:
> +  nsMainThreadPtrHandle<DataStore> mBackingStore;

Why is this public?

::: dom/workers/DataStoreCursor.h
@@ +52,5 @@
> +protected:
> +  virtual ~WorkerDataStoreCursor() {}
> +
> +public:
> +  nsMainThreadPtrHandle<DataStoreCursor> mBackingCursor;

Why is this public?

::: dom/workers/DataStoreWorkerUtils.h
@@ +22,5 @@
> +class WorkerDataStoreCursor;
> +
> +// Sub-class inheriting WorkerMainThreadRunnable to run
> +// WorkerNavigator::GetDataStores(...) on the main thread.
> +class NavigatorGetDataStoresRunnable MOZ_FINAL : public WorkerMainThreadRunnable

This should go in Navigator.cpp.

@@ +40,5 @@
> +};
> +
> +// Sub-class inheriting WorkerMainThreadRunnable, which holds a reference to
> +// WorkerDataStore.
> +class DataStoreRunnable : public WorkerMainThreadRunnable

This, and everything that inherits from it, should go in DataStore.cpp

@@ +52,5 @@
> +};
> +
> +// Sub-class inheriting WorkerMainThreadRunnable, which holds a reference to
> +// DataStoreCursor.
> +class DataStoreCursorRunnable : public WorkerMainThreadRunnable

This, and everything that inherits from it, should go in DataStoreCursor.cpp

Comment 189

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.6

Polished. Use nsContentUtils::ThreadsafeIsCallerChrome().

Comment 190

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3-1, fix tests to support navigator.getDataStores on worker

Comment 191

[(no longer active)]

Comment on attachment 8378963 [details] [diff] [review]
Part 3-1, fix tests to support navigator.getDataStores on worker

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

Sorry, I'm not a DOM peer.

Comment 192

[Gene Lian (I already quit Mozilla)]

No worry Ehsan! Sorry for my misunderstanding.

Comment 193

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V9

Comment 194

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-2, refactor WorkerMainThreadRunnable, V3.1

Fixed comment #187. Carry on review+.

Comment 195

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread, V2

Comment 196

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch event on workers (WIP)

Comment 197

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8379434 [details] [diff] [review]
Part 2-1, a proxy to resolve/reject Promise on workers, V9

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

\o/

::: dom/promise/Promise.cpp
@@ +1248,5 @@
> +  MutexAutoLock lock(mCleanUpLock);
> +
> +  // |mWorkerPrivate| might not be safe to use anymore if we have already
> +  // cleaned up and RemoveFeature(), so we need to check |mCleanedUp| first.
> +  if (mCleanedUp == true) {

Just if (mCleanedUp).

Comment 198

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8379476 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread, V2

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

This looks pretty good.

::: dom/workers/DataStore.cpp
@@ +13,5 @@
> +
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/dom/PromiseWorkerProxy.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsProxyRelease.h"

You included this in the header.  You don't need it here.

@@ +25,5 @@
> +NS_IMPL_RELEASE_INHERITED(WorkerDataStore, nsDOMEventTargetHelper)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(WorkerDataStore)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)

All you need here is

NS_INTERFACE_MAP_BEGIN(WorkerDataStore)
NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)

You don't need to redeclare nsISupports (the base class handles that), and you don't need the cycle collected version of the BEGIN macro because you didn't add a new cycle collected class (so the base class will handle the cycle collection for nsDOMEventTargetHelper).

@@ +27,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(WorkerDataStore)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> +
> +WorkerDataStore::WorkerDataStore() : nsDOMEventTargetHelper()

new line before ':'

@@ +102,5 @@
> +  MainThreadRun() MOZ_OVERRIDE
> +  {
> +    AssertIsOnMainThread();
> +
> +    nsAutoString string;

Don't use nsAutoString here.  The advantage is that it uses stack storage, but you have to copy it out to mString anyways.  Just use a plain nsString.

@@ +201,5 @@
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    aWorkerPrivate->AssertIsOnWorkerThread();
> +
> +    mId.AppendElements(aId);

This can fail if we're out of memory.

@@ +270,5 @@
> +                                       : nsContentUtils::GetSafeJSContext());
> +    MOZ_ASSERT(cx);
> +
> +    JS::Rooted<JS::Value> value(cx);
> +    if (!mObjBuffer.read(cx, &value)) {

bholley should tell you what the right thing to do with this JSContext is, because I don't know.

::: dom/workers/DataStoreCursor.cpp
@@ +13,5 @@
> +
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/dom/PromiseWorkerProxy.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsProxyRelease.h"

This is in the header.

::: dom/workers/DataStoreCursor.h
@@ +6,5 @@
> +#define mozilla_dom_workers_DataStoreCursor_h
> +
> +#include "nsAutoPtr.h"
> +#include "nsCOMPtr.h"
> +#include "nsCycleCollectionParticipant.h"

I don't think you need any of these headers.

Comment 199

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8379654 [details] [diff] [review]
Part 2-5, a proxy to dispatch event on workers (WIP)

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

This looks reasonable.

::: dom/workers/DataStore.cpp
@@ +676,5 @@
> +    aEvent->GetCancelable(&mCancelable);
> +    aEvent->GetRevisionId(mRevisionId);
> +    aEvent->GetId(mId);
> +    aEvent->GetOperation(mOperation);
> +  }

You don't need to do all of this.  You know what the type, bubbles, and cancelable will be.  You can just get the result values that actually vary.

@@ +702,5 @@
> +      DataStoreChangeEvent::Constructor(workerStore, mType, eventInit);
> +
> +    bool unused;
> +    workerStore->DispatchEvent(static_cast<nsDOMEvent*>(event.get()),
> +                               &unused);

Use DispatchDOMEvent.

@@ +769,5 @@
> +  MutexAutoLock lock(mCleanUpLock);
> +
> +  // |mWorkerPrivate| might not be safe to use anymore if we have already
> +  // cleaned up and RemoveFeature(), so we need to check |mCleanedUp| first.
> +  if (mCleanedUp == true) {

just if (mCleanedUp)

@@ +780,5 @@
> +
> +  // Release the WorkerStore and remove the DataStoreChangeEventProxy from the
> +  // features of the worker thread since the event has been dispatched or the
> +  // worker thread has been cancelled.
> +  mWorkerStore = nullptr;    

whitespace at EOL.

::: dom/workers/DataStoreCursor.cpp
@@ +71,5 @@
>    {
>      MOZ_ASSERT(aWorkerPrivate);
>      aWorkerPrivate->AssertIsOnWorkerThread();
> +
> +    // When we're on the worker thread, prepare an DataStoreChangeEventProxy.

a DataStore...

and no comma

::: dom/workers/Navigator.cpp
@@ +77,4 @@
>    nsRefPtr<WorkerDataStore> workerStore = new WorkerDataStore();
>    nsMainThreadPtrHandle<DataStore> backingStore = dataStoreholder;
> +
> +  // When we're on the worker thread, prepare an DataStoreChangeEventProxy.

here too

Comment 200

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

One thing you should be aware of: I made changes to Promise in https://hg.mozilla.org/integration/mozilla-inbound/rev/e581cbef0dd9.  The constructor now takes an nsIGlobalObject*, and it asserts that you do not pass it null.  You will need to get the worker's nsIGlobalObject, which you can do from mWorkerPrivate->GlobalScope(), and pass that to the Promise constructor.  It may be easier if you store the nsIGlobalObject* on DataStore/DataStoreCursor, rather than fetching the WorkerPrivate every time, but I'm not sure.

Comment 201

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread, V3

Fixed comment #198.

Hi :bholley, as mentioned at in comment #198 by Kyle. Could you please take a look at:

  DataStorePutRunnable::MainThreadRun()
  DataStoreAddRunnable::MainThreadRun()

regarding the structured-clone read as below?

  nsCOMPtr<nsIScriptContext> scriptContext = sgo->GetContext();
  AutoPushJSContext cx(scriptContext ? scriptContext->GetNativeContext()
                                     : nsContentUtils::GetSafeJSContext());
  MOZ_ASSERT(cx);

  JS::Rooted<JS::Value> value(cx);
  if (!mObjBuffer.read(cx, &value)) {
    JS_ClearPendingException(cx);
    mRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
    return true;
  }

Is that correct to use the JSContext this way?

Comment 202

[Bobby Holley (:bholley)]

Comment on attachment 8384354 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread, V3

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

::: dom/workers/DataStore.cpp
@@ +269,5 @@
> +
> +    nsCOMPtr<nsIScriptContext> scriptContext = sgo->GetContext();
> +    AutoPushJSContext cx(scriptContext ? scriptContext->GetNativeContext()
> +                                       : nsContentUtils::GetSafeJSContext());
> +    MOZ_ASSERT(cx);

I don't know what mBackingStore::GetOwner() returns. But assuming it returns an EventTarget in the correct global scope, you should probably just do:

AutoSafeJSContext cx;
JSAutoCompartment ac(cx, mBackingStore.get()->GetOwner()->GetParentObject());

Comment 203

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8384354 [details] [diff] [review]
Part 2-3, dispatch tasks on the worker to the main thread, V3

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

I assumed you didn't change anything except what I told you to.

::: dom/workers/DataStore.cpp
@@ +27,5 @@
> +NS_INTERFACE_MAP_BEGIN(WorkerDataStore)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> +
> +WorkerDataStore::WorkerDataStore()
> +  : nsDOMEventTargetHelper()

You should pass in the WorkerGlobalScope* here to nsDOMEventTargetHelper (you can get it via workerPrivate->Scope() in the ::Constructor).

Comment 204

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.7

Just rebased.

Comment 205

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-0, pass around global for creating Promise value (by baku)

Comment 206

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V9.1

Fixed comment #197.

Comment 207

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-2, refactor WorkerMainThreadRunnable, V3.2

Just rebased.

Comment 208

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread, V3.1

Fixed comment #203 and r=khuey.

Comment 209

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-4, should create array based on the window to return, V1.1

Just rebased.

Comment 210

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch the change event on workers

Fixed comment #199.

Comment 211

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3-1, fix tests to support navigator.getDataStores on worker, V1.1

Just rebased.

Comment 212

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases for running workers (WIP)

Comment 213

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch the change event on workers, V1.1

Fixed a lock issue in the test case.

Comment 214

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-6, a proxy to run HasDataStoreSupport() on the main thread

Need this because of the recent change at Bug 980419.

Comment 215

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8388460 [details] [diff] [review]
Part 2-5, a proxy to dispatch the change event on workers, V1.1

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

r- for the refcounting issue.  Let's work the other things out over email.

::: dom/workers/DataStore.cpp
@@ +796,5 @@
> +{
> +  // Manually addRef/release for this, otherwise DataStoreChangeEventProxy can
> +  // be garbage collected anytime from this point. If it still holds a
> +  // MutexAutoLock, PR_DestroyLock() will assert because it's still locked.
> +  NS_ADDREF_THIS();

We usually use a 'kungFuDeathGrip' smart pointer for this.  e.g. http://mxr.mozilla.org/mozilla-central/search?string=kungFuDeathGrip

And that's better because ...

@@ +804,5 @@
> +    // |mWorkerPrivate| might not be safe to use anymore if we have already
> +    // cleaned up and RemoveFeature(), so we need to check |mCleanedUp| first.
> +    if (mCleanedUp) {
> +      return true;
> +    }

... taking this early return means you will have AddRef()d but not Release()d this.

Comment 216

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch the change event on workers, V2

Fixed the previous comment and fixed the non-thread-safe nsDOMEventTargetHelper issue. Thanks Kyle for the comments and reviews again!

Comment 217

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8388961 [details] [diff] [review]
Part 2-5, a proxy to dispatch the change event on workers, V2

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

I'm not sure if you will still want me to review this after the email I sent you ... if you do, just reset the flag.

Comment 218

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8388961 [details] [diff] [review]
Part 2-5, a proxy to dispatch the change event on workers, V2

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

ok, and now I've read your response.

::: dom/workers/DataStore.cpp
@@ +668,5 @@
> +    : WorkerRunnable(aDataStoreChangeEventProxy->GetWorkerPrivate(),
> +                     WorkerThreadUnchangedBusyCount)
> +    , mDataStoreChangeEventProxy(aDataStoreChangeEventProxy)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());

AssertIsMainThread()

@@ +700,5 @@
> +    // a String type by default (see DataStoreChangeEvent.webidl). This will
> +    // fail the later assignment when the type of value we want to assign is
> +    // actually UnsignedLong.
> +    eventInit.mId.~OwningStringOrUnsignedLong();
> +    eventInit.mId = mId;

Can you file a bug on making unions less crazy?

@@ +786,5 @@
> +DataStoreChangeEventProxy::~DataStoreChangeEventProxy()
> +{
> +  MutexAutoLock lock(mCleanUpLock);
> +
> +  if (NS_IsMainThread() && !mCleanedUp) {

This still doesn't make any sense to me ... how does the DataStore on the main thread Release() the proxy if the proxy is not CleanUp()d?  You should figure out what is really going on here.

@@ +823,5 @@
> +  MutexAutoLock lock(mCleanUpLock);
> +  // If the worker thread's been cancelled we don't need to dispatch the event.
> +  if (mCleanedUp) {
> +    return NS_OK;
> +  }

Checking this here is not sufficient.  The proxy could get CleanUp()d between this and when DispatchDataStoreChangeEvent runs on the worker thread.

@@ +831,5 @@
> +
> +  nsRefPtr<DispatchDataStoreChangeEventRunnable> runnable =
> +    new DispatchDataStoreChangeEventRunnable(this, event);
> +
> +  mWorkerPrivate->Dispatch(runnable);

You should call runnable->Dispatch().  Something like https://hg.mozilla.org/mozilla-central/file/67485526e241/dom/workers/XMLHttpRequest.cpp#l1057.

@@ +842,5 @@
> +DataStoreChangeEventProxy::Notify(JSContext* aCx, Status aStatus)
> +{
> +  // Manually addRef/release for this, otherwise DataStoreChangeEventProxy can
> +  // be garbage collected anytime from this point. If it still holds a
> +  // MutexAutoLock, PR_DestroyLock() will assert because it's still locked.

You can drop this comment, kungFuDeathGrip is a common pattern, and people will know what it means.

Comment 219

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8388462 [details] [diff] [review]
Part 2-6, a proxy to run HasDataStoreSupport() on the main thread

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

::: dom/base/Navigator.cpp
@@ +2262,5 @@
> +  if (!NS_IsMainThread()) {
> +    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +
> +    nsRefPtr<HasDataStoreSupportRunnable> runnable =
> +      new HasDataStoreSupportRunnable(workerPrivate, aCx, aGlobal);

You definitely can't pass the worker cx/global through to a call on the main thread like this.  It'll be rooted on the wrong thread, for one.  You need to write the logic in this function to handle the off main thread case.  You can get the WorkerPrivate from the JSContext, and then the principal from the WorkerPrivate.  You may have to go to the main thread to call GetAppStatus safely, I'm not sure without reading code.

Comment 220

[Gene Lian (I already quit Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #218)
> @@ +823,5 @@
> > +  MutexAutoLock lock(mCleanUpLock);
> > +  // If the worker thread's been cancelled we don't need to dispatch the event.
> > +  if (mCleanedUp) {
> > +    return NS_OK;
> > +  }
> 
> Checking this here is not sufficient.  The proxy could get CleanUp()d
> between this and when DispatchDataStoreChangeEvent runs on the worker thread.

PromiseWorkerProxy might have the same issue. Right?

Comment 221

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch the change event on workers, V3

Fixed comment #218.

Comment 222

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-6, a proxy to run HasDataStoreSupport() on the main thread, V2

Comment 223

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Gene Lian [:gene] (work week Mar. 5 ~ Mar. 7) from comment #220)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #218)
> > @@ +823,5 @@
> > > +  MutexAutoLock lock(mCleanUpLock);
> > > +  // If the worker thread's been cancelled we don't need to dispatch the event.
> > > +  if (mCleanedUp) {
> > > +    return NS_OK;
> > > +  }
> > 
> > Checking this here is not sufficient.  The proxy could get CleanUp()d
> > between this and when DispatchDataStoreChangeEvent runs on the worker thread.
> 
> PromiseWorkerProxy might have the same issue. Right?

Actually, I'm wrong here.  We hold the lock until we call dispatch on the runnable, at which point it is in the worker's queue.  If we CleanUp after that point we will throw away all the runnables in the queue so DispatchDataStoreChangeEvent will never run.  So this and the PromiseWorkerProxy are wrong.  This is why I shouldn't reason about threaded code at 9 PM :)

Comment 224

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8389011 [details] [diff] [review]
Part 2-5, a proxy to dispatch the change event on workers, V3

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

::: dom/workers/DataStore.cpp
@@ +697,5 @@
> +    // of the worker thread), we don't need to dispatch the event because the
> +    // worker thread might be shutting down.
> +    if (!workerStore) {
> +      return true;
> +    }

So as I just said, this isn't actually necessary.  You can remove this, and we can assert that workerStore is not null.  Sorry for the confusion.

@@ +716,5 @@
> +    // initialized by DataStoreChangeEventInit, which will always set |mId| to
> +    // a UnsignedLong type by default (see DataStoreChangeEvent.webidl). This
> +    // will fail the later assignment when the type of value we want to assign
> +    // is actually String.
> +    eventInit.mId.~OwningStringOrUnsignedLong();

Please make the change that bz suggested in the other bug.

::: dom/workers/DataStore.h
@@ +121,5 @@
> +  ~DataStoreChangeEventProxy() {};
> +
> +  WorkerPrivate* mWorkerPrivate;
> +
> +  WorkerDataStore* mWorkerStore;

So like I said in the email yesterday, I'm worried that if this reference is weak that the DataStore can be GCed too early.  For example, if I do

var dataStore = getDataStore(); // pretend this does what you expect
dataStore.onchange = function() { postMessage("ohai main thread"); };
dataStore = null;

and then the garbage collector runs, we still need to see the message get posted to the main thread.  So I think this actually does need to be a strong reference, unless something else besides the JS wrapper is keeping the WorkerDataStore alive.

Comment 225

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8389027 [details] [diff] [review]
Part 2-6, a proxy to run HasDataStoreSupport() on the main thread, V2

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

r=me

Comment 226

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch the change event on workers, V4

Please see the e-mail for why I made this change. Also, we don't need the kungfuDeathGrip because now SetDataStoreChangeEventProxy won't go away immediately when the worker terminates. Thanks for the review again!

Comment 227

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8389580 [details] [diff] [review]
Part 2-5, a proxy to dispatch the change event on workers, V4

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

r=me

::: dom/workers/DataStore.h
@@ +7,5 @@
>  
>  #include "nsDOMEventTargetHelper.h"
>  #include "nsProxyRelease.h"
>  
> +// mozilla/dom/workers/bindings/

No need for this comment.

Comment 228

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm going to hold off on r+ing the last patch because I want to test these patches myself tomorrow.

Comment 229

[Gene Lian (I already quit Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #228)
> I'm going to hold off on r+ing the last patch because I want to test these
> patches myself tomorrow.

Thanks Kyle for the help! I'll also re-base all the patches later for you to test easily.

Btw, part 3-1 is actually not my last patch. I'm still preparing test cases for :baku to review. Also, part 2-0 is just a temporal one to fix the compartment issue in Promise. :baku is helping solve that at bug 974893. We have to fix it before landing.

Comment 230

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.8

Rebased.

Comment 231

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-0, pass around global for creating Promise value (by baku)

Rebased.

Comment 232

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V9.2

Rebased.

Comment 233

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V9.2

Rebased.

Comment 234

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-2, refactor WorkerMainThreadRunnable, V3.3

Rebased.

Comment 235

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread, V3.2

Rebased.

Comment 236

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-4, should create array based on the window to return, V1.2

Rebased.

Comment 237

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch the change event on workers, V4.1

Rebased and fixed comment #227.

Comment 238

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-6, a proxy to run HasDataStoreSupport() on the main thread, V2.1

Rebased and fixed comment #225.

Comment 239

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3-2, test basic cases on workers

Comment 240

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3-3, test sync cases on workers

Comment 241

[Gene Lian (I already quit Mozilla)]

Try server for the test cases: https://tbpl.mozilla.org/?tree=Try&rev=8fbbd2ea335e

Comment 242

[Andrea Marchesini [:baku]]

Comment on attachment 8389717 [details] [diff] [review]
Part 3-2, test basic cases on workers

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

Extra bonus would be to reuse the file_basic.js from dom/datastore/tests, but this is good.

::: dom/datastore/tests/file_basic_worker.html
@@ +24,5 @@
> +      }
> +    }
> +  }
> +
> +  

extra space, extra lines

Comment 243

[Andrea Marchesini [:baku]]

Comment on attachment 8389718 [details] [diff] [review]
Part 3-3, test sync cases on workers

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

same comment of the part 3-2. Can I see it green on try? Thanks!

::: dom/datastore/tests/file_sync_worker.html
@@ +24,5 @@
> +      }
> +    }
> +  }
> +
> +  

extra lines, extra spaces

Comment 244

[Gene Lian (I already quit Mozilla)]

(In reply to Andrea Marchesini (:baku) from comment #243)
> Can I see it green on try? Thanks!

Try looks perfect! https://tbpl.mozilla.org/?tree=Try&rev=8fbbd2ea335e

Comment 245

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8387713 [details] [diff] [review]
Part 3-1, fix tests to support navigator.getDataStores on worker, V1.1

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

r=me

I played with this for a while and didn't see any more problems.

Comment 246

[Gene Lian (I already quit Mozilla)]

Thanks Kyle for putting efforts to test this.

All the patches are ready to land but waiting for solving the compartment issue in Promise (bug 974893) which is our last piece to make everything work. ;)

Comment 247

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Are we ready to land now that bug 974893 is done?

Comment 248

[Gene Lian (I already quit Mozilla)]

I've rebased and tested everything on bug 974893 but it's still not working. I'm talking to Andrea about that.

Comment 249

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, C++ stubs for DataStore and DataStoreCursor, V6.9

Although we still have some compartment issues blocking us to land all the patches, I decided to land the part-1 patch earlier because:

1. Bug 957086 (Rewrite DataStoreService API in C++) has to be on top of this.
2. APIs are changing frequently which makes me hard to rebase the part-1 patch.

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=c8876e56f328

Comment 250

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/6f7f23f8828d

Comment 251

[Gene Lian (I already quit Mozilla)]

Please don't resolve this bug until all the patches are landed. I only want to land part-1 patch at this moment.

Comment 252

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f7f23f8828d

Comment 253

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-1, a proxy to resolve/reject Promise on workers, V10

Rebased and modified codes based on bug 995163.

Comment 254

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-2, refactor WorkerMainThreadRunnable, V3.4

Rebased.

Comment 255

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-3, dispatch tasks on the worker to the main thread, V3.3

Rebased.

Comment 256

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-4, should create array based on the window to return, V1.3

Rebased.

Comment 257

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-5, a proxy to dispatch the change event on workers, V4.2

Rebased.

Comment 258

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2-6, a proxy to run HasDataStoreSupport() on the main thread, V2.2

Rebased.

Comment 259

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3-1, fix tests to support navigator.getDataStores on worker, V1.2

Rebased.

Comment 260

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3-2, test basic cases on workers, V1.1

Rebased.

Comment 261

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3-3, test sync cases on workers, V1.1

Rebased.

Comment 262

[Gene Lian (I already quit Mozilla)]

Thanks to Andrea's support at bug 995163. We're ready to land. :D

Try looks perfect: https://tbpl.mozilla.org/?tree=Try&rev=99ffdb5f45ff

Comment 263

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b60249fb45ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/a61309076059
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5624c2a2d2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46eaebe6572
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c5b716e26c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6bb2cbb91e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbfeafe352c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2962581fca93
https://hg.mozilla.org/integration/mozilla-inbound/rev/27cd1a1fd1d7

Comment 264

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b60249fb45ac
https://hg.mozilla.org/mozilla-central/rev/a61309076059
https://hg.mozilla.org/mozilla-central/rev/c5624c2a2d2f
https://hg.mozilla.org/mozilla-central/rev/b46eaebe6572
https://hg.mozilla.org/mozilla-central/rev/6c5b716e26c8
https://hg.mozilla.org/mozilla-central/rev/8d6bb2cbb91e
https://hg.mozilla.org/mozilla-central/rev/2bbfeafe352c
https://hg.mozilla.org/mozilla-central/rev/2962581fca93
https://hg.mozilla.org/mozilla-central/rev/27cd1a1fd1d7

Comment 265

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

\o/

Comment 266

[Chris Mills [:cmills]]

Covered in https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers

Comment 268

[Chris Mills [:cmills]]

Availability of data store in web workers added to:

https://developer.mozilla.org/en-US/docs/Web/API/Data_Store_API
https://developer.mozilla.org/en-US/docs/Web/API/DataStoreCursor
https://developer.mozilla.org/en-US/docs/Web/API/DataStoreChangeEvent
https://developer.mozilla.org/en-US/docs/Web/API/DataStore
https://developer.mozilla.org/en-US/docs/Web/API/DataStoreTask
https://developer.mozilla.org/en-US/docs/Web/API/Data_Store_API/Using_the_Data_Store_API

For the record, this is also mentioned in the release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/32
https://developer.mozilla.org/en-US/Firefox_OS/Releases/2.0

A quick check/tech review would be nice. Thanks!