Closed Bug 994190 (IndexedDB-on-PBackground) Opened 10 years ago Closed 10 years ago

Modify main-thread IndexedDB to use PBackground

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

http://hg.mozilla.org/projects/jamun/file/tip/dom/indexedDB has all the action.

It turns out that this involves moving lots of code so patches are going to be impractical. The best review strategy I believe is to simply review the code as it exists in the repo.

I'm asking khuey to get started on the review since most is done already. Stuff that works:
  - opening/deleting databases and all the associated events
  - objectStores
  - indexes
  - cursors

Things that don't work yet:
  - blobs
  - filehandle
  - permission/quota prompts
  - clear private data integration

I'm working on the prompts next since that will let me remove a lot of old code. Then I'll tackle private data integration (should be quick) and finally blobs.
Flags: needinfo?(khuey)
Attached file review.txt (obsolete) —
This is my review so far.  I haven't looked at all of it, but I did take a pretty good look at the stuff around opening/closing databases and transaction management.
Flags: needinfo?(khuey)
Blocks: 1020196
Comment on attachment 8480762 [details] [diff] [review]
mega.patch

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

My comments from reviewing ActorsParent/Child:

MOZ_ENABLE_PROFILER_SPS in AutoSetCurrentTransaction is bullshit.
Fix comment too.

GetResult nsISupports overload will not work off main thread.

Give these arguments better names than aUserData.

Make arrays call the single versions

Line 587: function pointer that is always the same?

DispatchErrorEvent: early return if DispatchEvent fails.

~BackgroundRequestChild should be virtual.

Consolidate code to call OnRequestfinished between ~BackgroundRequestChild and ActorDestroy.

BackgroundTransactionChild::AssertIsOnOwningThread needs MOZ_OVERRIDE

BackgroundTransactionChild::RecvComplete should assert mTransaction

IDBFactory::CreateForWindow should assert it gets an inner window.

Look at Reset() on the main thread.

BackgroundDatabaseChild::EnsureDOMObject should be void

RecvBlocked should assert it has a request.

RecvVersionChange has totally busted bfcache handling.

Maybe<nsRefPtr<IDBCursor>> is super bizarre

ActorsParent.cpp:2535 RequestOp is dead.

~ShutdownTransactionThreadPoolRunnable() can assert threadedness

Factory::ActorDestroy: nested event loop is probably not cool.

AllocPBackgroundIDBFactoryRequestParent hard crash

State machine comments need to specify which thread they're on.

ActorsParent.cpp:3876 double spacing?

State_SendUpgradeNeeded is not a state.

FactoryOp::Run should be in the same order as the comments.

ActorsParent.cpp:11560 assert less than, not just inequality

How do we end up in NoteDatabaseBlocked if we're in State_BlockedWaitingForOtherDatabasesToClose.

ActorsParent.cpp:11729 revert the state?

Should ~FactoryOp assert that we are in the complete state?

ActorsParent.cpp:6184 should have a comment telling where this AddRef is balanced.

ActorsParent.cpp:6368 should have a comment saying it can't have ASSERT_UNLESS_FUZZING
because invalidation.

ACtorsParent.cpp:3179 type 'Mus'

Combine RequestParams::TObjectStoreAddParams and TObjectStorePutParams

ObjectStoreGetRequestOp::GetResponse assert mResponse.Length() <= mLimit

ObjectStoreGetRequestOp::GetResponse put the loop stuff into a function and share.

mCurrentlyRunningOp checks in Cursor need the ASSERT_UNLESS_FUZZING stuff.
all over RecvContinue actually.

Share code between the two BeginVersionChange functions.
SendBlockedNotification.

::: dom/indexedDB/ActorsParent.cpp
@@ +9233,5 @@
> +  MOZ_ASSERT(!mShutDown);
> +
> +  if (mBackgroundThread != aBackgroundThread) {
> +    mBackgroundThread = aBackgroundThread;
> +  }

Just assign this unconditionally.

