957086 - Rewrite DataStoreService API in C++

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[(no longer active)]

Can somebody please explain why we're now working on rewriting DataStore in C++?

Comment 2

[Andrea Marchesini [:baku]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> Can somebody please explain why we're now working on rewriting DataStore in
> C++?

The plan was to rewrite it soon or late. The Wrapper was a temporary solution and it's almost done.
I guess the rewriting will take more than 1 month but in the meantime gaia devs can play with DataStore in workers.

Comment 3

[(no longer active)]

(In reply to comment #2)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> > Can somebody please explain why we're now working on rewriting DataStore in
> > C++?
> 
> The plan was to rewrite it soon or late. The Wrapper was a temporary solution
> and it's almost done.
> I guess the rewriting will take more than 1 month but in the meantime gaia devs
> can play with DataStore in workers.

But with the wrapper in place, are we going to have any short term gains from this work?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 1 - DataStoreService (WIP)

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 1 - DataStoreService (WIP)

This first patch implements 90% of what DataStoreService.js does.
It's meant to be landed on top of the DataStore wrapper.

What I would like to know is:

1. is the IndexedDB code OK? It works, but maybe we have a better way to implement the same code.

2. the callbacks, runnables, and so on.

Keep in mind that there is a lot of JS code at the moment. This will be removed with the next patches for DataStore and DataStoreCursor.

Comment 6

[Jan Varga [:janv]]

I'm a bit confused, is this going to replace the wrapper thing ?

Comment 7

[Andrea Marchesini [:baku]]

yes. long run. This is just the 80% of first component.

Comment 8

[Jan Varga [:janv]]

Comment on attachment 8359779 [details] [diff] [review]
patch 1 - DataStoreService (WIP)

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

- I would rename the runnables to callbacks or something like that
- there are no RemoveEventListener() calls, I guess you plan to add them

I focused mostly on IDB changes and usage, and I think I didn't see anything horrible :)

::: dom/datastore/DataStoreDB.cpp
@@ +38,5 @@
> +{
> +}
> +
> +nsresult
> +DataStoreDB::CreateFactoryIfNeeded()

EnsureFactory() sounds a bit better

@@ +46,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    ErrorResult error;
> +    mRequest = mFactory->Open(mDatabaseName, DATASTOREDB_VERSION, error);
> +    ENSURE_SUCCESS(error, error.ErrorCode());

Hm, I don't think the Open() call should live here

@@ +57,5 @@
> +DataStoreDB::Open(IDBTransactionMode aMode, const Sequence<nsString>& aDbs,
> +                  DataStoreDBRunnable* aRunnable)
> +{
> +  MOZ_ASSERT(mState == Inactive);
> +  mState = Active;

hm, I don't think setting the state here is right

::: dom/datastore/DataStoreService.h
@@ +125,5 @@
> +  typedef nsTArray<PendingRequest> PendingRequests;
> +  nsClassHashtable<nsStringHashKey, PendingRequests> mPendingRequests;
> +
> +  friend class FirstRevisionIdRunnable;
> +  friend class RevisionAddedEnableStoreRunnable;

these friend declarations could go right before "public:"

::: dom/indexedDB/IDBFactory.cpp
@@ +227,5 @@
>  }
>  
>  // static
>  nsresult
> +IDBFactory::Create(IDBFactory** aFactory)

This is almost exact copy of the method below, we will need to do something with it.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIDataStore instead DataStore.jsm

This first patch implements the nsIDataStore interface. This is a temporary interface useful to rewrite DataStoreService in C++.

Once DataStore will be a WebIDL, we can get rid of this interface.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++ - WIP

It doesn't support OOP at the moment but we have the first results.
Memory leaks to fix.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Smaug, can you give me a feedback about memory management?
It leaks less than yesterday but still it's not at 0.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8374011 [details] [diff] [review]
patch 2 - DataStoreService in C++

Janv, can you review the IDB part? feel free to review all if you want :)

Ehsan, since you reviewed the previous DataStoreService implementation, can you review this patch? It implements DataStoreService but not OOP. This will be part of another patch.

Comment 13

[Jan Varga [:janv]]

Comment on attachment 8374011 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

I think we are not using NS_ENSURE_SUCCESS() anymore.
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

the same probably applies to ENSURE_SUCCESS()

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStoreService OOP

This patch is ready but I want to ask a review only when patch 2 is ok.

Comment 15

[(no longer active)]

Comment on attachment 8374011 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

Looks good overall, thanks for doing this!  I gotta say, it's pretty terrible that we need to use the DOM API for accessing the indexed DB.  :(

I'd r+ a patch which addresses the comments below or explain why they should not be addressed.  Please provide an interdiff on the next iteration.

Thanks!

::: dom/datastore/DataStoreDB.cpp
@@ +47,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    ErrorResult error;
> +    mRequest = mFactory->Open(mDatabaseName, DATASTOREDB_VERSION, error);
> +    ENSURE_SUCCESS(error, error.ErrorCode());

What's ENSURE_SUCCESS? ;-)

@@ +213,5 @@
> +nsresult
> +DataStoreDB::AddEventListeners()
> +{
> +  nsresult rv = mRequest->EventTarget::AddEventListener(NS_LITERAL_STRING("success"),
> +                                               this, false);

Nit: indentation

::: dom/datastore/DataStoreDB.h
@@ +37,5 @@
> +  DataStoreDB(const nsAString& aManifestURL, const nsAString& aName);
> +  ~DataStoreDB();
> +
> +  nsresult
> +  Open(IDBTransactionMode aMode, const Sequence<nsString>& aDb,

General style nit: please put the return types on the same line as the name of functions in declarations

::: dom/datastore/DataStoreService.cpp
@@ +129,5 @@
> +
> +// This DataStoreDBRunnable is called when DataStoreDB opens the DataStore DB.
> +// Then the first revision will be created if it doesn't exist yet.
> +class FirstRevisionIdRunnable MOZ_FINAL : public DataStoreDBRunnable
> +                                        , public nsIDOMEventListener

Note that I just looked over the logic here and did not review the IDB-ism.  I trust that Jan will do that.

Please document the accepted threads for these methods as assertions.  (They're main thread only, right?)

@@ +183,5 @@
> +    if (!type.EqualsASCII("success")) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);

MOZ_UTF16

@@ +198,5 @@
> +    }
> +
> +    MOZ_ASSERT(mTxn);
> +    nsRefPtr<IDBObjectStore> store =
> +      mTxn->ObjectStore(NS_LITERAL_STRING(DATASTOREDB_REVISION), error);

MOZ_UTF16

@@ +311,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!gDataStoreService) {
> +    gDataStoreService = new DataStoreService();
> +    NS_ENSURE_TRUE(gDataStoreService, nullptr);

new cannot return null.

@@ +344,5 @@
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      obs->AddObserver(this, "webapps-clear-data", false);

As far as I can tell, you're not removing this observer.  You should probably do that in Shutdown().

@@ +360,5 @@
> +                                   const nsAString& aOriginURL,
> +                                   const nsAString& aManifestURL,
> +                                   bool aReadOnly)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

I don't think you can just assert this, since it's an XPCOM component which can be called from anything.  The same comment applies to all assertions of this type.  You should probably do these checks at runtime and return error codes if they do not hold.

(The assertions are useful for debugging purposes, but they're not enough.)

@@ +366,5 @@
> +
> +  HashApp* apps = nullptr;
> +  DataStoreInfo* info = nullptr;
> +  if (mStores.Get(aName, &apps) && apps->Get(aAppId, &info)) {
> +    MOZ_ASSERT(false, "This should not happen");

Shouldn't you return an error code if it does happen?

@@ +393,5 @@
> +                                         const nsAString& aManifestURL,
> +                                         bool aReadOnly)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());

Ditto.

@@ +398,5 @@
> +
> +  HashApp* apps = nullptr;
> +  DataStoreInfo* info = nullptr;
> +  if (mAccessStores.Get(aName, &apps) && apps->Get(aAppId, &info)) {
> +    MOZ_ASSERT(false, "This should not happen");

Ditto.

@@ +418,5 @@
> +DataStoreService::GetDataStores(nsIDOMWindow* aWindow,
> +                                const nsAString& aName,
> +                                nsISupports** aDataStores)
> +{
> +  // FIXME This will be a thread-safe method.

Can it ever be?  It needs to stop using a window for one thing.  :-)

@@ +422,5 @@
> +  // FIXME This will be a thread-safe method.
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  MOZ_ASSERT(window);

nsIDOMWindow is not builtinclass, so this QI can actually fail at runtime, therefore you should handle the case where it does.

@@ +471,5 @@
> +DataStoreService::GetDataStoresCreate(nsPIDOMWindow* aWindow, Promise* aPromise,
> +                                      const nsTArray<DataStoreInfo>& aStores)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());

Ditto.

@@ +506,5 @@
> +                                       Promise* aPromise,
> +                                       const nsTArray<DataStoreInfo>& aStores)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());

Ditto.

@@ +525,5 @@
> +
> +  for (uint32_t i = 0; i < aStores.Length(); ++i) {
> +    nsCOMPtr<nsIDataStore> dataStore =
> +      do_CreateInstance("@mozilla.org/dom/datastore;1");
> +    MOZ_ASSERT(dataStore);

You need to check the return value at runtime, an assertion is not enough.

@@ +532,5 @@
> +                                  aStores[i].mManifestURL,
> +                                  aStores[i].mReadOnly);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +
> +    nsCOMPtr<nsIXPConnectWrappedJS> xpcwrappedjs = do_QueryInterface(dataStore);

Please get someone to review the XPConnect/JSAPI usages in your patch before landing it.

@@ +577,5 @@
> +                                    uint32_t aAppId,
> +                                    nsTArray<DataStoreInfo>& aStores)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());

Ditto.

@@ +580,5 @@
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIAppsService> appsService =
> +    do_GetService("@mozilla.org/AppsService;1");

This can fail.

@@ +596,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (status != nsIPrincipal::APP_STATUS_CERTIFIED &&
> +      !Preferences::GetBool("dom.testing.datastore_enabled_for_hosted_apps",
> +                              false)) {

Nit: indentation.

@@ +625,5 @@
> +                                              DataStoreInfo* aInfo,
> +                                              void* aUserData)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());

Ditto.

