666693 - Remote IndexedDB for multiple IndexedDB-using processes

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Benjamin Smedberg]

IndexDB got hacked to work with a single content process in bug 616494, but multiple content processes are a priority for desktop Firefox, so we have to fix this the "right way" by remoting the database access to the chrome process. This may also require some additional synchronization, because content from the same domain may be making indexdb requests from multiple content processes.

Comment 1

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

Are multiple content processes to a state where I would be able to test racing writes to the same DB from different processes?

Comment 2

[Benjamin Smedberg]

In the e10s repo, yes. That hasn't merged to m-c because of trees and other stuff.

Comment 3

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

I disagree that this needs security review.

Comment 4

[Justin Lebar (not reading bugmail)]

You can get an easy startup assertion by starting a b2g build with the patch in bug 749018 and a recent Gaia.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I learned from bent that IDB-from-workers is on the radar too.  Let's implement the cross-context communication side of this by a PIndexedDB (or whatever) protocol.  We can use cross-thread IPDL support to do worker<-->main-thread comm in single-process with the same code that does content-process<-->chrome-process.  It also allows us to do worker-thread<-->chrome-process direct comm with no other changes.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is extremely high priority.  We need to find a victim^Wassignee.  Let's chat next week.

It blocks all of b2g multi-process architecture except (in a limited way) tabs in browser apps.

Comment 7

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

Kyle is on it :)

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bent, where are your WIP patch(es) from last week's work week?  Don't bogart the good stuff! ;)

Comment 9

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

Attachment: Snapshot 1

First snapshot. Databases load, version upgrades can happen, object stores and indexes can be created/deleted. Additional transactions can be created.

Working on Add/Put now.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

!  Nice

Comment 11

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

Attachment: Snapshot 2

Now all object store and index methods are implemented with the exception of cursors. Those are next.

Comment 12

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

Attachment: Snapshot 3

Everything should be implemented now. Works fine on my test page, but I'm sure there are bugs lurking somewhere. And some cleanup maybe. I'll work on getting tests going now, but this is ready to start looking at.

Comment 13

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

Attachment: Snapshot 4

All in-process tests pass with this. Working on out-of-process tests.

Comment 14

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

Attachment: Patch, v1

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 628081 [details] [diff] [review]
Patch, v1

I have very few comments because
 - we discussed the design quite a bit at the work week, so nothing is
   architecturally suprising here
 - we discussed the implementation at a high level for common IPC bugs
   I would normally flag (not your first rodeo!)
 - I barely understand the meat of the IDB code.

So take my r+ for what it's worth.

>diff --git a/dom/indexedDB/AsyncConnectionHelper.cpp b/dom/indexedDB/AsyncConnectionHelper.cpp

>+    ChildProcessSendResult sendResult =
>+      IndexedDatabaseManager::IsMainProcess() ?
>+      MaybeSendResponseToChildProcess(mResultCode) :
>+      Success_NotSent;
>+

I would recommend striking mentions of "ChildProcess" since this
should be mostly process/thread-neutral.

>diff --git a/dom/indexedDB/ipc/IndexedDBParent.cpp b/dom/indexedDB/ipc/IndexedDBParent.cpp

