697247 - IndexedDB: Database opening needs to block other database openings from proceeding

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Right now:

mozIndexedDB.open("foo", 1);
mozIndexedDB.open("foo", 1);

The second opening will start proceeding and when the first opening realizes it needs to do a VERSION_CHANGE transaction things will fall down.

Comment 1

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

Attachment: Part 1: Use atoms instead of IDs

Comment 2

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

Attachment: Part 2: Everything else

Rip out SetVersionRunnable, and maintain a list of OpenDatabaseHelpers instead.  We move the delayed runnables/waiting for databases to close stuff to OpenDatabaseHelper.  Then we fix everything up and add some tests.

Comment 3

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

Comment on attachment 569485 [details] [diff] [review]
Part 1: Use atoms instead of IDs

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

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +526,5 @@
>  nsresult
> +OpenDatabaseHelper::Init()
> +{
> +  nsCString str;
> +  str.Append(mASCIIOrigin);

Nit: Pass mASCIIOrigin as the constructor arg.

Comment 4

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

Comment on attachment 569487 [details] [diff] [review]
Part 2: Everything else

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

::: dom/indexedDB/CheckPermissionsHelper.h
@@ +63,5 @@
>    NS_DECL_NSIOBSERVER
>  
>    CheckPermissionsHelper(OpenDatabaseHelper* aHelper,
>                           nsIDOMWindow* aWindow,
> +                         nsIAtom* aId,

Hm. Why do you need this here? Looks like you can get rid of it.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +462,3 @@
>                 "Delayed runnables should have been dispatched already!");
>  
> +  // Remove this helper from the list. This will allow openings to proceed.

Nit: "allow *other* openings to proceed"

@@ +467,5 @@
>    }
>  }
>  
>  nsresult
> +IndexedDatabaseManager::WaitForOpenAllowed(nsIAtom* aId,

Hm, I don't especially like the changes here. We can chat on irc about it a bit maybe.

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +43,5 @@
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"
>  #include "mozilla/dom/indexedDB/IDBDatabase.h"
>  #include "mozilla/dom/indexedDB/IDBRequest.h"
>  
> +#ifdef IMPL_INDEXEDDB

Boo. Let's not do this.

Comment 5

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

Attachment: Part 2 - Everything else

This is mostly what we discussed the other day, except that I decided to maintain everything in the list of pending operations.

Comment 6

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

Comment on attachment 570237 [details] [diff] [review]
Part 2 - Everything else

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

This looks good! Mostly nits below.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +321,2 @@
>  
> +  nsAutoPtr<SynchronizedOp> op = new SynchronizedOp(aOrigin, aId);

nsAutoPtr kinda sucks, I don't think this compiles on GCC. Use the constructor (e.g. | nsAutoPtr<SynchronizedOp> op(new SynchronizedOp(aOrigin, aId)); |).

@@ +345,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +IndexedDatabaseManager::AllowOpensToProceed(const nsACString& aOrigin,

Nit: OnSynchronizedOpComplete? AllowNextSynchronizedOp?

@@ +360,5 @@
> +        NS_ASSERTION(op->mDatabases.IsEmpty(), "How did this happen?");
> +
> +        op->DispatchDelayedRunnables();
> +
> +        mSynchronizedOps.RemoveElementAt(index);

Hm. How about you just make this method take a SynchronizedOp* and then have the helpers pass 'this'? Then you could just call IndexOf instead of looping manually.

@@ +375,4 @@
>  }
>  
>  nsresult
> +IndexedDatabaseManager::AcquireExclusiveAccess(const nsACString& aOrigin, 

Nit: Leave this impl in the header.

@@ +383,5 @@
> +  return AcquireExclusiveAccess(aOrigin, nsnull, aHelper, aCallback, aClosure);
> +}
> +
> +nsresult
> +IndexedDatabaseManager::AcquireExclusiveAccess(IDBDatabase* aDatabase, 

Nit: Leave this impl in the header.

@@ +418,4 @@
>      }
>    }
>  
> +  NS_ASSERTION(op, "We didn't find a SynchronizedOp?");

This isn't testing what you want it to since you're assigning op every time through the loop. I think you want to set it to null, then only assign if you break.

@@ +422,5 @@
>  
> +  nsTArray<IDBDatabase*>* array;
> +  mLiveDatabases.Get(aDatabase->Origin(), &array);
> +
> +  // Otherwise, we need to wait for the databases to go away.

Nit: "Otherwise" doesn't make sense here.