@@ +727,5 @@
> +  return NS_OK;
> +}
> +
> +// This class is useful to enumerate the add permissions for each app.
> +class AddPermissionsData

Please annotate this with MOZ_STACK_CLASS.  Same with any other classes that are only supposed to be used on the stack (haven't checked all of these helper classes.)

@@ +848,5 @@
> +  nsString permission;
> +  permission.AppendASCII("indexedDB-chrome-");
> +  permission.Append(data->mName);
> +  permission.AppendASCII("|");
> +  permission.Append(aInfo->mManifestURL);

Nit: perhaps encapsulate this in a helper?

@@ +904,5 @@
> +    permission.Append(basePermission);
> +    permission.AppendASCII("-write");
> +
> +    uint32_t perm = nsIPermissionManager::UNKNOWN_ACTION;
> +     rv = pm->TestExactPermissionFromPrincipal(principal, permission.get(),

Nit: indentation

@@ +942,5 @@
> +  }
> +
> +  // Generic permission
> +  uint32_t perm = nsIPermissionManager::UNKNOWN_ACTION;
> +   rv = pm->TestExactPermissionFromPrincipal(principal, basePermission.get(),

Nit: indentation

@@ +972,5 @@
> +  nsRefPtr<FirstRevisionIdRunnable> runnable =
> +    new FirstRevisionIdRunnable(aAppId, aName, aManifestURL);
> +
> +  Sequence<nsString> dbs;
> +  dbs.AppendElement(NS_LITERAL_STRING(DATASTOREDB_REVISION));

Please use MOZ_UTF16 instead.

::: dom/datastore/DataStoreService.h
@@ +8,5 @@
> +#define mozilla_dom_DataStoreService_h
> +
> +#include "nsIDataStoreService.h"
> +#include "nsIObserver.h"
> +#include "nsIMessageManager.h"

You don't need this in the header.

@@ +124,5 @@
> +  void
> +  RemoveCounter(uint32_t aId);
> +
> +  nsClassHashtable<nsStringHashKey, HashApp> mStores;
> +  nsClassHashtable<nsStringHashKey, HashApp> mAccessStores;

Instead of storing a hashtable of hashtables, I think it would be better if we chose a different key type (through nsGenericHashKey perhaps) which would hash a string/appID pair.

::: dom/datastore/moz.build
@@ -14,4 @@
>  EXTRA_COMPONENTS += [
>      'DataStore.js',
>      'DataStore.manifest',
> -    'DataStoreService.js',

You should remove the js file as well.

::: dom/datastore/nsIDataStoreService.idl
@@ -30,5 @@
> -  // - name: DOMString
> -  // - owner: DOMString
> -  // - enabled: true/false - true if this dataStore is ready to be used.
> -  jsval getDataStoresInfo(in DOMString name,
> -                          in unsigned long appId);

Please rev the uuid.

::: dom/webidl/DataStore.webidl
@@ +73,5 @@
>    any data;
>  };
> +
> +// For internal use.
> +dictionary DataStoreRevisionData {

Can you please define this as a struct in C++?  I don't think we want this in the webidl.

@@ +80,5 @@
> +  DOMString operation = "";
> +};
> +
> +// For internal use.
> +dictionary DataStoreEnableMessage {

This is unused as far as I can tell.

Comment 16

[Jan Varga [:janv]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #15)
> Comment on attachment 8374011 [details] [diff] [review]
> patch 2 - DataStoreService in C++
> 
> Review of attachment 8374011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall, thanks for doing this!  I gotta say, it's pretty
> terrible that we need to use the DOM API for accessing the indexed DB.  :(

In theory, with IDB on PBackground, you could write your own child actor for accessing IDB :)

Comment 17

[Andrea Marchesini [:baku]]

> What's ENSURE_SUCCESS? ;-)

An useful macro defined in ErrorResult.h. It works with ErrorResult objects.

> @@ +183,5 @@
> > +    if (!type.EqualsASCII("success")) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +
> > +    mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);
> 
> MOZ_UTF16

This is a const char16_t*. I want nsString.

> I don't think you can just assert this, since it's an XPCOM component which
> can be called from anything.  The same comment applies to all assertions of
> this type.  You should probably do these checks at runtime and return error
> codes if they do not hold.

Right. I did a macro for it and I use it only in the XPCOM entry point (the methods defined in the idl).

> @@ +418,5 @@
> > +DataStoreService::GetDataStores(nsIDOMWindow* aWindow,
> > +                                const nsAString& aName,
> > +                                nsISupports** aDataStores)
> > +{
> > +  // FIXME This will be a thread-safe method.
> 
> Can it ever be?  It needs to stop using a window for one thing.  :-)

It will. Some how. Not in this patch but the main goal is to have DataStore and DataStoreService available in workers.

> @@ +848,5 @@
> > +  nsString permission;
> > +  permission.AppendASCII("indexedDB-chrome-");
> > +  permission.Append(data->mName);
> > +  permission.AppendASCII("|");
> > +  permission.Append(aInfo->mManifestURL);
> 
> Nit: perhaps encapsulate this in a helper?

I just use them here...
 
> ::: dom/webidl/DataStore.webidl
> @@ +73,5 @@
> >    any data;
> >  };
> > +
> > +// For internal use.
> > +dictionary DataStoreRevisionData {
> 
> Can you please define this as a struct in C++?  I don't think we want this
> in the webidl.

I use something similar for Console.webidl and bz reviewed it positively.
The problem is that I need to convert it to a JSObject, and webidl does it for me.

Interdiff + new patch soon.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch 2 - interdiff

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 20

[(no longer active)]

(In reply to Andrea Marchesini (:baku) from comment #17)
> > What's ENSURE_SUCCESS? ;-)
> 
> An useful macro defined in ErrorResult.h. It works with ErrorResult objects.

Interesting, I did not know about that!

> > @@ +183,5 @@
> > > +    if (!type.EqualsASCII("success")) {
> > > +      return NS_ERROR_FAILURE;
> > > +    }
> > > +
> > > +    mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);
> > 
> > MOZ_UTF16
> 
> This is a const char16_t*. I want nsString.

Ah, this is the nsDOMEventTargetHelper method... Ok, then that's fine!

> > I don't think you can just assert this, since it's an XPCOM component which
> > can be called from anything.  The same comment applies to all assertions of
> > this type.  You should probably do these checks at runtime and return error
> > codes if they do not hold.
> 
> Right. I did a macro for it and I use it only in the XPCOM entry point (the
> methods defined in the idl).

Makes sense.

> > @@ +418,5 @@
> > > +DataStoreService::GetDataStores(nsIDOMWindow* aWindow,
> > > +                                const nsAString& aName,
> > > +                                nsISupports** aDataStores)
> > > +{
> > > +  // FIXME This will be a thread-safe method.
> > 
> > Can it ever be?  It needs to stop using a window for one thing.  :-)
> 
> It will. Some how. Not in this patch but the main goal is to have DataStore
> and DataStoreService available in workers.

Yeah.  We will need to adjust the code somehow to make it work with both types of globals.  But that can wait.

> > @@ +848,5 @@
> > > +  nsString permission;
> > > +  permission.AppendASCII("indexedDB-chrome-");
> > > +  permission.Append(data->mName);
> > > +  permission.AppendASCII("|");
> > > +  permission.Append(aInfo->mManifestURL);
> > 
> > Nit: perhaps encapsulate this in a helper?
> 
> I just use them here...

The same pattern is used in DataStoreService::AddPermissions and DataStoreService::AddAccessPermissionsEnumerator.

> > ::: dom/webidl/DataStore.webidl
> > @@ +73,5 @@
> > >    any data;
> > >  };
> > > +
> > > +// For internal use.
> > > +dictionary DataStoreRevisionData {
> > 
> > Can you please define this as a struct in C++?  I don't think we want this
> > in the webidl.
> 
> I use something similar for Console.webidl and bz reviewed it positively.
> The problem is that I need to convert it to a JSObject, and webidl does it
> for me.

That's a good reason.  I'm convinced.  :-)

Comment 21

[(no longer active)]

Comment on attachment 8374847 [details] [diff] [review]
patch 2 - interdiff

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

One general comment: can you please add MOZ_COUNT_CTOR and MOZ_COUNT_DTOR to the constructors and destructors?

r=me with the comments here, removal of DataStoreService.js since you removed it from the build system, and also refactoring the code to construct the permission name into a helper method as I mentioned in comment 20.

Thanks!

::: dom/datastore/DataStoreService.cpp
@@ +46,5 @@
>  
>  // Singleton for DataStoreService.
>  StaticRefPtr<DataStoreService> gDataStoreService;
>  
> +#define ASSERT_PARENT_THREAD()                                              \

Nit: ASSERT_PARENT_PROCESS() seems like a more correct name.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 23

[Jan Varga [:janv]]

baku, sorry for the delay, I've been trying to get a better understanding of DataStore API and implementation of it.

Comment 24

[Jan Varga [:janv]]

Comment on attachment 8375616 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

Sending what I have, I'm not done with DataStoreService.h/cpp

::: dom/datastore/DataStoreDB.cpp
@@ +43,5 @@
> +DataStoreDB::CreateFactoryIfNeeded()
> +{
> +  if (!mFactory) {
> +    nsresult rv = IDBFactory::Create(getter_AddRefs(mFactory));
> +    NS_ENSURE_SUCCESS(rv, rv);

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

@@ +47,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    ErrorResult error;
> +    mRequest = mFactory->Open(mDatabaseName, DATASTOREDB_VERSION, error);
> +    ENSURE_SUCCESS(error, error.ErrorCode());

I'm afraid that we need to use this:
if (NS_WARN_IF(error.Failed())) {
  return error.ErrorCode());
}

@@ +58,5 @@
> +DataStoreDB::Open(IDBTransactionMode aMode, const Sequence<nsString>& aDbs,
> +                  DataStoreDBRunnable* aRunnable)
> +{
> +  MOZ_ASSERT(mState == Inactive);
> +  mState = Active;

what if something fails below ?

@@ +98,5 @@
> +  else if (type.EqualsASCII("blocked")) {
> +    RemoveEventListeners();
> +    NS_WARNING("Opening database request is blocked.");
> +  }
> +

I think you should assert if you get an unknown event.

@@ +173,5 @@
> +    NS_WARNING("Didn't get the object we expected!");
> +    return rv;
> +  }
> +
> +  nsRefPtr<IDBTransaction> txn = mDatabase->Transaction(mDatabases,

mDatabases looks a bit confusing here, mObjectStores ?

@@ +179,5 @@
> +                                                        error);
> +  ENSURE_SUCCESS(error, error.ErrorCode());
> +
> +  mState = Inactive;
> +  mTransaction = txn;

mTransaction = txn.forget();

@@ +193,5 @@
> +
> +  nsresult rv = CreateFactoryIfNeeded();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mRequest = nullptr;

Hm, it's possible that you create a request in CreateFactoryNeeded() and then you immediately null it out here.
I think you should change CreateFactoryIfNeeded() to not call mFactory->Open() and rather do it in DataStoreDB::Open()

@@ +197,5 @@
> +  mRequest = nullptr;
> +  mTransaction = nullptr;
> +
> +  if (mDatabase) {
> +    mDatabase->Close();

this can fail in theory

@@ +203,5 @@
> +  }
> +
> +  ErrorResult error;
> +  IDBOpenDBOptions params;
> +  mFactory->DeleteDatabase(mDatabaseName, params, error);

mFactory->DeleteDatabase(mDatabaseName, IDBOpenDBOptions(), error);

@@ +204,5 @@
> +
> +  ErrorResult error;
> +  IDBOpenDBOptions params;
> +  mFactory->DeleteDatabase(mDatabaseName, params, error);
> +  ENSURE_SUCCESS(error, error.ErrorCode());

A debug only onerror handler might be helpful here.

::: dom/datastore/DataStoreDB.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_DataStoreDB_h
> +#define mozilla_dom_DataStoreDB_h
> +
> +#include "DataStoreRunnable.h"

just forward declare

@@ +43,5 @@
> +  nsresult Delete();
> +
> +  indexedDB::IDBTransaction* Transaction() const
> +  {
> +    MOZ_ASSERT(mTransaction);

I don't see any implementations of DataStoreRunnable, but I guess they will call this method and expect that the transaction is still "open", so I guess you should assert mTransaction->IsOpen() here.

Actually, I'm not sure if this is right at all, an instance of DataStoreDB should be able to create a new transaction from DataStoreRunnable::Run(), no ?

::: dom/datastore/DataStoreRevision.cpp
@@ +27,5 @@
> +{
> +  MOZ_ASSERT(aStore);
> +  MOZ_ASSERT(aRunnable);
> +
> +  mRunnable = aRunnable;

this should go before the final return NS_OK, no ?

@@ +90,5 @@
> +
> +  if (!type.EqualsASCII("success")) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

assert if you don't get "success" event

@@ +91,5 @@
> +  if (!type.EqualsASCII("success")) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mRequest->RemoveEventListener(NS_LITERAL_STRING("success"), this, false);

you are done with mRequest, you can set it to null

::: dom/datastore/DataStoreRevision.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_DataStoreRevision_h
> +#define mozilla_dom_DataStoreRevision_h
> +
> +#include "DataStoreDB.h"

Can you just forward declare classes instead of |#include "DataStoreDB.h"| ?

@@ +18,5 @@
> +class IDBObjectStore;
> +class IDBRequest;
> +}
> +
> +class DataStoreRevision MOZ_FINAL : public nsIDOMEventListener

do you plan to use this class outside DataStoreService.cpp ?

::: dom/datastore/DataStoreRunnable.h
@@ +13,5 @@
> +namespace dom {
> +
> +class DataStoreDB;
> +
> +class DataStoreDBRunnable

this looks more like a callback, not runnable

@@ +23,5 @@
> +
> +  virtual void Run(DataStoreDB* aDb, bool aSuccess) = 0;
> +
> +  NS_IMETHOD_(nsrefcnt) AddRef(void) = 0;
> +  NS_IMETHOD_(nsrefcnt) Release(void) = 0;

Why not put these first ?

@@ +26,5 @@
> +  NS_IMETHOD_(nsrefcnt) AddRef(void) = 0;
> +  NS_IMETHOD_(nsrefcnt) Release(void) = 0;
> +};
> +
> +class DataStoreRevisionRunnable

shouldn't this live in DataStoreRevision.h ?

@@ +36,5 @@
> +
> +  virtual void Run(const nsAString& aRevisionID) = 0;
> +
> +  NS_IMETHOD_(nsrefcnt) AddRef(void) = 0;
> +  NS_IMETHOD_(nsrefcnt) Release(void) = 0;

Why not put these first?

::: dom/datastore/DataStoreService.cpp
@@ +157,5 @@
> +
> +  void
> +  Run(DataStoreDB* aDb, bool aSuccess)
> +  {
> +    MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

This is repeated too many times, can you add a helper method like AssertIsInMainProcess() in BackgroundImpl.cpp ?

::: dom/datastore/DataStoreService.h
@@ +119,5 @@
> +  nsRefPtrHashtable<nsUint32HashKey, RetrieveRevisionsCounter> mPendingCounters;
> +
> +  friend class FirstRevisionIdRunnable;
> +  friend class RetrieveRevisionsCounter;
> +  friend class RevisionAddedEnableStoreRunnable;

You could move these friend class declarations right before |public:\n  NS_DECL_ISUPPORTS|.

::: dom/indexedDB/IDBFactory.cpp
@@ +228,5 @@
>  }
>  
>  // static
>  nsresult
> +IDBFactory::Create(IDBFactory** aFactory)

As I already mentioned this method is almost identical with Create(ContentParent* aContentParent, IDBFactory** aFactory).
We need to refactor it somehow. Actually, I wonder if we could just relax the |NS_ASSERTION(aContentParent, "Null ContentParent!");| assertion in IDBFactory::Create(ContentParent* aContentParent, IDBFactory** aFactory)

Comment 25

[Gene Lian (I already quit Mozilla)]

Just a side note. If the bug 949325 is blocking this (it's pretty much done, though), we can land its part-1 patch first. That part isn't only working for workers but also for making Data Store API C++'ed.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 27

[Andrea Marchesini [:baku]]

> Actually, I'm not sure if this is right at all, an instance of DataStoreDB
> should be able to create a new transaction from DataStoreRunnable::Run(), no
> ?

It does only for the open. This is what is needed at the moment.
Do you see other scenarios?

> > +class DataStoreRevisionRunnable
> 
> shouldn't this live in DataStoreRevision.h ?

It's used in DataStoreService.cpp

Comment 28

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++ - WIP

Comment 29

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Rebased + a crash fixed.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStoreService OOP

Comment 31

[Jan Varga [:janv]]

Comment on attachment 8373342 [details] [diff] [review]
patch 1 - nsIDataStore instead DataStore.jsm

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

::: dom/datastore/DataStore.js
@@ +61,2 @@
>    debug("DataStore created");
> +  this.wrappedJSObject = this;

|wrappedJSObject| is needed only in this patch, Part 2 removes usage of wrappedJSObject, but not the property itself. Do you plan to use it later ?

::: dom/datastore/DataStoreService.js
@@ +351,5 @@
>      for (let i = 0; i < aStores.length; ++i) {
> +      let obj = Cc["@mozilla.org/dom/datastore;1"]
> +                  .createInstance(Ci.nsIDataStore);
> +      if (!obj || !obj.wrappedJSObject) {
> +        dump("Failed to create a DataStore object.\n");

This should be a debug, but you actually removed entire file in patch Part 2.

Comment 32

[Jan Varga [:janv]]

Comment on attachment 8397772 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

Still not done with DataStoreService.cpp/.h

::: dom/datastore/DataStoreCallback.h
@@ +26,5 @@
> +
> +  virtual void Run(DataStoreDB* aDb, bool aSuccess) = 0;
> +};
> +
> +class DataStoreRevisionCallback

Nit: since you decided to leave this callback here, the file should be called DataStoreCallbacks.h :)

::: dom/datastore/DataStoreDB.cpp
@@ +3,5 @@
> +/* 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/. */
> +
> +#include "DataStoreDB.h"

Nit: add an empty line here

@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "DataStoreDB.h"
> +#include "DataStoreCallback.h"
> +

Nit: remove the empty line here

@@ +12,5 @@
> +#include "mozilla/dom/indexedDB/IDBIndex.h"
> +#include "mozilla/dom/indexedDB/IDBObjectStore.h"
> +#include "mozilla/dom/indexedDB/IDBRequest.h"
> +#include "mozilla/dom/IDBDatabaseBinding.h"
> +#include "mozilla/dom/IDBFactoryBinding.h"

Nit: these two should go before |#include "mozilla/dom/indexedDB/IDBDatabase.h"|

also remove the empty line here

@@ +68,5 @@
> +
> +  ErrorResult error;
> +  mRequest = mFactory->Open(mDatabaseName, DATASTOREDB_VERSION, error);
> +  if (NS_WARN_IF(error.Failed())) {
> +    return error.ErrorCode();

You need to update the state here too.

@@ +93,5 @@
> +    return rv;
> +  }
> +
> +  if (type.EqualsASCII("success")) {
> +    RemoveEventListeners();

I think |mState = Inactive| should be placed here. Currently, if DatabaseOpened() fails, it leaves this object in "active" state.

Also, if DatabaseOpened() fails you should call the callback, right?

@@ +98,5 @@
> +    return DatabaseOpened();
> +  }
> +
> +  else if (type.EqualsASCII("upgradeneeded")) {
> +    return UpgradeSchema();

Sorry, I haven't noticed this in my previous review, |return| after |else|.
It seems you can remove all "else" if you add |return NS_OK| to |error| and |blocked| branches. The existing |return NS_OK| should go away too.

@@ +109,5 @@
> +  }
> +
> +  else if (type.EqualsASCII("blocked")) {
> +    RemoveEventListeners();
> +    NS_WARNING("Opening database request is blocked.");

I think this branch should either just warn and wait for getting upgradeneeded/success events (once other IDB database objects stop blocking this request) or remove event listeners and set the state to Inactive and call the callback with |/* aSuccess */ false|.

In all cases, either you get a success or error, the callback should be called and mRequest should be set to nullptr.

@@ +139,5 @@
> +    NS_WARNING("Didn't get the object we expected!");
> +    return rv;
> +  }
> +
> +  JSContext* cxx = cx;

Nit: cxx doesn't look nice

@@ +224,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  mRequest = nullptr;

Once you add |mRequest = nullptr| to success/error handling for open, you don't need to do it here.

::: dom/datastore/DataStoreDB.h
@@ +11,5 @@
> +#include "nsISupportsImpl.h"
> +#include "nsAutoPtr.h"
> +#include "nsString.h"
> +
> +#include "mozilla/dom/IDBTransactionBinding.h"

Alphabetical order and no empty lines ?

::: dom/datastore/DataStoreRevision.cpp
@@ +3,5 @@
> +/* 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/. */
> +
> +#include "DataStoreRevision.h"

put an empty line here

@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "DataStoreRevision.h"
> +#include "DataStoreCallback.h"
> +

remove the empty line here

@@ +7,5 @@
> +#include "DataStoreRevision.h"
> +#include "DataStoreCallback.h"
> +
> +#include "mozilla/dom/indexedDB/IDBObjectStore.h"
> +#include "mozilla/dom/DataStoreBinding.h"

alphabetical order and no empty lines except after cpp's .h file

@@ +41,5 @@
> +
> +  switch (aRevisionType) {
> +    case RevisionVoid:
> +      data.mOperation = NS_LITERAL_STRING("void");
> +      break;

|default:| with an assertion or something wouldn't hurt here

@@ +72,5 @@
> +DataStoreRevision::GenerateUUID(nsAString& aID)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +    do_GetService("@mozilla.org/uuid-generator;1", &rv);

Hmm, getting uuid generator for every revision is a bit inefficient.
What about caching the generator in the data store service ?

::: dom/datastore/DataStoreService.h
@@ +27,5 @@
> +                                 , public nsIObserver
> +{
> +  friend class FirstRevisionIdCallback;
> +  friend class RetrieveRevisionsCounter;
> +  friend class RevisionAddedEnableStoreCallback;

I think FirstRevisionIdCallback and RevisionAddedEnableStoreCallback should be forward declared too. Maybe check with someone else, but it was needed several months ago (for windows IIRC).

::: dom/indexedDB/IDBFactory.h
@@ +74,5 @@
>                           ContentParent* aContentParent,
>                           IDBFactory** aFactory);
>  
> +  // Called when using IndexedDB from a JS component or a JSM.
> +  static nsresult Create(IDBFactory** aFactory);

This patch still adds almost identical Create() method. Please ask bent what bent thinks about it. As I mentioned maybe we can just remove the assertion for content parent in the existing Create() method.

::: ipc/glue/BackgroundParent.h
@@ +42,5 @@
>  bool
>  IsOnBackgroundThread();
>  
> +bool
> +IsMainProcess();

All these BackgroundParent.h, BackgroundImpl.cpp and BackgroundParentImpl.cpp changes should review bent.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 34

[Andrea Marchesini [:baku]]

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

Just to share. Fully green on try.

Comment 35

[Jan Varga [:janv]]

test_oop.html still sometimes fails (not only on windows, it sometimes fails on my mac too)
https://tbpl.mozilla.org/?tree=Try&rev=d572a093a1b7

Comment 36

[Jan Varga [:janv]]

Attachment: process pending requests fix

this fixes the random timeouts for me

Comment 37

[Jan Varga [:janv]]

Comment on attachment 8398523 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

Posting some comments...

Don't you need to remove DataStoreDB.jsm, too ?

::: dom/datastore/DataStoreService.cpp
@@ +807,5 @@
> +
> +void
> +DataStoreService::GeneratePermissionName(nsAString& aPermission,
> +                                         const nsAString& aName,
> +                                         const nsAString& aManifestURL)

Move to the anonymous namespace ?

@@ +1069,5 @@
> +}
> +
> +void
> +DataStoreService::DeleteDatabase(const nsAString& aName,
> +                                 const nsAString& aManifestURL)

Can be moved to the anonymous namespace, then you can also move DeleteDataStoresAppEnumerator.
DeleteDataStoresEnumerator can be moved too, if you remove the private HashApp typedef and use the type directly.

@@ +1080,5 @@
> +}
> +
> +void
> +DataStoreService::RejectPromise(nsPIDOMWindow* aWindow, Promise* aPromise,
> +                                nsresult aRv)

Move to the anonymous namespace ?

@@ +1112,5 @@
> +}
> +
> +void
> +DataStoreService::ResolvePromise(Promise* aPromise,
> +                                 const nsTArray<nsRefPtr<DataStore>>& aStores)

Thhis method is only used in DataStoreService.cpp and it doesn't access any DataStoreService members, so you can move it to the anonymouse namespace.

::: dom/datastore/DataStoreService.h
@@ +8,5 @@
> +#define mozilla_dom_DataStoreService_h
> +
> +#include "nsIDataStoreService.h"
> +#include "nsIObserver.h"
> +#include "nsClassHashtable.h"

Nit: move before |#include "nsIDataStoreService.h"|

@@ +21,5 @@
> +class DataStore;
> +class DataStoreInfo;
> +class PendingRequest;
> +class Promise;
> +class FirstRevisionIdCallback;

Nit: move after |class DataStoreInfo;|

@@ +126,5 @@
> +  nsClassHashtable<nsStringHashKey, PendingRequests> mPendingRequests;
> +
> +  nsRefPtrHashtable<nsUint32HashKey, RetrieveRevisionsCounter> mPendingCounters;
> +
> +  nsCOMPtr<nsIUUIDGenerator> mUuidgen;

Nit: s/mUuidgen/mUUIDGenerator ?

Comment 38

[Andrea Marchesini [:baku]]

> Don't you need to remove DataStoreDB.jsm, too ?

It's still used by DataStore.js. It's going to be removed in the next steps.

Comment 39

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 40

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStoreService OOP

Comment 41

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=4eb67092209d
green on b2g emulator.

Comment 42

[Jan Varga [:janv]]

Comment on attachment 8399531 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

::: dom/datastore/DataStoreService.cpp
@@ +44,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +static uint64_t gCounterID = 0;

anon namespace

@@ +49,5 @@
> +
> +using namespace indexedDB;
> +
> +// Singleton for DataStoreService.
> +StaticRefPtr<DataStoreService> gDataStoreService;

anon namespace

@@ +255,5 @@
> +  Run(const nsAString& aRevisionId)
> +  {
> +    AssertIsInMainProcess();
> +    MOZ_ASSERT(NS_IsMainThread());
> +

I think you should add DataStoreService::Get() and not use gDataStoreService directly, here and elsewhere, except GetOrCreate() and Shutdown() of course.
Also add MOZ_ASSERT() after each ::Get()

@@ +646,5 @@
> +}
> +
> +void
> +DataStoreService::GetDataStoresCreate(nsPIDOMWindow* aWindow, Promise* aPromise,
> +                                      const nsTArray<DataStoreInfo>& aStores)

only one caller and not very nice method name :)

@@ +681,5 @@
> +
> +void
> +DataStoreService::GetDataStoresResolve(nsPIDOMWindow* aWindow,
> +                                       Promise* aPromise,
> +                                       const nsTArray<DataStoreInfo>& aStores)

a better method name would be nice

@@ +734,5 @@
> +    counter->AppendDataStore(cx, dataStoreObj, dataStore);
> +  }
> +}
> +
> +class GetDataStoresInfoData