>+void
>+IndexedDBParent::ActorDestroy(ActorDestroyReason aWhy)
>+{
>+  // XXXbent Need to invalidate transactions from this process...

I'm not exactly sure what this means.  Is this something not taken
care of by subprotocol ActorDestroy()?

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp

>+bool
>+TabParent::RecvPIndexedDBConstructor(PIndexedDBParent* aActor,
>+                                     const nsCString& aASCIIOrigin,
>+                                     bool* aAllowed)
>+{
>+  // XXX Security checks, pref checks.

What's missing here?

>+  nsCOMPtr<nsINode> node = do_QueryInterface(GetOwnerElement());
>+  NS_ENSURE_TRUE(node, false);
>+
>+  nsIDocument* doc = node->GetOwnerDocument();
>+  NS_ENSURE_TRUE(doc, false);
>+
>+  nsCOMPtr<nsPIDOMWindow> window = doc->GetInnerWindow();
>+  NS_ENSURE_TRUE(window, false);
>+

This code makes me a little nervous ... are you sure these things are
always guaranteed to be findable?  I guess race conditions on shutdown
would just trigger content-process termination, which would happen
soon anyway.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 628081 [details] [diff] [review]
Patch, v1

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

I really think it would have been much simpler to remote the database communication rather than remoting the whole DOM API. But it's not worth rewriting to do that at this point.


Anyhow, please document all the things.

r+ with that fixed.

::: dom/base/nsGlobalWindow.cpp
@@ +8062,5 @@
>      }
>  
> +    nsRefPtr<indexedDB::IndexedDatabaseManager> mgr =
> +      indexedDB::IndexedDatabaseManager::GetOrCreate();
> +    NS_ENSURE_TRUE(mgr, false);

Document why this is needed

::: dom/indexedDB/AsyncConnectionHelper.h
@@ +44,5 @@
> +    Error
> +  };
> +
> +  virtual ChildProcessSendResult
> +  MaybeSendResponseToChildProcess(nsresult aResultCode) = 0;

Just return a bool