@@ +433,5 @@
> +    for (PRUint32 index = 0; index < count; index++) {
> +      IDBDatabase*& database = array->ElementAt(index);
> +      if (!database->IsClosed() &&
> +          database != aDatabase &&
> +          (aDatabase && database->Id() == aDatabase->Id())) {

This is icky... How about:

  if (aDatabase &&
      aDatabase != database &&
      aDatabase->Id() == database->Id() &&
      !database->IsClosed()) {
    ...
  }

@@ +530,5 @@
> +        (op->mId == aDatabase->Id() || !op->mId)) {
> +      // This database is in the scope of this SynchronizedOp.  Remove it
> +      // from the list if necessary.
> +      if (!op->mDatabases.IsEmpty() &&
> +          op->mDatabases.RemoveElement(aDatabase)) {

Nit: No need for the IsEmpty check, RemoveElement will return false in that case anyway.

@@ +532,5 @@
> +      // from the list if necessary.
> +      if (!op->mDatabases.IsEmpty() &&
> +          op->mDatabases.RemoveElement(aDatabase)) {
> +        // Now set up the helper if there are no more live databases.
> +        NS_ASSERTION(op->mHelper, "How did we get rid of the helper before"

Nit: need a space after "before"

@@ +540,5 @@
> +          // can be started.  There may, however, still be outstanding
> +          // transactions that have not completed.  We need to wait for those
> +          // before we dispatch the helper.
> +
> +          TransactionThreadPool* pool = TransactionThreadPool::GetOrCreate();

This can theoretically fail, bail if it does. (I know the old code didn't, but hey, some moron wrote that ;) )

@@ +546,5 @@
> +          nsRefPtr<WaitForTransactionsToFinishRunnable> waitRunnable =
> +            new WaitForTransactionsToFinishRunnable(op);
> +
> +          nsAutoTArray<nsRefPtr<IDBDatabase>, 1> array;
> +          if (!array.AppendElement(aDatabase)) {

This is infallible now, no need to check.

@@ +676,5 @@
>    return rv;
>  }
>  
> +void
> +IndexedDatabaseManager::DispatchHelper(AsyncConnectionHelper* aHelper)

Nit: add a comment above to denote static method.

Also, this can fail, so maybe return nsresult and have the callers handle failure.

@@ +685,5 @@
> +    if (NS_FAILED(aHelper->DispatchToTransactionPool())) {
> +      NS_WARNING("Failed to dispatch to thread pool!");
> +    }
> +  }
> +  // Otherwise, dispatch it to the IO thread.

Nit: Move comment inside else clause

@@ +695,5 @@
> +  }
> +}
> +
> +bool
> +IndexedDatabaseManager::HaveClearOriginRunnableForOrigin(const nsACString& origin)

Nit: IsClearOriginPending?

@@ +933,3 @@
>      // Tell the IndexedDatabaseManager that we're done.
>      IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
> +    // XXXkhuey why is this conditional?

Probably should be asserted, not conditional.

@@ +1106,5 @@
>    return NS_OK;
>  }
> +
> +
> +IndexedDatabaseManager::SynchronizedOp::SynchronizedOp(const nsACString& aOrigin,

Assert main thread on constructor and destructor?

@@ +1134,5 @@
> +  if (aRhs.mId == mId) {
> +    return true;
> +  }
> +
> +  // Waiiting is required if either one corresponds to an origin clearing

Nit: "Waiiting"

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +210,5 @@
>  
>    // Called when AsyncUsageRunnable has finished its Run() method.
>    inline void OnUsageCheckComplete(AsyncUsageRunnable* aRunnable);
>  
> +  // Struct contain

Nit: Unfinished comment

@@ +260,5 @@
>    // usage statistics.
>    nsAutoTArray<nsRefPtr<AsyncUsageRunnable>, 1> mUsageRunnables;
>  
> +  // Maintains a list of synchronized operatons that are in progress or queued.
> +  nsAutoTArray<nsAutoPtr<SynchronizedOp>, 1> mSynchronizedOps;

I'm feeling generous today, let's make this 5.

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1093,5 @@
>    return WrapNative(aCx, NS_ISUPPORTS_CAST(nsIDOMEventTarget*, mDatabase),
>                      aVal);
>  }
>  
> +/* static */ void

Nit: Two lines, and use //

::: dom/indexedDB/OpenDatabaseHelper.h
@@ +85,5 @@
>  
>    nsresult NotifySetVersionFinished();
>    void BlockDatabase();
>  
> +  nsIAtom* Id()

Nit: const

@@ +90,5 @@
> +  {
> +    return mDatabaseId.get();
> +  }
> +
> +  IDBDatabase* Database()

Nit: const

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a64b3abb31c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/572e4723a496

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a64b3abb31c4
https://hg.mozilla.org/mozilla-central/rev/572e4723a496

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 570237 [details] [diff] [review]
Part 2 - Everything else

>--- a/dom/indexedDB/CheckPermissionsHelper.cpp
>+++ b/dom/indexedDB/CheckPermissionsHelper.cpp
>@@ -201,13 +201,13 @@ CheckPermissionsHelper::Observe(nsISuppo
>   IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
>   NS_ASSERTION(mgr, "This should never be null!");
> 
>-  rv = mgr->WaitForOpenAllowed(mName, mASCIIOrigin, this);
>+  rv = NS_DispatchToCurrentThread(this);

And mgr is unused now?