Closed Bug 957086 Opened 6 years ago Closed 6 years ago

Rewrite DataStoreService API in C++

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: airpingu, Assigned: baku)

References

Details

Attachments

(4 files, 43 obsolete files)

8.31 KB, patch
Details | Diff | Splinter Review
88.53 KB, patch
Details | Diff | Splinter Review
29.75 KB, patch
Details | Diff | Splinter Review
8.51 KB, patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → amarchesini
Can somebody please explain why we're now working on rewriting DataStore in C++?
(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.
(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?
Attached patch patch 1 - DataStoreService (WIP) (obsolete) — Splinter Review
Attached patch patch 1 - DataStoreService (WIP) (obsolete) — Splinter Review
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.
Attachment #8356679 - Attachment is obsolete: true
Attachment #8359779 - Flags: feedback?(bent.mozilla)
Attachment #8359779 - Flags: feedback?(bent.mozilla) → feedback?(Jan.Varga)
I'm a bit confused, is this going to replace the wrapper thing ?
yes. long run. This is just the 80% of first component.
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.
Attachment #8359779 - Flags: feedback?(Jan.Varga) → feedback+
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.
Attachment #8359779 - Attachment is obsolete: true
It doesn't support OOP at the moment but we have the first results.
Memory leaks to fix.
Smaug, can you give me a feedback about memory management?
It leaks less than yesterday but still it's not at 0.
Attachment #8373343 - Attachment is obsolete: true
Attachment #8374011 - Flags: feedback?(bugs)
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.
Attachment #8374011 - Attachment description: patch 2 - DataStoreService in C++ - WIP → patch 2 - DataStoreService in C++
Attachment #8374011 - Flags: review?(ehsan)
Attachment #8374011 - Flags: review?(Jan.Varga)
Attachment #8374011 - Flags: feedback?(bugs)
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()
Attached patch patch 3 - DataStoreService OOP (obsolete) — Splinter Review
This patch is ready but I want to ask a review only when patch 2 is ok.
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.
Attachment #8374011 - Flags: review?(ehsan) → feedback+
(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 :)
> 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.
Attached patch patch 2 - interdiff (obsolete) — Splinter Review
Attachment #8374011 - Attachment is obsolete: true
Attachment #8374011 - Flags: review?(Jan.Varga)
Attachment #8374847 - Flags: review?(ehsan)
Attachment #8374848 - Flags: review?(Jan.Varga)
(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 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.
Attachment #8374847 - Flags: review?(ehsan) → review+
Attachment #8374847 - Attachment is obsolete: true
Attachment #8374848 - Attachment is obsolete: true
Attachment #8374848 - Flags: review?(Jan.Varga)
Attachment #8375616 - Flags: review?(Jan.Varga)
baku, sorry for the delay, I've been trying to get a better understanding of DataStore API and implementation of it.
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)
Attachment #8375616 - Flags: review?(Jan.Varga)
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.
Attachment #8375616 - Attachment is obsolete: true
Attachment #8394896 - Flags: review?(Jan.Varga)
> 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
Attachment #8394896 - Attachment is obsolete: true
Attachment #8394896 - Flags: review?(Jan.Varga)
Attachment #8394901 - Flags: review?(Jan.Varga)
Rebased + a crash fixed.
Attachment #8397772 - Flags: review?(Jan.Varga)
Attachment #8394901 - Attachment is obsolete: true
Attachment #8394901 - Flags: review?(Jan.Varga)
Attached patch patch 3 - DataStoreService OOP (obsolete) — Splinter Review
Attachment #8374370 - Attachment is obsolete: true
Attachment #8397773 - Flags: review?(Jan.Varga)
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.
Attachment #8373342 - Flags: review+
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.
Attachment #8397772 - Attachment is obsolete: true
Attachment #8397772 - Flags: review?(Jan.Varga)
Attachment #8398523 - Flags: review?(Jan.Varga)
https://tbpl.mozilla.org/?tree=Try&rev=690431eafa9e

Just to share. Fully green on try.
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
Attached patch process pending requests fix (obsolete) — Splinter Review
this fixes the random timeouts for me
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 ?
> 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.
Attachment #8398523 - Attachment is obsolete: true
Attachment #8398978 - Attachment is obsolete: true
Attachment #8398523 - Flags: review?(Jan.Varga)
Attachment #8399531 - Flags: review?(Jan.Varga)
Attached patch patch 3 - DataStoreService OOP (obsolete) — Splinter Review
Attachment #8397773 - Attachment is obsolete: true
Attachment #8397773 - Flags: review?(Jan.Varga)
Attachment #8399532 - Flags: review?(Jan.Varga)
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 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 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 ?
Janv suggested to have a review from IPDL and JS API use.
bent, bz, can you help with this?
Attachment #8399531 - Attachment is obsolete: true
Attachment #8399531 - Flags: review?(Jan.Varga)
Attachment #8399635 - Flags: review?(bzbarsky)
Attachment #8399635 - Flags: review?(bent.mozilla)
Attachment #8399635 - Flags: review?(Jan.Varga)
Attached patch patch 3 - DataStoreService OOP (obsolete) — Splinter Review
Attachment #8399532 - Attachment is obsolete: true
Attachment #8399532 - Flags: review?(Jan.Varga)
Attachment #8399637 - Flags: review?(bent.mozilla)
Attachment #8399637 - Flags: review?(Jan.Varga)
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?
Depends on: 990484
> 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.
Attachment #8399635 - Attachment is obsolete: true
Attachment #8399635 - Flags: review?(bzbarsky)
Attachment #8399635 - Flags: review?(bent.mozilla)
Attachment #8399635 - Flags: review?(Jan.Varga)
Attachment #8399887 - Flags: review?(bzbarsky)
Attachment #8399887 - Flags: review?(bent.mozilla)
Attachment #8399887 - Flags: review?(Jan.Varga)
Blocks: 990550
Summary: Rewrite DataStore API in C++ → Rewrite DataStoreService API in C++
Blocks: 990554
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.
Attachment #8399887 - Flags: review?(bzbarsky) → review-
> 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.
Attachment #8399887 - Attachment is obsolete: true
Attachment #8399887 - Flags: review?(bent.mozilla)
Attachment #8399887 - Flags: review?(Jan.Varga)
Attachment #8400700 - Flags: review?(bzbarsky)
Attachment #8400700 - Flags: review?(bent.mozilla)
Attachment #8400700 - Flags: review?(Jan.Varga)
Attached patch patch 2 - interdiff (obsolete) — Splinter Review
Attachment #8400701 - Flags: review?(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.
Attachment #8400701 - Flags: review?(bzbarsky) → review+
Attachment #8400700 - Flags: review?(bzbarsky) → review+
Depends on: 991753
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"
Attachment #8406083 - Flags: review?(Jan.Varga)
Attached patch patch 3 - DataStoreService OOP (obsolete) — Splinter Review
Attachment #8399637 - Attachment is obsolete: true
Attachment #8400700 - Attachment is obsolete: true
Attachment #8400701 - Attachment is obsolete: true
Attachment #8399637 - Flags: review?(bent.mozilla)
Attachment #8399637 - Flags: review?(Jan.Varga)
Attachment #8400700 - Flags: review?(bent.mozilla)
Attachment #8400700 - Flags: review?(Jan.Varga)
Attachment #8406084 - Flags: review?(bent.mozilla)
Attachment #8406084 - Flags: review?(Jan.Varga)
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);
}
(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().
Attachment #8414497 - Flags: review?(Jan.Varga)
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().
Attachment #8406084 - Flags: review?(Jan.Varga) → review+
Attached patch DataStore OOP - interdiff 2 (obsolete) — Splinter Review
Attachment #8414517 - Flags: review?(Jan.Varga)
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
Attachment #8406084 - Attachment is obsolete: true
Attachment #8406084 - Flags: review?(bent.mozilla)
Attachment #8414518 - Flags: review?(Jan.Varga)
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
Attachment #8373342 - Attachment is obsolete: true
Attachment #8406083 - Attachment is obsolete: true
Attachment #8406083 - Flags: review?(Jan.Varga)
Attachment #8415078 - Flags: review?(Jan.Varga)
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
Attachment #8414497 - Attachment is obsolete: true
Attachment #8414517 - Attachment is obsolete: true
Attachment #8414518 - Attachment is obsolete: true
Attachment #8414497 - Flags: review?(Jan.Varga)
Attachment #8414517 - Flags: review?(Jan.Varga)
Attachment #8414518 - Flags: review?(Jan.Varga)
Attachment #8415080 - Flags: review?(Jan.Varga)
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;
Attachment #8415080 - Flags: review?(Jan.Varga) → review+
Attached patch appendElement() optimization (obsolete) — Splinter Review
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
Attachment #8415078 - Flags: review?(Jan.Varga) → review+
> @@ +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
Flags: needinfo?(bent.mozilla)
Attachment #8415078 - Attachment is obsolete: true
Attachment #8415427 - Flags: review?(Jan.Varga)
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
Attachment #8415080 - Attachment is obsolete: true
Attachment #8415288 - Attachment is obsolete: true
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
The previous patch had a merge problem.
Attachment #8415429 - Attachment is obsolete: true
Attachment #8415435 - Flags: review+
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 ?
Attachment #8415427 - Flags: review?(Jan.Varga) → review+
Attachment #8415427 - Attachment is obsolete: true
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
Attachment #8415852 - Attachment is obsolete: true
This patch got lost.
Attachment #8415435 - Attachment is obsolete: true
Just use bug 990554 to keep track of.
No longer blocks: 916089
Bent, any luck with this NI?
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.
> >    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 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.
Flags: needinfo?(bent.mozilla)
Attached patch interdiff patch 2 (obsolete) — Splinter Review
Attachment #8429885 - Flags: review?(Jan.Varga)
Attachment #8417890 - Attachment is obsolete: true
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
rebased
Attachment #8415853 - Attachment is obsolete: true
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
Attachment #8429898 - Flags: review+
Attachment #8429885 - Flags: review?(Jan.Varga)
https://tbpl.mozilla.org/?tree=Try&rev=d919a4fa24c5

if green, I'll land these patches.
Attachment #8415077 - Attachment is obsolete: true
Attachment #8429885 - Attachment is obsolete: true
Attachment #8429898 - Attachment is obsolete: true
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
Attachment #8429899 - Attachment is obsolete: true
m-i tree is closed at the moment: checkin-needed.
Keywords: checkin-needed
Sorry, it's not green at all. Ignore the checkin-needed
Keywords: checkin-needed
Ok it was just a problem from the merging: https://tbpl.mozilla.org/?tree=Try&rev=dac2d5da1bf3
Attached patch patch 3 - DataStore OOP (obsolete) — Splinter Review
Attachment #8430143 - Attachment is obsolete: true
Keywords: checkin-needed
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))
Keywords: checkin-needed
Those need to be RootedDictionary instances.  Hooray for the rooting analysis!
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
Attachment #8430167 - Attachment is obsolete: true
Depends on: 1018561
Depends on: 1018406
Backed this out for causing bug 1018406: https://hg.mozilla.org/mozilla-central/rev/6a984e21c2ca
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla32 → ---
Attached patch patch 4 - Permissions (obsolete) — Splinter Review
Attachment #8432686 - Flags: review?(ehsan)
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.
Attachment #8432686 - Flags: review?(ehsan) → review+
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.
Attachment #8432686 - Attachment is obsolete: true
Thanks, Andrea!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.