::: dom/indexedDB/IDBCursor.cpp
@@ -565,5 @@
>  
> -  if (!key.IsUnset()) {
> -    switch (mDirection) {
> -      case IDBCursor::NEXT:
> -      case IDBCursor::NEXT_UNIQUE:

Let's keep this stuff here. No need to run these checks in the parent process too.

::: dom/indexedDB/IDBCursor.h
@@ +58,5 @@
>      NEXT_UNIQUE,
>      PREV,
> +    PREV_UNIQUE,
> +
> +    DIRECTION_INVALID

You know what to do.

::: dom/indexedDB/IDBDatabase.cpp
@@ +318,5 @@
> +  NS_ENSURE_TRUE(objectStore, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +  if (IndexedDatabaseManager::IsMainProcess()) {
> +    nsRefPtr<CreateObjectStoreHelper> helper =
> +      new CreateObjectStoreHelper(aTransaction, objectStore);

Feel free to file a followup on maybe fixing this up to do everything through the helper.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +681,5 @@
> +    ipc::ObjectStoreConstructorParams params;
> +
> +    if (aCreating) {
> +      ipc::CreateObjectStoreParams createParams;
> +      createParams.info() = *static_cast<ObjectStoreInfoGuts*>(aStoreInfo);

No need for explicit cast here.

@@ +2441,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +ObjectStoreHelper::OnParentProcessRequestComplete(

Can we move this to the base AsyncConnectionHelper? And only override for the open-related requests.

@@ +2842,5 @@
> +  return NS_OK;
> +}
> +
> +HelperBase::ChildProcessSendResult
> +GetHelper::MaybeSendResponseToChildProcess(nsresult aResultCode)

I'd say just use mResultCode rather than passing it here, but I'm fine either way.

::: dom/indexedDB/IDBTransaction.h
@@ +126,5 @@
>    }
>  
>    bool IsAborted() const
>    {
> +    return !!mAbortCode;

return NS_FAILED(...);

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +206,5 @@
> +    if (NS_FAILED(Preferences::AddIntVarCache(&gIndexedDBQuotaMB,
> +                                              PREF_INDEXEDDB_QUOTA,
> +                                              DEFAULT_QUOTA_MB))) {
> +      NS_WARNING("Unable to respond to quota pref changes!");
> +      gIndexedDBQuotaMB = DEFAULT_QUOTA_MB;

Just do this for the main process

@@ +711,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    if (StringEndsWith(leafName, NS_LITERAL_STRING(".sqlite-journal"))) {
> +      continue;
> +    }

We should backport this. Please file a separate bug.

@@ +908,5 @@
>  
>      if (aASCIIOrigin.EqualsLiteral("null")) {
>        NS_WARNING("IndexedDB databases not allowed for this principal!");
> +      // XXXbent remove this
> +      return NS_OK;

you *should* remove this :)

::: dom/indexedDB/Key.cpp
@@ +158,5 @@
>          nsresult rv = EncodeJSValInternal(aCx, val, aTypeOffset,
>                                            aRecursionDepth + 1);
> +        if (NS_FAILED(rv)) {
> +          return rv;
> +        }

Revert this

::: dom/indexedDB/ipc/IndexedDBChild.cpp
@@ +211,5 @@
> +  }
> +
> +  nsCString str(aDBInfo.origin);
> +  str.Append("*");
> +  str.Append(NS_ConvertUTF16toUTF8(aDBInfo.name));

This seems silly/dangerous to have duplicated. Please abstract somewhere.

@@ +385,5 @@
> +
> +bool
> +IndexedDBDatabaseChild::RecvPIndexedDBTransactionConstructor(
> +                                             PIndexedDBTransactionChild* aActor,
> +                                             const TransactionParams& aParams)

Maybe add a comment that saying that this code path is just for upgradeneeded

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +155,5 @@
> +                                           mEventListener, false, false, 2);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = aRequest->AddEventListener(NS_LITERAL_STRING(ERROR_EVT_STR),
> +                                  mEventListener, false, false, 2);

Just lose the last two arguments.

@@ +156,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = aRequest->AddEventListener(NS_LITERAL_STRING(ERROR_EVT_STR),
> +                                  mEventListener, false, false, 2);
> +  NS_ENSURE_SUCCESS(rv, false);

NS_ENSURE_SUCCESS(rv, rv);

@@ +171,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +IndexedDBDatabaseParent::ConcreteHandleEvent(nsIDOMEvent* aEvent)

Just name this HandleEvent

@@ +246,5 @@
> +  }
> +
> +  jsval result;
> +  rv = mOpenRequest->GetResult(&result);
> +  NS_ENSURE_SUCCESS(rv, rv);

We could avoid dealing with jsval's and safe js-contexts here. Since this is a open request, we could make open requests internally hold a database pointer and then just use that pointer here. You can avoid the static cast that way too.

Up to you.

@@ +258,5 @@
> +  MOZ_ASSERT(cx);
> +
> +  nsCOMPtr<nsIXPConnectWrappedNative> wrapper;
> +  rv = xpc->GetWrappedNativeOfJSObject(cx, JSVAL_TO_OBJECT(result),
> +                                        getter_AddRefs(wrapper));

r-!!!!!

@@ +296,5 @@
> +    nsRefPtr<IDBOpenDBRequest> request;
> +    mOpenRequest.swap(request);
> +
> +    rv = databaseConcrete->AddEventListener(NS_LITERAL_STRING(ERROR_EVT_STR),
> +                                            mEventListener, false, false, 2);

Make this #ifdef DEBUG

@@ +301,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    NS_NAMED_LITERAL_STRING(versionChange, VERSIONCHANGE_EVT_STR);
> +    rv = databaseConcrete->AddEventListener(versionChange, mEventListener,
> +                                            false, false, 2);

Remove last two arguments.

@@ +309,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    if (mDatabase) {
> +      MOZ_ASSERT(mDatabase == databaseConcrete);

Just do

MOZ_ASSERT(!mDatabase || mDatabase == databaseConcrete);
mDatabase = databaseConcrete;

Just for the fun of it :)

@@ +367,5 @@
> +  nsresult rv;
> +
> +  if (aType.EqualsLiteral(ERROR_EVT_STR)) {
> +    rv = aEvent->PreventDefault();
> +    NS_ENSURE_SUCCESS(rv, rv);

Make this #ifdef DEBUG and just assert that we don't get here

@@ +1691,5 @@
> +    NS_ENSURE_SUCCESS(rv, false);
> +  }
> +
> +  nsRefPtr<IDBRequest> request = mCursor->Request();
> +  MOZ_ASSERT(request);

Just make this mRequest = mCursor->...

::: dom/indexedDB/ipc/SerializationHelpers.h
@@ +55,5 @@
> +  typedef mozilla::dom::indexedDB::IndexInfo paramType;
> +
> +  static void Write(Message* aMsg, const paramType& aParam)
> +  {
> +    WriteParam(aMsg, aParam.name);

Add a comment to DatabaseInfo.h to make sure people don't forget to add here too.

::: gfx/layers/d3d10/ThebesLayerD3D10.cpp
@@ +567,5 @@
>    NS_ABORT_IF_FALSE(mTexture, "Couldn't open foreign texture");
>  
>    // The content process tracks back/front buffers on its own, so
>    // the newBack is in essence unused.
> +  *aNewBack = aNewFront;

Chris needs to review this.

::: ipc/app/MozillaRuntimeMain.cpp
@@ +29,2 @@
>      MessageBox(NULL, L"Hi", L"Hi", MB_OK);
>  #endif

Just nuke this instead

::: ipc/glue/WindowsMessageLoop.cpp
@@ +292,5 @@
>          }
>  
>          log.AppendLiteral(", sending it to DefWindowProc instead of the normal "
>                            "window procedure.");
> +        NS_WARNING(log.get());

Revert this

Comment 17

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

Attachment: Bug fixes and tests, v1

These are bug fixes and tests. On top of other patch.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 629015 [details] [diff] [review]
Bug fixes and tests, v1

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

r=me with the TestRunner.js change explained.

::: ipc/app/MozillaRuntimeMain.cpp
@@ +24,5 @@
>  
>  int
>  main(int argc, char* argv[])
>  {
> +#if defined(XP_WIN)

I don't think you want to check this in.

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +172,5 @@
>  
> +TestRunner.generateFailureList = function () {
> +    if (TestRunner._failureFile) {
> +        var failures = new SpecialPowersLogger(TestRunner._failureFile);
> +        failures.log(JSON.stringify(TestRunner._failedTests));

I don't understand this change.

Comment 19

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

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

Comment 20

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> >+    ChildProcessSendResult sendResult =
> 
> I would recommend striking mentions of "ChildProcess" since this
> should be mostly process/thread-neutral.

Yeah, we're going to remove it as soon as we finish blob support.

> >+  // XXXbent Need to invalidate transactions from this process...
> 
> I'm not exactly sure what this means.  Is this something not taken
> care of by subprotocol ActorDestroy()?

Subprotocol ActorDestroy was handling it just fine so I removed this.

> >+  // XXX Security checks, pref checks.
> 
> What's missing here?

Mainly I was thinking about integrating indexedDB with our B2G security model... You know, comparing the requesting app to the web store permission list, the user granted permission list, etc. Not relevant until we have all that in place, so I removed this.

> This code makes me a little nervous ... are you sure these things are
> always guaranteed to be findable?  I guess race conditions on shutdown
> would just trigger content-process termination, which would happen
> soon anyway.

For now I'm fine killing the child if any of those things aren't findable. If we figure out some legitimate case where they're not then I'll definitely relax it, but right now I can't think of any.

(In reply to Jonas Sicking (:sicking) from comment #16)
> > +  virtual ChildProcessSendResult
> > +  MaybeSendResponseToChildProcess(nsresult aResultCode) = 0;
>
> Just return a bool

I don't remember how we decided to do that with the blob stuff not being implemented. I think we should leave this for now and then remove once we land blob support.

> > +  if (IndexedDatabaseManager::IsMainProcess()) {
> > +    nsRefPtr<CreateObjectStoreHelper> helper =
> > +      new CreateObjectStoreHelper(aTransaction, objectStore);
> 
> Feel free to file a followup on maybe fixing this up to do everything
> through the helper.

Bug 760598

> > +    if (StringEndsWith(leafName, NS_LITERAL_STRING(".sqlite-journal"))) {
> > +      continue;
> > +    }
> 
> We should backport this. Please file a separate bug.

Bug 760600

> > +        if (NS_FAILED(rv)) {
> > +          return rv;
> > +        }
> 
> Revert this

I think we're going to have to agree to disagree here ;)

> > +  rv = aRequest->AddEventListener(NS_LITERAL_STRING(ERROR_EVT_STR),
> > +                                  mEventListener, false, false, 2);
> 
> Just lose the last two arguments.

I did this, but it caused extra headaches (had to cast to nsIDOMEventTarget* before it would work since nsDOMEventTargetHelper has several overrides of AddEventListener that shadow).

> > +  *aNewBack = aNewFront;
> 
> Chris needs to review this.

He basically wrote it ;)

(In reply to Jonas Sicking (:sicking) from comment #18)
> I don't understand this change.

I explained over irc, but for the record, the failure file thing is an optional arg that is passed to runtests.py. Apparently we always pass it on tinderbox, but if you don't then TestRunner.py blows up. I just kept it optional.

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d7d612dd7255

Comment 22

[Justin Wood (:Callek)]

Soooo this is burning SeaMonkey with the following error... (from http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1338739676.1338739983.6604.gz&fulltext=1#err0 ) Going to mark for clobber but I'm not holding my breath.

=========

/e/builds/slave/comm-cen-trunk-w32/build/objdir/mozilla/_virtualenv/Scripts/python.exe -O /e/builds/slave/comm-cen-trunk-w32/build/mozilla/build/cl.py cl -FoIDBCursor.obj -c -D_HAS_EXCEPTIONS=0 -I../../dist/stl_wrappers  -D_IMPL_NS_LAYOUT -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DMOZ_SUITE=1 -DEXCLUDE_SKIA_DEPENDENCIES  -DUNICODE -D_UNICODE -DNOMINMAX -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -D_SECURE_ATL -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DOS_WIN=1 -DWIN32 -D_WIN32 -D_WINDOWS -DWIN32_LEAN_AND_MEAN  -DCOMPILER_MSVC -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/db/sqlite3/src -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/xpcom/build -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/dom/base -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/dom/src/storage -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/content/base/src -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/content/events/src  -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/ipc/chromium/src -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/e/builds/slave/comm-cen-trunk-w32/build/mozilla/dom/indexedDB -I. -I../../dist/include -I../../dist/include/nsprpub  -Ie:/builds/slave/comm-cen-trunk-w32/build/objdir/mozilla/dist/include/nspr -Ie:/builds/slave/comm-cen-trunk-w32/build/objdir/mozilla/dist/include/nss        -TP -nologo -W3 -Gy -Fdgenerated.pdb -wd4800 -we4553  -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O1 -Oy -MD            -FI ../../dist/include/mozilla-config.h -DMOZILLA_CLIENT /e/builds/slave/comm-cen-trunk-w32/build/mozilla/dom/indexedDB/IDBCursor.cpp
IDBCursor.cpp

e:/builds/slave/comm-cen-trunk-w32/build/mozilla/dom/indexedDB/IDBCursor.cpp(42) : error C2872: 'ipc' : ambiguous symbol

        could be 'mozilla::dom::indexedDB::ipc'

        or 'mozilla::ipc'

make[6]: Leaving directory `/e/builds/slave/comm-cen-trunk-w32/build/objdir/mozilla/dom/indexedDB'

Comment 23

[Justin Wood (:Callek)]

...actually it broke the nightly too, so its not just that

Comment 24

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

That's really strange, why wouldn't it burn FF and TB also? Anyway, the fix is simple, just clarify that it's "mozilla::dom::indexedDB::ipc" there.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to ben turner [:bent] from comment #24)
> That's really strange, why wouldn't it burn FF and TB also?

My guess would be this is related to (still) compiling with MSVC8 (not MSVC10).

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #25)
> My guess would be this is related to (still) compiling with MSVC8 (not
> MSVC10).

Bug 761635 Submitted

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

This sounds important to verify; is there a clear QA-reproducible testcase that we can run through, and if so, can you please change [qa?] to [qa+] in the whiteboard status, with steps?  Thanks!

Comment 28

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

This is an extremely complicated feature (the diff is half a megabyte) ... there isn't a single test case that could cover this all.

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #28)
> This is an extremely complicated feature (the diff is half a megabyte) ...
> there isn't a single test case that could cover this all.

(Clearing qa?, and my apologies for the spam.  We jumped the gun on some bug-verification process without understanding the full implications.  We'll follow up with a better method--shared with dev--for the next iteration.)

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why do we need a security review of this? This doesn't make any functional changes.

Comment 31

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

dveditz can you respond to comment 30 please

Comment 32

[Chris Mills [:cmills]]

ddc'ing this; not sure what documentation it needs really, as it doesn't make a difference to web developers, afaics.