MOZ_STACK_CLASS, and can be moved to the anonymous namespace

@@ +754,5 @@
> +// name and available for this 'aAppId'.
> +nsresult
> +DataStoreService::GetDataStoresInfo(const nsAString& aName,
> +                                    uint32_t aAppId,
> +                                    nsTArray<DataStoreInfo>& aStores)

GetDataStoreInfos ?
and s/aStores/aInfos ?

@@ +807,5 @@
> +
> +/* static */ PLDHashOperator
> +DataStoreService::GetDataStoresInfoEnumerator(const uint32_t& aAppId,
> +                                              DataStoreInfo* aInfo,
> +                                              void* aUserData)

This method can be moved to the anonymous namespace.
I tested it locally, I'll send you the patch.

@@ +917,5 @@
> +DataStoreService::AddPermissions(uint32_t aAppId,
> +                                 const nsAString& aName,
> +                                 const nsAString& aOriginURL,
> +                                 const nsAString& aManifestURL,
> +                                 bool aReadOnly)

this method has only one caller, maybe you could just merge it with InstallDataStore()

@@ +949,5 @@
> +
> +/* static */ PLDHashOperator
> +DataStoreService::AddPermissionsEnumerator(const uint32_t& aAppId,
> +                                           DataStoreInfo* aInfo,
> +                                           void* userData)