@@ +9529,5 @@
> +
> +  mShutDown = true;
> +
> +  if (mBackgroundThread) {
> +    nsRefPtr<ShutdownTransactionThreadPoolRunnable> runnable = 

whitespace at EOL

::: dom/indexedDB/IDBDatabase.cpp
@@ +811,5 @@
> +
> +  // We use the DOMFile's nsIWeakReference as the key to the table because
> +  // a) it is unique per blob, b) it is reference-counted so that we can
> +  // guarantee that it stays alive, and c) it doesn't hold the actual DOMFile
> +  // alive. We don't ever need to actually use the nsIWeakReference.

You do use the weak ref, in ExpireFileActors.

::: dom/indexedDB/IDBRequest.h
@@ +43,3 @@
>  {
> +protected:
> +  // At most one of these three fields can be non-null.

This comment is wrong now for cursor.

::: dom/indexedDB/IndexedDBParams.ipdlh
@@ +1,1 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Move into PBackgroundIDBSharedTypes.ipdlh?

::: dom/indexedDB/TransactionThreadPool.cpp
@@ +183,5 @@
> +{
> +  mozilla::Monitor mMonitor;
> +
> +  TransactionThreadPool* mOwningThreadPool;
> +  nsCOMPtr<nsIEventTarget> mOwningThread;

What's this about?  These are the same for every queue, no?  Get this from the TransactionThreadPool.

@@ -156,5 @@
>  {
> -  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> -
> -  PROFILER_MAIN_THREAD_LABEL("TransactionThreadPool", "Cleanup",
> -    js::ProfileEntry::Category::STORAGE);

why is this gone?

@@ +689,3 @@
>    callback->mCallback = aCallback;
> +  callback->mDatabaseIds.SwapElements(aDatabaseIds);
> +  

extraneous white space

::: dom/indexedDB/TransactionThreadPool.h
@@ +16,2 @@
>  
> +template <typename> class nsAutoPtr;

unnecessary forward decl

::: dom/indexedDB/test/unit/test_invalidate.js
@@ +24,5 @@
> +  is(event.type, "upgradeneeded", "Upgrading database " + dbCount);
> +
> +  request.onupgradeneeded = unexpectedSuccessHandler;
> +
> +  let objStore = 

whitespace at EOL

::: dom/indexedDB/test/unit/test_setVersion_events.js
@@ +15,4 @@
>    // Sanity checks
>    ok(request instanceof IDBRequest, "Request should be an IDBRequest");
>    ok(request instanceof IDBOpenDBRequest, "Request should be an IDBOpenDBRequest");
>    //ok(request instanceof EventTarget, "Request should be an EventTarget");

Remove this line.

::: dom/ipc/Blob.cpp
@@ +1483,5 @@
>  
>    nsRefPtr<DOMFile> blob = new DOMFile(remoteBlob);
>    blob.forget(&mBlob);
>  
> +

Extraneous \n

@@ +2405,5 @@
> +
> +// static
> +BlobParent*
> +BlobParent::CreateFromParams(nsIContentParent* aManager,
> +                             const ParentBlobConstructorParams& aParams)

Template your way to victory here.

@@ +2417,5 @@
>               ChildBlobConstructorParams::TMysteryBlobConstructorParams);
>  
>    switch (blobParams.type()) {
>      case ChildBlobConstructorParams::TMysteryBlobConstructorParams:
> +      MOZ_ASSERT(false);

This should be ASSERT_UNLESS_FUZZING.

@@ +2794,5 @@
>        const NormalBlobConstructorParams& params =
>          aParams.get_NormalBlobConstructorParams();
> +
> +      if (NS_WARN_IF(params.length() == UINT64_MAX)) {
> +        MOZ_ASSERT(false);

ASSERT_UNLESS_FUZZING

@@ +2812,5 @@
>      case ResolveMysteryParams::TFileBlobConstructorParams: {
>        const FileBlobConstructorParams& params =
>          aParams.get_FileBlobConstructorParams();
> +      if (NS_WARN_IF(params.name().IsVoid())) {
> +        MOZ_ASSERT(false);

ASSERT_UNLESS_FUZZING

@@ +2851,5 @@
> +  MOZ_ASSERT_IF(!mBlob, mBlobImpl);
> +  MOZ_ASSERT(!mRemoteBlob);
> +
> +  if (NS_WARN_IF(!IndexedDatabaseManager::InTestingMode())) {
> +    MOZ_ASSERT(false);

ASSERT_UNLESS_FUZZING

@@ +2879,5 @@
> +  MOZ_ASSERT_IF(!mBlob, mBlobImpl);
> +  MOZ_ASSERT(!mRemoteBlob);
> +
> +  if (NS_WARN_IF(!IndexedDatabaseManager::InTestingMode())) {
> +    MOZ_ASSERT(false);

ASSERT_UNLESS_FUZZING.

::: dom/ipc/TabParent.cpp
@@ +734,5 @@
> +  -> PIndexedDBPermissionRequestParent*
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIPrincipal> principal(aPrincipal);

We need to check this principal and make sure it matches the app/etc.

::: dom/quota/nsIOfflineStorage.h
@@ +12,3 @@
>  #define NS_OFFLINESTORAGE_IID \
>    {0x3ae00063, 0x6c13, 0x4afd, \
>    { 0x86, 0x7d, 0x33, 0xc2, 0x12, 0xd8, 0x97, 0x25 } }

Change the IID.

::: ipc/glue/BackgroundImpl.cpp
@@ +849,5 @@
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(aBackgroundActor);
> +  MOZ_ASSERT(aBlobImpl);
> +
> +  // If the blob represents a remote blob for this ContentParent then we can

s/ContentParent/BackgroundParent/

@@ +867,5 @@
> +  ChildBlobConstructorParams params;
> +
> +  if (aBlobImpl->IsSizeUnknown() || aBlobImpl->IsDateUnknown()) {
> +    // We don't want to call GetSize or GetLastModifiedDate
> +    // yet since that may stat a file on the main thread

on this thread

@@ +869,5 @@
> +  if (aBlobImpl->IsSizeUnknown() || aBlobImpl->IsDateUnknown()) {
> +    // We don't want to call GetSize or GetLastModifiedDate
> +    // yet since that may stat a file on the main thread
> +    // here. Instead we'll learn the size lazily from the
> +    // other process.

other side?

@@ +1132,5 @@
>      // it for us. This is safe since we are guaranteed that our AddRef runnable
>      // will run before the reference we hand out can be released, and the
>      // ContentParent can't die as long as the existing reference is maintained.
>      nsCOMPtr<nsIRunnable> runnable =
> +      NS_NewNonOwningRunnableMethod(actor->mContent.get(),

Did we fix this?  I think we did.

::: ipc/glue/InputStreamParams.ipdlh
@@ +41,5 @@
>  {
>    PBlob remoteBlob;
>  };
>  
> +// XXX This may only be used for same-process intra-thread communication! The

interthread.  intra- means within.

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +434,5 @@
> +          } else if (('appId' in msg) && msg.appId !== undefined) {
> +            qm.clearStoragesForURI(uri, null, appId);
> +          } else {
> +            qm.clearStoragesForURI(uri);
> +          }

Explain why we're getting usage (to synchronize and call back).
Attachment #8480762 - Flags: review-
Attachment #8409340 - Attachment is obsolete: true
Attached patch khueyreview.patch (obsolete) — Splinter Review
Here's a full diff of the changes. They're broken into several separate patches on jamun if you want to see smaller changes.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3)
> Factory::ActorDestroy: nested event loop is probably not cool.

This was pretty complicated... We should go over these changes probably. See the second patch on jamun.

> Move into PBackgroundIDBSharedTypes.ipdlh?

This file is gone now.

> > +BlobParent::CreateFromParams(nsIContentParent* aManager,
> > +                             const ParentBlobConstructorParams& aParams)
> 
> Template your way to victory here.

This actually wasn't worth it due to the different types involved (nsIDOMBlob vs. DOMFileImpl) and different functions that need to be called. I guess we were both just tired when we looked at it the other day.

> > -  PROFILER_MAIN_THREAD_LABEL("TransactionThreadPool", "Cleanup",
> > -    js::ProfileEntry::Category::STORAGE);
> 
> why is this gone?

It causes problems since it spins the event loop and one of the events that it could process is the event that shuts down the thread and unregisters the profiler tls stuff.
Attachment #8483591 - Flags: review?(khuey)
Updated patch based on changes to make tinderbox happy.
Attachment #8483591 - Attachment is obsolete: true
Attachment #8483591 - Flags: review?(khuey)
Attachment #8484300 - Flags: review?(khuey)
Comment on attachment 8484300 [details] [diff] [review]
khueyreview.patch

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

We need at least two followups:

1. To make PBackground thread never shut down.  Once we use it from the main process parent thread it lives forever anyways, so let's just sidestep the edge case.
2. Deal with transaction ordering across different transaction threadpools.  #1 would probably subsume this.

::: dom/indexedDB/ActorsParent.cpp
@@ +5837,4 @@
>    // Clean up if there are no more instances.
>    if (!(--sFactoryInstanceCount)) {
> +    if (gCurrentTransactionThreadPool) {
> +      gCurrentTransactionThreadPool->ShutdownAsync();

Assert that mTransactionThreadPool is gCurrentTransactionThreadPool.  Or just assign into the kungFuDeathGrip here.

@@ +15049,5 @@
>          aResponse.get_ObjectStoreGetAllResponse().cloneInfos();
>  
>        fallibleCloneInfos.SwapElements(cloneInfos);
>      }
>    

extraneous whitespace
Attachment #8484300 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> We need at least two followups:
> 
> 1. To make PBackground thread never shut down.

Bug 1067046.

> 2. Deal with transaction ordering across different transaction threadpools.

Bug 1067140.
https://hg.mozilla.org/mozilla-central/rev/14a2fe92d07b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1067307
Depends on: 1067351
This has caused IndexedDB to fail in the B2G Music and Gallery apps. This is the log output in the case of the music app:

E/GeckoConsole( 6271): [JavaScript Error: "IndexedDB UnknownErr: ActorsParent.cpp:415"]
E/GeckoConsole( 6271): [JavaScript Error: "IndexedDB UnknownErr: ActorsParent.cpp:415"]
E/Music   ( 6983): [JavaScript Error: "UnknownError"]
E/Music   ( 6983): [JavaScript Error: "UnknownError"]
E/Music   ( 6983): Content JS ERROR at app://music.gaiamobile.org/gaia_build_defer_index.js:227 in MediaDB/openRequest.onerror: MediaDB(): UnknownError
E/Music   ( 6983): Content JS ERROR at app://music.gaiamobile.org/gaia_build_defer_index.js:388 in withStoreOnError: asyncStorage: can't open database: UnknownError
Depends on: 1067885
Depends on: 1068102
Depends on: 1068113
This bug's commit removed a FAIL_ON_WARNINGS annotation from dom/indexedDB:
https://hg.mozilla.org/mozilla-central/rev/14a2fe92d07b#l126.88

Why was that?

(I filed bug 1068113 on adding it back, and bug 1068102 on some build warnings that were introduced by this bug that would've been automatically caught if the annotation hadn't been removed.)
Flags: needinfo?(bent.mozilla)
It was just temporally disabled on a private branch during development and then accidentally forgotten to enable back.
Flags: needinfo?(bent.mozilla)
Depends on: 1068175
Depends on: 1068177
(In reply to Jan Varga [:janv] from comment #12)
> It was just temporally disabled on a private branch during development

That's probably not a good practice to follow, since it lets issues creep in undetected, as shown by the various dependent bugs that I've just filed, for new build warnings caused by problems in this patch.

I presume someone disabled it because it was giving you trouble by causing unexpected bustage on TBPL for Try, or for a project branch.  If that's the case, it means you probably want to add "ac_add_options --enable-warnings-as-errors" to your local mozconfig.  This will help you discover & fix (or temporarily hack around) most warnings locally, instead of having to wait until you discover them on a failed Try run.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/00f11fa87d80
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Before re-landing, could you take out the FAIL_ON_WARNINGS-removal? (since that wasn't intended to land in the first place)

(That'd mean you'd need to also fix the bugs in this patch that cause build warnings before re-landing, too; I've posted fixes for nearly all of them, which you're welcome to merge into the changeset; the only ones that I haven't posted patches for are the NS_WARN_IF misusages covered in bug 1068175l.)
Yeah, I'll include your warning changes.
https://hg.mozilla.org/mozilla-central/rev/8892214038df
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 1074418
Blocks: 771288
Note that this has (probably) regressed the Gaia email app's ability to send messages with attachments via ActiveSync. See bug 1075302 for a modified mochitest that simply reproduces the problem without any e10s or multiprocess involvement.  It may be appropriate to back this out again.
For those playing along at home:  I spoke with Andrew Sutherland offline and he agreed it's okay to let this be broken on master for a few days until Ben Turner can get to it.
Depends on: 1077106
Depends on: 1078438
Depends on: 1079313
Depends on: 1080020
Depends on: 1080090
Depends on: 1076975
Depends on: 1078210
Depends on: 1077331
Depends on: 1080298
Depends on: 1079824
Depends on: 1081348
Depends on: 1083285
Depends on: 1075562
(In reply to Andrew Overholt [:overholt] from comment #23)
> For those playing along at home:  I spoke with Andrew Sutherland offline and
> he agreed it's okay to let this be broken on master for a few days until Ben
> Turner can get to it.

Lots of us in Gaia land are eager to have a fix for this ASAP as can be seen by all the bugs that depend on this one.

Andrew or Ben: could one of you give us an update on the progress here? Now that "a few days" has turned into 18 days, should we revisit the decision not to back this out? Is there an active non-resolved bug where work is ongoing to fix all of this?
Flags: needinfo?(overholt)
Flags: needinfo?(bent.mozilla)
Bug 1075302 was fixed two weeks ago, other work is happening in other bugs (bug 1079546, bug 1076975). What problem specifically are you asking about here?
Flags: needinfo?(overholt)
Flags: needinfo?(bent.mozilla)
Since I don't know anything about IndexedDB, the specific problem in my mind is "all the gaia regressions caused by 994190". I was hoping that there would be one patch that would just fix them all. :-)

I guess once you get 1076975 fixed we can re-test the others to see how many of those were duplicates.
Depends on: 1079546
No longer depends on: 1078210
Depends on: 1087464
Depends on: 1088595
No longer depends on: 1088595
Depends on: 1088043
No longer depends on: 1079824
Depends on: 1079824
No longer depends on: 1078438
No longer depends on: 1079824
Alias: IndexedDB-on-PBackground
Depends on: 1122750
No longer depends on: 1143885
Depends on: 1300007
Depends on: 1339393
Depends on: CVE-2017-7843
Regressions: 1741201
You need to log in before you can comment on or make changes to this bug.