anon namespace

@@ +968,5 @@
> +  return NS_FAILED(data->mResult) ? PL_DHASH_STOP : PL_DHASH_NEXT;
> +}
> +
> +// This class is useful to enumerate the add permissions for each app.
> +class MOZ_STACK_CLASS AddAccessPermissionsData

this class can be in anon namespace

@@ +986,5 @@
> +nsresult
> +DataStoreService::AddAccessPermissions(uint32_t aAppId, const nsAString& aName,
> +                                       const nsAString& aOriginURL,
> +                                       const nsAString& aManifestURL,
> +                                       bool aReadOnly)

again, only one caller

@@ +1006,5 @@
> +
> +/* static */ PLDHashOperator
> +DataStoreService::AddAccessPermissionsEnumerator(const uint32_t& aAppId,
> +                                                 DataStoreInfo* aInfo,
> +                                                 void* userData)

can be in anon namespace

@@ +1031,5 @@
> +nsresult
> +DataStoreService::ResetPermission(uint32_t aAppId, const nsAString& aOriginURL,
> +                                  const nsAString& aManifestURL,
> +                                  const nsAString& aPermission,
> +                                  bool aReadOnly)

can be in anon namespace too

@@ +1148,5 @@
> +// if needed.
> +nsresult
> +DataStoreService::CreateFirstRevisionId(uint32_t aAppId,
> +                                        const nsAString& aName,
> +                                        const nsAString& aManifestURL)

only one caller

@@ +1212,5 @@
> +  return NS_OK;
> +}
> +
> +RetrieveRevisionsCounter*
> +DataStoreService::GetCounter(uint32_t aId) const

this can be merged with RemoveCounter() and it seems RetrieveRevisionsCounter::JSCallback() doesn't need to know about RetrieveRevisionsCounter

Comment 43

[Jan Varga [:janv]]

Comment on attachment 8399532 [details] [diff] [review]
patch 3 - DataStoreService OOP

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

::: dom/datastore/DataStoreService.cpp
@@ +1196,5 @@
> +    for (uint32_t i = 0; i < children.Length(); i++) {
> +      unused << children[i]->SendDataStoreNotify(aAppId, nsAutoString(aName),
> +                                                 nsAutoString(aManifestURL));
> +    }
> +  }

This can be done better. The current loop notifies all child processes, some of them may not be interested. I think we should create a new IPDL subprotocol and only notify all PDataStore actors.
Or DataStoreGetStores() shouldn't be sync and send back an async response when the datastore is ready/enabled.
Andrea convinced me that we can do this in a followup, since the problem already exists in current JS implementation.

Comment 44

[Jan Varga [:janv]]

Comment on attachment 8399531 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

Note, I don't think I'm right person to review XPConnect/JSAPI usage.

::: dom/datastore/DataStoreRevision.cpp
@@ +27,5 @@
> +{
> +  MOZ_ASSERT(aStore);
> +  MOZ_ASSERT(aCallback);
> +
> +  nsRefPtr<DataStoreService> service = DataStoreService::GetOrCreate();

The calling sequence of this method originates in DataStoreService::CreateFirstRevisionId(), so I think you shouldn't call GetOrCrate() here, just Get() and assert ?

::: dom/datastore/DataStoreService.h
@@ +104,5 @@
> +
> +  void RemoveCounter(uint32_t aId);
> +
> +  nsClassHashtable<nsStringHashKey, HashApp> mStores;
> +  nsClassHashtable<nsStringHashKey, HashApp> mAccessStores;

Didn't ehsan suggest to choose a different key type ?

Comment 45

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Janv suggested to have a review from IPDL and JS API use.
bent, bz, can you help with this?

Comment 46

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStoreService OOP

Comment 47

[Boris Zbarsky [:bzbarsky]]

Should ResolvePromise really ignore all exceptions?  Or should it reject the promise if an exception is thrown?  Similar for RejectPromise.

I'll try to read AppendDataStore, JSCallback, RejectPromise, and ResolvePromise, more carefully tomorrow.  In the meantime, a few comments:

1) Can you file a followup bug to get JSAPI to expose a version of JS_IsIdentifier that takes chars+length and then nix the need for a cx in IDB's CreateObjectStore and CreateIndex code?  That seems all the cx is used for there.

2) Please file a followup bug to add a version of IDBRequest::GetResult() that does not take a cx and use it in this new code.  That would make it much more obvious that AutoSafeJSContext is good enough.

3) If this code is supposed to be running in workers, AutoSafeJSContext is wrong.  Should be ThreadsafeAutoSafeJSContext.  If it's not supposed to be running in workers, assert that it's running on mainthread?

4) Why is AutoSafeJSContext correct in AddRevision?  It's creating an object in that compartment, right?  Is that the compartment that object should be created in?

5) Are we just trying to pass undefined as the key to Put() in AddRevision?  If so, JS::UndefinedHandleValue seems better than a temp rooted.

6) Please document that using AutoSafeJSContext in FirstRevisionIdCallback::Run is OK because store->OpenCursor() ignores the cx when range is undefined.  And maybe emphasize that by passing null or something?   And "range" should be JS::UndefinedHandleValue(), in which case you wouldn't even need the AutoSafeJSContext.

7) Please don't use JSVAL_IS_PRIMITIVE(foo).  Use foo.isObject() (which returns the same thing as !JSVAL_IS_PRIMITIVE(foo)) instead.

8) IDBFactory::Create needs a comment about why you're entering the compartment of "global".  It's not at all clear why that needs to happen, unless the IDBFactory constructor uses AutoJSContext?

Comment 48

[Andrea Marchesini [:baku]]

> 2) Please file a followup bug to add a version of IDBRequest::GetResult()
> that does not take a cx and use it in this new code.  That would make it
> much more obvious that AutoSafeJSContext is good enough.

Done in the next version of these patches.

> 3) If this code is supposed to be running in workers, AutoSafeJSContext is
> wrong.  Should be ThreadsafeAutoSafeJSContext.  If it's not supposed to be
> running in workers, assert that it's running on mainthread?

At the moment this code doesn't run on worker. Assertions added.

Comment 49

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8399887 [details] [diff] [review]
patch 2 - DataStoreService in C++

Comment 47 item 7 still applies.  So does item 1.

Why is the "ctx" temporary in DataStoreDB::UpgradeSchema needed?  Just use "cx".

The code in AddRevision still makes no sense; it's just more obfuscated about it.  Now it's taiking a JSContext from caller, but the only caller is passing an AutoSafeJSContext that's presumbly in a system compartment.  Is that the behavior we want?

>+  if (!data.ToObject(aCx, JS::NullPtr(), &value)) {

You need to merge to trunk and drop the JS::NullPtr there.

>+RejectPromise(nsPIDOMWindow* aWindow, Promise* aPromise, nsresult aRv)
>+  JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx));
>+  JSAutoCompartment ac(cx, global);

This code doesn't make sense.  CurrentGlobalOrNull() returns cx->global(), which is cx->compartment_->global_.  In other words, the cx is already in the compartment of the thing returned by that function.

You presumably want to enter some sort of useful compartment here, though I can't tell which one it is.  Perhaps the compartment of the window's JSObject?  That would actually makes some sense for this stuff.

Even better would be not reinventing wheels here.  DOMError is on WebIDL bindings and wrappercached and nsISupports, so it seems like you should be able to do:

  aPromise->MaybeReject(error);

and be done with it, no?

On a side note, I thought we were trying to minimize use of DOMError in new APIs, and that promises should generally be rejected with Error instances (which does not include DOMError).  Is that not the case?  Followup bug OK for this part, but I'd be a bit surprised if the spec for this stuff has a DOMError here.

>+ResolvePromise(Promise* aPromise, const nsTArray<nsRefPtr<DataStore>>& aStores)

This has some of the same issues as RejectPromise, but they aren't as simple to solve because we have no ArgumentToJSValue overload for nsTArray<T> in Promise.h.  Maybe we should add one?  Failing that, at least crib the oner-arg form of Promise::MaybeResolve() would do when writing this code.  That applies to how the object is being wrapped (in particular, unless you know that the DataStores in the list being passed in are all brand-new, WrapObject is wrong), compartments, etc.  But I would prefer an ArgumentToJSValue that works here if we can manage it.

>+  AppendDataStore(JSContext* aCx, DataStore* aDataStore,

I don't think you need the "idValue" temporary; just pass JS::Int32Value(mId) directly to js::SetFunctionNativeReserved.

>+  JSCallback(JSContext* aCx, unsigned aArgc, JS::Value* aVp)

>+                                js::GetFunctionNativeReserved(&args.callee(),
>+                                0));

That's some pretty misleading indentation.  Please fix?

>+    // This keeps a live it self.

"This keeps alive itself"?  But what's "this"?  The comment here could use work.

Is there a reason we can't have RetrieveRevisionId() return a promise, and then just used Promise.all() instead of creating something pretty similar to that here?  Followup bug is ok; we may need to add a version of All() that just takes an nsTArray of promises.

Comment 51

[Andrea Marchesini [:baku]]

> Comment 47 item 7 still applies.  So does item 1.

Bug 990484 is for item 1.

> The code in AddRevision still makes no sense; it's just more obfuscated
> about it.  Now it's taiking a JSContext from caller, but the only caller is
> passing an AutoSafeJSContext that's presumbly in a system compartment.  Is
> that the behavior we want?

This is not clear to me. Let's talk on IRC about this issue.

> Is there a reason we can't have RetrieveRevisionId() return a promise, and
> then just used Promise.all() instead of creating something pretty similar to
> that here?  Followup bug is ok; we may need to add a version of All() that
> just takes an nsTArray of promises.

This is going to change in the next patches. RetrieveRevisionId is implemented in js but once DataStore is rewritten in C++, we'll get rid of this method.

Comment 52

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 53

[Andrea Marchesini [:baku]]

Attachment: patch 2 - interdiff

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8400701 [details] [diff] [review]
patch 2 - interdiff

>+// Note: this code in it must not assume anything about the compartment cx is

s/in it//

r=me.  This makes me much happier.

Comment 55

[Jan Varga [:janv]]

Comment on attachment 8400700 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

More comments coming later today...

::: dom/datastore/DataStoreDB.cpp
@@ +76,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    mState = Inactive;
> +    return rv;
> +  }
> +

It now looks like |mState = Active| can be move right before |mTransactionMode = aMode| and all "mState = Inactive" can be just removed, no ?

@@ +95,5 @@
> +
> +  if (type.EqualsASCII("success")) {
> +    RemoveEventListeners();
> +    mState = Inactive;
> +    DatabaseOpened();

Hm, this is still not right, you're now calling |mCallback->Run(this, true)| even if DatabaseOpened() failed.

@@ +113,5 @@
> +    mCallback->Run(this, false);
> +    mRequest = nullptr;
> +    return NS_OK;
> +  }
> +

"error" and "blocked" is almost identical, I think the warning can be removed, so the handling can look like this:
if (type.EqualsASCII("error") || type.EqualsASCII("blocked")) {
  ...
}

::: dom/datastore/DataStoreDB.h
@@ +10,5 @@
> +#include "nsAutoPtr.h"
> +#include "nsIDOMEventListener.h"
> +#include "nsISupportsImpl.h"
> +#include "nsString.h"
> +#include "mozilla/dom/IDBTransactionBinding.h"

"m" comes before "n"

::: dom/datastore/DataStoreService.cpp
@@ +44,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +static uint64_t gCounterID = 0;

why gCounterID and gDataStoreService can't live in the anonymous namespace ?

@@ +298,5 @@
> +
> +  return NS_OK;
> +}
> +
> +class GetDataStoresInfoData

MOZ_STACK_CLASS and s/GetDataStoresInfoData/GetDataStoreInfosData

@@ +317,5 @@
> +  nsTArray<DataStoreInfo>& mStores;
> +};
> +
> +PLDHashOperator
> +GetDataStoresInfoEnumerator(const uint32_t& aAppId,

s/GetDataStoresInfoEnumerator/GetDataStoreInfosEnumerator

@@ +474,5 @@
> +    AssertIsInMainProcess();
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    MOZ_ASSERT(gDataStoreService);
> +    gDataStoreService->EnableDataStore(mAppId, mName, mManifestURL);

I think you should add DataStoreService::Get() and not use gDataStoreService directly, here and elsewhere, except GetOrCreate() and Shutdown() of course.
Also add MOZ_ASSERT() after each ::Get()

@@ +954,5 @@
> +
> +// Thie method populates 'aStores' with the list of DataStores with 'aName' as
> +// name and available for this 'aAppId'.
> +nsresult
> +DataStoreService::GetDataStoresInfo(const nsAString& aName,

s/GetDataStoresInfo/GetDataStoreInfos

::: dom/datastore/DataStoreService.h
@@ +8,5 @@
> +#define mozilla_dom_DataStoreService_h
> +
> +#include "nsIDataStoreService.h"
> +#include "nsIObserver.h"
> +#include "nsClassHashtable.h"

"C" comes before "I"

Comment 56

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 57

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStoreService OOP

Comment 58

[Jan Varga [:janv]]

Comment on attachment 8406083 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

I'm going to send more comments, no need to send a new patch.

::: dom/datastore/DataStoreDB.cpp
@@ +94,5 @@
> +    RemoveEventListeners();
> +    mState = Inactive;
> +
> +    rv = DatabaseOpened();
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

I believe this should look like this:
rv = DatabaseOpened();
if (NS_WARN_IF(NS_FAILED(rv))) {
  mCallback->Run(this, false);
} else {
  mCallback->Run(this, true);
}

Comment 59

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #43)
> Comment on attachment 8399532 [details] [diff] [review]
> patch 3 - DataStoreService OOP
> 
> Review of attachment 8399532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/datastore/DataStoreService.cpp
> @@ +1196,5 @@
> > +    for (uint32_t i = 0; i < children.Length(); i++) {
> > +      unused << children[i]->SendDataStoreNotify(aAppId, nsAutoString(aName),
> > +                                                 nsAutoString(aManifestURL));
> > +    }
> > +  }
> 
> This can be done better. The current loop notifies all child processes, some
> of them may not be interested. I think we should create a new IPDL
> subprotocol and only notify all PDataStore actors.
> Or DataStoreGetStores() shouldn't be sync and send back an async response
> when the datastore is ready/enabled.
> Andrea convinced me that we can do this in a followup, since the problem
> already exists in current JS implementation.

Ok, since the C++ implementation might change again in relatively near future using PBackground stuff, we can fix this without creating the subprotocol.

ContentParent.h has a member |bool mSendPermissionUpdates| and a getter for it |NeedsPermissionsUpdate()|.
mSendPermissionUpdates is set to false initially. If |RecvReadPermissions()| is called then the bool flag
is set to true. So if a new permission is added and the child already read permissions (the bool flag is set to true), we call |child->SendAddPermisson()|.
The same technique can be used for |sync DataStoreGetStores()| and "async DataStoreNotify()", so we send the notification only if the child ever called SendDataStoreGetStores().

Comment 60

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP - interdiff

Comment 61

[Jan Varga [:janv]]

Comment on attachment 8406084 [details] [diff] [review]
patch 3 - DataStoreService OOP

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

I'd like to see updated patch before landing.

::: dom/datastore/DataStoreService.cpp
@@ +15,5 @@
>  #include "mozilla/Services.h"
>  #include "mozilla/StaticPtr.h"
>  #include "mozilla/dom/DOMError.h"
> +#include "mozilla/dom/ContentChild.h"
> +#include "mozilla/dom/ContentParent.h"

C comes before D

@@ +860,5 @@
>  
>    else {
>      // This method can be called in the child so we need to send a request
>      // to the parent and create DataStore object here.
> +    ContentChild *cc = ContentChild::GetSingleton();

|ContentChild* cc|
actually you don't need the extra variable, but if you really want, rename it to something else, cc evokes cycle collection :)

@@ +875,5 @@
> +    for (uint32_t i = 0; i < array.Length(); ++i) {
> +      DataStoreInfo info(array[i].name(), array[i].originURL(),
> +                         array[i].manifestURL(), array[i].readOnly(),
> +                         array[i].enabled());
> +      stores.AppendElement(info);

Can you append the element first like you do in GetDataStoresFromIPC() and set properties on returned element then. That should be more efficient.

@@ +1160,4 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // Notify the child processes.
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {

IsMainProcess() ?

@@ +1162,5 @@
> +  // Notify the child processes.
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    nsTArray<ContentParent*> children;
> +    ContentParent::GetAll(children);
> +    for (uint32_t i = 0; i < children.Length(); i++) {

Add the |if (child->NeedsDataStoreNotify())| here

@@ +1233,1 @@
>    MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

IsMainProcess()

@@ +1238,5 @@
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  nsTArray<DataStoreInfo> stores;
> +  rv = GetDataStoreInfos(aName, appId, stores);
> +  NS_ENSURE_SUCCESS(rv, false);

NS_ENSURE_SUCCESS -> NS_WARN_IF ...

::: dom/datastore/DataStoreService.h
@@ +10,5 @@
>  #include "nsClassHashtable.h"
>  #include "nsIDataStoreService.h"
>  #include "nsIObserver.h"
>  #include "nsRefPtrHashtable.h"
> +#include "mozilla/dom/PContent.h"

m comes before n

@@ +15,4 @@
>  
>  class nsIUUIDGenerator;
>  class nsPIDOMWindow;
> +class nsIPrincipal;

Put before nsIUUIDGenerator.

@@ +50,5 @@
>    static void Shutdown();
>  
>    nsresult GenerateUUID(nsAString& aID);
>  
> +  bool GetDataStoresFromIPC(const nsAString& aName,

I would use nsresult here

::: dom/ipc/ContentChild.cpp
@@ +669,5 @@
> +                                  const nsString& aManifestURL)
> +{
> +  nsRefPtr<DataStoreService> service =
> +    DataStoreService::GetOrCreate();
> +  if (service) {

The same as in ContentParent::RecvDataStoreGetStores()

@@ +670,5 @@
> +{
> +  nsRefPtr<DataStoreService> service =
> +    DataStoreService::GetOrCreate();
> +  if (service) {
> +    service->EnableDataStore(aAppId, aName, aManifestURL);

Ignoring return value ?

::: dom/ipc/ContentParent.cpp
@@ +1966,5 @@
>  
>  bool
> +ContentParent::RecvDataStoreGetStores(const nsString& aName,
> +                                      const IPC::Principal& aPrincipal,
> +                                      InfallibleTArray<DataStoreSetting>* aValue)

over 80 chars

@@ +1969,5 @@
> +                                      const IPC::Principal& aPrincipal,
> +                                      InfallibleTArray<DataStoreSetting>* aValue)
> +{
> +  nsRefPtr<DataStoreService> service =
> +    DataStoreService::GetOrCreate();

This fits on one line.

@@ +1970,5 @@
> +                                      InfallibleTArray<DataStoreSetting>* aValue)
> +{
> +  nsRefPtr<DataStoreService> service =
> +    DataStoreService::GetOrCreate();
> +  if (service) {

Do you really want to silently return no values if GetOrCreate() failed ?

What about:
if (NS_WARN_IF(!service)) {
  return false;
}

@@ +1971,5 @@
> +{
> +  nsRefPtr<DataStoreService> service =
> +    DataStoreService::GetOrCreate();
> +  if (service) {
> +    return service->GetDataStoresFromIPC(aName, aPrincipal, aValue);

I would let GetDataStoresFromIPC to return nsresult and do:
if (NS_WARN_IF(NS_FAILED(rv))) {
  return false;
}

::: dom/ipc/ContentParent.h
@@ +507,5 @@
>                                                       const bool& aHidden) MOZ_OVERRIDE;
>  
> +    virtual bool RecvDataStoreGetStores(const nsString& aName,
> +                                        const IPC::Principal& aPrincipal,
> +                                        InfallibleTArray<DataStoreSetting>* aValue) MOZ_OVERRIDE;

This needs better wrapping. Take a look at AllocPAsmJSCacheEntryParent().

Comment 62

[Andrea Marchesini [:baku]]

Attachment: DataStore OOP - interdiff 2

Comment 63

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

Comment 64

[Jan Varga [:janv]]

Andrea, could you please submit rebased patches (patch 1,2,3) ?
I just found out that the wrapper for workers landed in the meantime.
Thanks

Comment 65

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIDataStore instead DataStore.jsm

Comment 66

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 67

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

Comment 68

[Jan Varga [:janv]]

Comment on attachment 8415080 [details] [diff] [review]
patch 3 - DataStore OOP

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

::: dom/datastore/DataStoreRevision.h
@@ +12,2 @@
>  #include "nsString.h"
>  #include "jsapi.h"

j comes before n

::: dom/datastore/DataStoreService.cpp
@@ +861,5 @@
>  
>    else {
>      // This method can be called in the child so we need to send a request
>      // to the parent and create DataStore object here.
> +    ContentChild *contentChild = ContentChild::GetSingleton();

* after ContentChild:
|ContentChild* contentChild|

@@ +876,5 @@
> +    for (uint32_t i = 0; i < array.Length(); ++i) {
> +      DataStoreInfo info(array[i].name(), array[i].originURL(),
> +                         array[i].manifestURL(), array[i].readOnly(),
> +                         array[i].enabled());
> +      stores.AppendElement(info);

In my previous comment I wrote:
Can you append the element first like you do in GetDataStoresFromIPC() and set properties on returned element then. That should be more efficient.

I'll submit a patch for that, so it will be clear what I mean

@@ +1167,4 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // Notify the child processes.
> +  if (IsMainProcess()) {

It's a bit weird that you send the notification before the info is actually enabled in the main process. I would probably move this block right before |// Maybe we have some pending request waiting for this DataStore.|

@@ +1181,4 @@
>    {
>      HashApp* apps = nullptr;
>      DataStoreInfo* info = nullptr;
>      if (mStores.Get(aName, &apps) && apps->Get(aAppId, &info)) {

Can it happen that you get a notification and there are no entries for it in the hash tables ?
Should the |if| be converted to an assertion ?

::: dom/ipc/ContentParent.cpp
@@ +1970,5 @@
> +                                    const nsString& aName,
> +                                    const IPC::Principal& aPrincipal,
> +                                    InfallibleTArray<DataStoreSetting>* aValue)
> +{
> +  mSendDataStoreInfos = true;

Why would we want to receive data store info updates if something below failed ?

@@ +1978,5 @@
> +    return false;
> +  }
> +
> +  nsresult rv = service->GetDataStoresFromIPC(aName, aPrincipal, aValue);
> +  return NS_SUCCEEDED(rv);

You don't want a warning here ?

if (NS_WARN_IF(!service)) {
  return false;
}

return true;

::: dom/ipc/ContentParent.h
@@ +160,5 @@
>      bool NeedsPermissionsUpdate() {
>          return mSendPermissionUpdates;
>      }
>  
> +    bool NeedsDataStoreInfos() {

|bool NeedsDataStoreInfos() const {|

@@ +511,5 @@
>                                                       const bool& aHidden) MOZ_OVERRIDE;
>  
> +    virtual bool RecvDataStoreGetStores(const nsString& aName,
> +                                        const IPC::Principal& aPrincipal,
> +                                        InfallibleTArray<DataStoreSetting>* aValue) MOZ_OVERRIDE;

virtual bool RecvDataStoreGetStores(
                       const nsString& aName,
                       const IPC::Principal& aPrincipal,
                       InfallibleTArray<DataStoreSetting>* aValue) MOZ_OVERRIDE;

Comment 69

[Jan Varga [:janv]]

Attachment: appendElement() optimization

Comment 70

[Jan Varga [:janv]]

Comment on attachment 8415078 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

please needinfo/private chat bent about changes in BackgroundImpl.cpp, BackgroundParent.h and BackgroundParentImpl.cpp

hopefully, I caught everything else :)

please submit updated patches before landing

::: dom/datastore/DataStoreCallbacks.h
@@ +19,5 @@
> +public:
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef(void) = 0;
> +  NS_IMETHOD_(MozExternalRefCountType) Release(void) = 0;
> +
> +  virtual ~DataStoreDBCallback()

I think the new policy is to have protected/private destructors for ref counted objects.

@@ +32,5 @@
> +public:
> +  NS_IMETHOD_(MozExternalRefCountType) AddRef(void) = 0;
> +  NS_IMETHOD_(MozExternalRefCountType) Release(void) = 0;
> +
> +  virtual ~DataStoreRevisionCallback()

dtto

::: dom/datastore/DataStoreDB.cpp
@@ +94,5 @@
> +    RemoveEventListeners();
> +    mState = Inactive;
> +
> +    rv = DatabaseOpened();
> +

you can remove this empty line

::: dom/datastore/DataStoreRevision.cpp
@@ +30,5 @@
> +{
> +  MOZ_ASSERT(aStore);
> +  MOZ_ASSERT(aCallback);
> +
> +  nsRefPtr<DataStoreService> service = DataStoreService::GetOrCreate();

I still think that you don't need GetOrCreate() here, just Get()
I ran DS mochitests with Get() and it worked fine.

Also, you can now update Navigator::GetDataStores() to get the service using ::GetOrCreate and not:
do_GetService("@mozilla.org/datastore-service;1");

::: dom/datastore/DataStoreService.cpp
@@ +39,5 @@
> +#include "nsContentUtils.h"
> +#include "nsServiceManagerUtils.h"
> +#include "nsThreadUtils.h"
> +#include "nsXULAppAPI.h"
> +#include "nsNetCID.h"

Hm, this list of includes isn't sorted and the first one |#include "DataStoreService.h"| should be separated by an empty line.
It seems that we're leaving the 4 section schema (interface files, other includes, local includes, ...), so you should just have one sorted list.

@@ +48,5 @@
> +namespace dom {
> +
> +using namespace indexedDB;
> +
> +#define ASSERT_PARENT_PROCESS()                                             \

move the #define before |namespace mozilla {| |

@@ +104,5 @@
> +typedef nsClassHashtable<nsUint32HashKey, DataStoreInfo> HashApp;
> +
> +void
> +RejectPromise(nsPIDOMWindow* aWindow, Promise* aPromise, nsresult aRv)
> +{

It would be nicer to use empty lines like this:
MOZ_ASSERT(NS_IsMainThread());
MOZ_ASSERT(NS_FAILED(aRv));

nsRefPtr<DOMError> error;
if (aRv == NS_ERROR_DOM_SECURITY_ERR) {
...

@@ +125,5 @@
> +void
> +DeleteDatabase(const nsAString& aName,
> +               const nsAString& aManifestURL)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

AssertIsMainProcess()

@@ +138,5 @@
> +                             const uint32_t& aAppId,
> +                             nsAutoPtr<DataStoreInfo>& aInfo,
> +                             void* aUserData)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

AssertIsMainProcess()

@@ +141,5 @@
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  uint32_t* appId = static_cast<uint32_t*>(aUserData);

you can use |auto*| here

@@ +155,5 @@
> +DeleteDataStoresEnumerator(const nsAString& aName,
> +                           nsAutoPtr<HashApp>& aApps,
> +                           void* aUserData)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

AssertIsMainProcess()

@@ +179,5 @@
> +                const nsAString& aManifestURL,
> +                const nsAString& aPermission,
> +                bool aReadOnly)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

AssertIsMainProcess()

@@ +235,5 @@
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +    }
> +

remove this empty line

@@ +242,5 @@
> +                                nsIPermissionManager::ALLOW_ACTION,
> +                                nsIPermissionManager::EXPIRE_NEVER, 0);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }

you can replace those two |if (NS_WARN_IF(NS_FAILED(rv))) { return rv }| with just one, put it after if/else block

@@ +267,5 @@
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +    }
> +  }

I don't understand why you need these { } scopes

@@ +312,5 @@
> +GetDataStoreInfosEnumerator(const uint32_t& aAppId,
> +                            DataStoreInfo* aInfo,
> +                            void* aUserData)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

AssertIsInMainProcess()

@@ +315,5 @@
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  GetDataStoreInfosData* data = static_cast<GetDataStoreInfosData*>(aUserData);

auto* data = ...

@@ +359,5 @@
> +AddPermissionsEnumerator(const uint32_t& aAppId,
> +                         DataStoreInfo* aInfo,
> +                         void* userData)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

AssertIsInMainProcess()

@@ +362,5 @@
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  AddPermissionsData* data = static_cast<AddPermissionsData*>(userData);

auto* data = ...

@@ +394,5 @@
> +AddAccessPermissionsEnumerator(const uint32_t& aAppId,
> +                               DataStoreInfo* aInfo,
> +                               void* userData)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

AssertIsInMainProcess()

@@ +398,5 @@
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  AddAccessPermissionsData* data =
> +    static_cast<AddAccessPermissionsData*>(userData);

auto* data = ...

@@ +434,5 @@
> +  nsCOMPtr<nsPIDOMWindow> mWindow;
> +  nsRefPtr<Promise> mPromise;
> +  nsTArray<DataStoreInfo> mStores;
> +
> +  // This is contains the list of manifestURLs of the DataStores that are not

This array ... ?

@@ +603,5 @@
> +// This class calls the 'retrieveRevisionId' method of the DataStore object for
> +// any DataStore in the 'mResults' array. When all of them are called, the
> +// promise is resolved with 'mResults'.
> +// The reson why this has to be done is because DataStore are object that can be
> +// created in any thread and in any process. The first revision has been crated,

created

@@ +665,5 @@
> +
> +    nsRefPtr<DataStoreService> service = DataStoreService::Get();
> +    MOZ_ASSERT(service);
> +
> +    nsRefPtr<RetrieveRevisionsCounter> counter =service->GetCounter(id);

add a space after =

@@ +741,5 @@
> +
> +  if (IsMainProcess()) {
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      obs->AddObserver(this, "webapps-clear-data", false);

Since AddObserver can fail in theory you shouldn't call it in the constructor. You need to add Init() method ...

@@ +971,5 @@
> +DataStoreService::GetDataStoreInfos(const nsAString& aName,
> +                                    uint32_t aAppId,
> +                                    nsTArray<DataStoreInfo>& aStores)
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

This is changed to MOZ_ASSERT(IsMainProcess()) in patch 3, but that can be just AssertIsInMainProcess() here and maybe elsewhere.

@@ +1198,5 @@
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsRefPtr<RetrieveRevisionsCounter> counter;
> +  return mPendingCounters.Get(aId, getter_AddRefs(counter)) ? counter : nullptr;

uhm, this should return counter.forget(), no ?
and the return type needs to be already_AddRefed<...>

@@ +1230,5 @@
> +  }
> +
> +  char chars[NSID_LENGTH];
> +  id.ToProvidedString(chars);
> +  aID.Assign(NS_ConvertASCIItoUTF16(chars));

CopyASCIItoUTF16() ?

::: dom/indexedDB/IDBFactory.cpp
@@ +233,5 @@
>                     IDBFactory** aFactory)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(IndexedDatabaseManager::IsMainProcess(), "Wrong process!");
>    NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Only for chrome!");

please needinfo/private msg bent about this

Comment 71

[Andrea Marchesini [:baku]]

> @@ +267,5 @@
> > +      if (NS_WARN_IF(NS_FAILED(rv))) {
> > +        return rv;
> > +      }
> > +    }
> > +  }
> 
> I don't understand why you need these { } scopes

because I don't want to reuse the permission string.

> ::: dom/indexedDB/IDBFactory.cpp
> @@ +233,5 @@
> >                     IDBFactory** aFactory)
> >  {
> >    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> >    NS_ASSERTION(IndexedDatabaseManager::IsMainProcess(), "Wrong process!");
> >    NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Only for chrome!");
> 
> please needinfo/private msg bent about this

Bent, can you please take a quick look at the changes I have done in the:

1. dom/indexedDB/IDBFactory.cpp - patch 2
2. ipc/glue/BackgroundImpl.* - patch 2

Comment 72

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 73

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

Comment 74

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

The previous patch had a merge problem.

Comment 75

[Jan Varga [:janv]]

Comment on attachment 8415427 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

::: dom/datastore/DataStoreService.cpp
@@ +233,5 @@
> +    if (aReadOnly && perm == nsIPermissionManager::ALLOW_ACTION) {
> +      rv = pm->RemoveFromPrincipal(principal, permission.get());
> +    }
> +
> +    else if (!aReadOnly && perm != nsIPermissionManager::ALLOW_ACTION) {

sorry, is that empty line above needed ?

@@ +431,5 @@
> +  nsCOMPtr<nsPIDOMWindow> mWindow;
> +  nsRefPtr<Promise> mPromise;
> +  nsTArray<DataStoreInfo> mStores;
> +
> +  // This array is contains the list of manifestURLs of the DataStores that are

without *is* ?

@@ +742,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +}
> +
> +DataStoreService::~DataStoreService()
> +{

maybe assert the main thread here too

@@ +757,5 @@
> +  if (!obs) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  obs->AddObserver(this, "webapps-clear-data", false);

check return value here ?

Comment 76

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 77

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

Comment 78

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

This patch got lost.

Comment 79

[Gene Lian (I already quit Mozilla)]

Just use bug 990554 to keep track of.

Comment 80

[Andrea Marchesini [:baku]]

Bent, any luck with this NI?

Comment 81

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

Comment on attachment 8417890 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

::: dom/indexedDB/IDBFactory.cpp
@@ +233,5 @@
>                     IDBFactory** aFactory)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(IndexedDatabaseManager::IsMainProcess(), "Wrong process!");
>    NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Only for chrome!");

I am missing context here. Why is it necessary to remove these?

::: ipc/glue/BackgroundParent.h
@@ +63,5 @@
>  bool
>  IsOnBackgroundThread();
>  
> +bool
> +IsMainProcess();

It doesn't really make sense to expose this... BackgroundParent stuff is *always* in the main process.

Comment 82

[Andrea Marchesini [:baku]]

> >    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> >    NS_ASSERTION(IndexedDatabaseManager::IsMainProcess(), "Wrong process!");
> >    NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Only for chrome!");
> 
> I am missing context here. Why is it necessary to remove these?

-  NS_ASSERTION(aContentParent, "Null ContentParent!");
-  NS_ASSERTION(!nsContentUtils::GetCurrentJSContext(), "Should be called from C++");

I remove these 2 because I'm using the IDBFactory::Create without aContentParent.
nsContextUtils::GetCurrentJSContext() fails because DataStoreService is called from JS.

The first implementation was duplicating IDBFActory::Create but when I and Janv agreed to merge the 2 implementations removing these 2 checks.

> > +bool
> > +IsMainProcess();
> 
> It doesn't really make sense to expose this... BackgroundParent stuff is
> *always* in the main process.

Indeed, but I use that has an helper to check if DataStoreService is in the main process.

Comment 83

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

Comment on attachment 8417890 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

::: dom/indexedDB/IDBFactory.cpp
@@ -236,5 @@
>    NS_ASSERTION(IndexedDatabaseManager::IsMainProcess(), "Wrong process!");
>    NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Only for chrome!");
> -  NS_ASSERTION(aContentParent, "Null ContentParent!");
> -
> -  NS_ASSERTION(!nsContentUtils::GetCurrentJSContext(), "Should be called from C++");

Ok, I understand.

::: ipc/glue/BackgroundParent.h
@@ +63,5 @@
>  bool
>  IsOnBackgroundThread();
>  
> +bool
> +IsMainProcess();

Yeah, you should just have your own helpers rather than expose these here.

Comment 84

[Andrea Marchesini [:baku]]

Attachment: interdiff patch 2

Comment 85

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 86

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

rebased

Comment 87

[Jan Varga [:janv]]

Comment on attachment 8429898 [details] [diff] [review]
patch 2 - DataStoreService in C++

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

Ok, it seems I overlooked some NS_ENSURE_TRUEs

for example:
NS_ENSURE_TRUE(appsService, NS_ERROR_FAILURE);
should be changed to:
if (NS_WARN_IF(!appsService)) {
  return NS_ERROR_FAILURE;
}

::: dom/datastore/DataStoreService.cpp
@@ +42,5 @@
> +#include "nsThreadUtils.h"
> +#include "nsXULAppAPI.h"
> +
> +bool
> +IsMainProcess()

Can't this live in the anonymous namespace below ? Maybe before RejectPromise()

@@ +50,5 @@
> +  return isMainProcess;
> +}
> +
> +void
> +AssertIsInMainProcess()

see the comment above

Comment 88

[Andrea Marchesini [:baku]]

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

if green, I'll land these patches.

Comment 89

[Andrea Marchesini [:baku]]

Attachment: patch 1 - nsIDataStore instead DataStore.jsm

Comment 90

[Andrea Marchesini [:baku]]

Attachment: patch 2 - DataStoreService in C++

Comment 91

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

Comment 92

[Andrea Marchesini [:baku]]

m-i tree is closed at the moment: checkin-needed.

Comment 93

[Andrea Marchesini [:baku]]

Sorry, it's not green at all. Ignore the checkin-needed

Comment 94

[Andrea Marchesini [:baku]]

Ok it was just a problem from the merging: https://tbpl.mozilla.org/?tree=Try&rev=dac2d5da1bf3

Comment 95

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

Comment 96

[Ryan VanderMeulen [:RyanVM]]

AFAICT, the Try run is showing a legitimate rooting hazard regression.

Function '_ZN7mozilla3dom11DataStoreDB13UpgradeSchemaEv|uint32 mozilla::dom::DataStoreDB::UpgradeSchema()' has unrooted 'params' of type 'mozilla::dom::IDBObjectStoreParameters' live across GC call '_ZN7mozilla3dom24IDBObjectStoreParameters4InitERK18nsAString_internal|uint8 mozilla::dom::IDBObjectStoreParameters::Init(nsAString_internal*)' at dom/datastore/DataStoreDB.cpp:148
    dom/datastore/DataStoreDB.cpp:148: Call(46,47, __temp_15.~nsLiteralString())
    dom/datastore/DataStoreDB.cpp:151: Call(47,48, __temp_18 := cx.field:0.operator 258())
    dom/datastore/DataStoreDB.cpp:151: Call(48,49, __temp_19*.nsLiteralString(0,D"D\u0000a\u0000t\u0000a\u0000S\u0000t\u0000o\u0000r\u0000e\u0000D\u0000B\u0000\u0000\u0000"))
    dom/datastore/DataStoreDB.cpp:151: Call(49,50, __temp_17 := database*.CreateObjectStore(__temp_18*,__temp_19.field:0.field:0,params,error))

Function '_ZN7mozilla3dom11DataStoreDB13UpgradeSchemaEv|uint32 mozilla::dom::DataStoreDB::UpgradeSchema()' takes unsafe address of unrooted 'params' at dom/datastore/DataStoreDB.cpp:151
    dom/datastore/DataStoreDB.cpp:151: Call(49,50, __temp_17 := database*.CreateObjectStore(__temp_18*,__temp_19.field:0.field:0,params,error))

Function '_ZN7mozilla3dom11DataStoreDB13UpgradeSchemaEv|uint32 mozilla::dom::DataStoreDB::UpgradeSchema()' has unrooted 'params:1' of type 'mozilla::dom::IDBObjectStoreParameters' live across GC call '_ZN7mozilla3dom24IDBObjectStoreParameters4InitERK18nsAString_internal|uint8 mozilla::dom::IDBObjectStoreParameters::Init(nsAString_internal*)' at dom/datastore/DataStoreDB.cpp:161
    dom/datastore/DataStoreDB.cpp:161: Call(68,69, __temp_22.~nsLiteralString())
    dom/datastore/DataStoreDB.cpp:165: Call(69,70, __temp_24 := cx.field:0.operator 258())
    dom/datastore/DataStoreDB.cpp:165: Call(70,71, __temp_25*.nsLiteralString(0,D"r\u0000e\u0000v\u0000i\u0000s\u0000i\u0000o\u0000n\u0000\u0000\u0000"))
    dom/datastore/DataStoreDB.cpp:165: Call(71,72, __temp_23 := database*.CreateObjectStore(__temp_24*,__temp_25.field:0.field:0,params:1,error))

Function '_ZN7mozilla3dom11DataStoreDB13UpgradeSchemaEv|uint32 mozilla::dom::DataStoreDB::UpgradeSchema()' takes unsafe address of unrooted 'params:1' at dom/datastore/DataStoreDB.cpp:165
    dom/datastore/DataStoreDB.cpp:165: Call(71,72, __temp_23 := database*.CreateObjectStore(__temp_24*,__temp_25.field:0.field:0,params:1,error))

Comment 97

[Boris Zbarsky [:bzbarsky]]

Those need to be RootedDictionary instances.  Hooray for the rooting analysis!

Comment 98

[Andrea Marchesini [:baku]]

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef379607cff7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4cbc8cf0b619
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/083b9fb75e9a

Comment 99

[Ryan VanderMeulen [:RyanVM]]

Backed out for Win7/Win8 debug mochitest crashes. Please wait on your Try run to complete before pushing to inbound. Not doing so defeats the entire purpose.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7edac9664a

https://tbpl.mozilla.org/php/getParsedLog.php?id=40650664&tree=Mozilla-Inbound

Comment 100

[Andrea Marchesini [:baku]]

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

Comment 101

[Andrea Marchesini [:baku]]

Attachment: patch 3 - DataStore OOP

Comment 102

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3b45335d4694
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b69bcaa24ab5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b6dab2f985

Comment 103

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b45335d4694
https://hg.mozilla.org/mozilla-central/rev/b69bcaa24ab5
https://hg.mozilla.org/mozilla-central/rev/f8b6dab2f985

Comment 104

[(no longer active)]

Backed this out for causing bug 1018406: https://hg.mozilla.org/mozilla-central/rev/6a984e21c2ca

Comment 105

[Andrea Marchesini [:baku]]

Attachment: patch 4 - Permissions

Comment 106

[(no longer active)]

Comment on attachment 8432686 [details] [diff] [review]
patch 4 - Permissions

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

Why was this not detected by our unit tests?  Please also add a test that would fail before this patch and would pass afterwards.  r=me with that.

Comment 107

[Andrea Marchesini [:baku]]

Attachment: patch 4 - Permissions

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

Here a test for this. The reason why we didn't see this issue is because in our test we always install the owner of the datastores and then the app that wants to have access to them.
Plus, in many builds we don't have MOZ_CHILD_PERMISSIONS defined.

Comment 108

[Andrea Marchesini [:baku]]

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1a3e1fe6de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d296d4109a41
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/641defc7ab22
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f02eab0ae3

Comment 109

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c1a3e1fe6de
https://hg.mozilla.org/mozilla-central/rev/d296d4109a41
https://hg.mozilla.org/mozilla-central/rev/641defc7ab22
https://hg.mozilla.org/mozilla-central/rev/c1f02eab0ae3

Comment 110

[(no longer active)]

Thanks, Andrea!