603811 - IndexedDB: Implement setVersion changes to the spec

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Attachment: Patch 1, v1

We're out of date. The spec has changed a lot here.

Comment 1

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

Attachment: Patch 2, v1

Comment 2

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

Comment on attachment 482707 [details] [diff] [review]
Patch 1, v1

Please comment all functions in the headerfiles. Also comment all the classes, especially all runnables, describe when they are created, when they are run, and what its main duties are.

>@@ -1074,16 +1075,23 @@ nsGlobalWindow::FreeInnerObjects(PRBool 
...
>+  // Close all IndexedDB databases for this window.
>+  indexedDB::IndexedDatabaseManager* idbManager =
>+    indexedDB::IndexedDatabaseManager::Get();
>+  if (idbManager) {
>+    idbManager->CloseDatabasesForWindow(this);
>+  }

File a bug on making this prevent us from staying in the bfcache if a versionchange is requested. It needs to be a blocker :(

>+IDBDatabase::CloseInternal()

Please document the difference between mClosed and mInvalidated in the headerfile

> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(IDBDatabase,
>                                                 nsDOMEventTargetHelper)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mOnErrorListener)
>+
>+  // Do some cleanup.
>+  tmp->OnUnlink();

I'd also be fine with putting the contents of the onunlink function directly in here

>@@ -566,37 +616,58 @@ IDBDatabase::RemoveObjectStore(const nsA
> 
> NS_IMETHODIMP
> IDBDatabase::SetVersion(const nsAString& aVersion,
...
>+  bool alreadyInProgress;
>+  nsresult rv = mgr->SetDatabaseVersion(this, request, aVersion, helper,
>+                                        &alreadyInProgress);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (alreadyInProgress) {
>+    // Another database has requested a SetVersion before us, and we must not
>+    // continue or else deadlock will occur.
>+    helper->SetError(nsIIDBDatabaseException::DEADLOCK_ERR);
>+
>+    rv = NS_DispatchToCurrentThread(helper);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

Seems strange that the caller of SetDatabaseVersion can count on the helper always getting called *unless* alreadyInProgress is set. Especially when that is the only reason for alreadyInProgress existing. Might be cleaner to have SetDatabaseVersion call the helper and remove the alreadyInProgress argument.

>+IDBVersionChangeEvent::CreateInternal(nsISupports* aSource,
>+                                      const nsAString& aType,
>+                                      const nsAString& aVersion)
>+{
>+  nsRefPtr<IDBVersionChangeEvent> event(new IDBVersionChangeEvent());

Nit: juse use =, it's equivalent and easier on the eyes.

>+IDBVersionChangeEvent::GetVersion(nsAString& aVersion)
>+{
>+  aVersion.Assign(mVersion);

aVersion = mVersion works as well.

>+class VersionChangeEventsRunnable : public nsRunnable
...
>+  NS_IMETHOD Run()
>+  {
>+    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>+
>+    // Fire version change events at all of the databases that are not already
>+    // closed.
>+    for (PRUint32 index = 0; index < mWaitingDatabases.Length(); index++) {
>+      nsRefPtr<IDBDatabase>& database = mWaitingDatabases[index];
>+
>+      if (database->IsClosed()) {
>+        continue;
>+      }
>+
>+      nsCOMPtr<nsIDOMEvent> event(IDBVersionChangeEvent::Create(mVersion));
>+      NS_ENSURE_TRUE(event, NS_ERROR_FAILURE);
>+
>+      PRBool dummy;
>+      database->DispatchEvent(event, &dummy);
>+    }
>+
>+    // Now check to see if any didn't close. If there are some running still
>+    // then fire the blocked event.
>+    for (PRUint32 index = 0; index < mWaitingDatabases.Length(); index++) {
>+      if (mWaitingDatabases[index]->IsClosed()) {
>+        continue;
>+      }
>+
>+      nsISupports* source =
>+        static_cast<nsPIDOMEventTarget*>(mRequestingDatabase);
>+
>+      nsCOMPtr<nsIDOMEvent> event =
>+        IDBVersionChangeEvent::CreateBlocked(source, mVersion);
>+      NS_ENSURE_TRUE(event, NS_ERROR_FAILURE);
>+
>+      PRBool dummy;
>+      mRequest->DispatchEvent(event, &dummy);
>+
>+      break;

This loop feels sort of inverted. Might make more sense to invert if-statement and avoid the continue.

>+IndexedDatabaseManager::WaitForClearAndDispatch(const nsAString& aName,

Please rename this function since it's no longer just waiting for clearing to be done.

>+IndexedDatabaseManager::SetDatabaseVersion(IDBDatabase* aDatabase,
...
>+  // Make a guess at how many elements we'll need.
>+  nsTArray<nsRefPtr<IDBDatabase> > liveDatabases;
>+  if (!liveDatabases.SetCapacity(array->Length())) {
>+    NS_WARNING("Out of memory?");
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

I would say this is not worth it. This only helps if there is more than one *other* IDBDatabase object open to the same database. That doesn't seem like the common case.

...
>+  // When all databases are closed we want to dispatch the SetVersion
>+  // transaction to the transaction pool.
>+  runnable->mCallback =
>+    NS_NewRunnableMethod(aHelper,
>+                         &AsyncConnectionHelper::DispatchToTransactionPool);
>+  NS_ENSURE_TRUE(runnable->mCallback, NS_ERROR_FAILURE);

It'd be nice of you if you could change mCallback to be |nsRefPtr<AsyncConnectionHelper> mHelper| and just directly call DispatchToTransactionPool. The current indirection is a bit confusing.

>+  nsresult rv;
>+  if (runnable->mDatabases.IsEmpty()) {
>+    // There are no other databases that need to be closed. Go ahead and run
>+    // the callback now, using the OnDatabaseClosed hook. The database isn't
>+    // actually closed, of course, but this avoids code duplication.
>+    OnDatabaseClosed(aDatabase);

Maybe rename this function. Yes! What a great idea Jonas!!


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

>-TransactionThreadPool::DatabaseCompleteCallbackRunnable::Run()
>+TransactionThreadPool::DatabasesCompleteCallbackRunnable::Run()
> {
>   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Check for mCallback being null here, and swap with null before calling Run below.

Alternatively, could you just run this immediately when we reach 0 transactions, rather than dispatching.

(Then you could also remove the IsCallbackFreeToRun function since there would only be one caller.)

r=me

Comment 3

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

Comment on attachment 482708 [details] [diff] [review]
Patch 2, v1

>@@ -214,16 +239,25 @@ GenerateRequest(IDBDatabase* aDatabase)
>+struct IDBDatabase::QueuedHelperChunk
>+{
>+  QueuedHelperChunk() : objectStore(nsnull), index(nsnull) { }
>+
>+  IDBObjectStore* objectStore;
>+  IDBIndex* index;

Make these debug-only

>+IDBDatabase::UpdatePendingObjectStores(IDBObjectStore* aObjectStore,
...
>+  // See if we can dispatch any queued helpers now.
>+  if (!mQueuedHelperChunks.IsEmpty()) {
>+    QueuedHelperChunk& chunk = mQueuedHelperChunks[0];
>+
>+    NS_ASSERTION((chunk.objectStore && !chunk.index) ||
>+                 (!chunk.objectStore && chunk.index), "Bad chunk!");
>+
>+    if (chunk.objectStore == aObjectStore) {

Assert that this is true.

> IDBDatabase::CreateObjectStore(const nsAString& aName,
...
>-  if (info->ContainsStoreName(aName)) {
>+  if (databaseInfo->ContainsStoreName(aName)) {
>+    // XXX Should be nsIIDBTransaction::CONSTRAINT_ERR.
>     return NS_ERROR_ALREADY_INITIALIZED;
>   }
> 
>-  nsTArray<nsString> objectStores;
>-  nsString* name = objectStores.AppendElement(aName);
>-  if (!name) {
>-    NS_ERROR("Out of memory?");
>-    return nsIIDBDatabaseException::UNKNOWN_ERR;
>+  IDBTransaction* transaction = AsyncConnectionHelper::GetCurrentTransaction();
>+
>+  if (!transaction ||
>+      transaction->Mode() != nsIIDBTransaction::VERSION_CHANGE) {
>+    // XXX Should be nsIIDBTransaction::NOT_ALLOWED_ERR.
>+    return NS_ERROR_NOT_AVAILABLE;
>   }

Reverse these two so that the right exception is always thrown (when we fix that). Same in RemoveObjectStore/CreateIndex/RemoveIndex


>+RemoveObjectStoreHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
>+{
>+  nsCOMPtr<mozIStorageStatement> stmt =
>+    mTransaction->GetCachedStatement(NS_LITERAL_CSTRING(
>+    "DELETE FROM object_store "
>+    "WHERE name = :name "
>+  ));
>+  NS_ENSURE_TRUE(stmt, nsIIDBDatabaseException::UNKNOWN_ERR);

It's probably theoretically faster to delete based on ID rather than name. Doesn't really matter though.

>@@ -756,28 +753,30 @@ GetAllHelper::DoDatabaseWork(mozIStorage
...
>+  bool unique = mIndex->IsUnique();

This temporary seems unneccesary.

>@@ -882,30 +881,32 @@ GetAllObjectsHelper::DoDatabaseWork(mozI
...
>+  bool unique = mIndex->IsUnique();

Same here

>@@ -1005,28 +1006,30 @@ OpenCursorHelper::DoDatabaseWork(mozISto
...
>+  bool unique = mIndex->IsUnique();

Aaaaand here..

>@@ -1208,30 +1211,32 @@ OpenObjectCursorHelper::DoDatabaseWork(m
...
>+  bool unique = mIndex->IsUnique();

See above

>@@ -679,22 +716,57 @@ IDBObjectStore::UpdateIndexes(IDBTransac
...
>+    if (updateInfo.info.id == LL_MININT) {
>+      // This index has been created in this transaction and we haven't had a
>+      // chance to update the id yet. Grab it out of the database manually.

Make sure you revert this.



> IDBObjectStore::CreateIndex(const nsAString& aName,
...
>+  if (mTransaction->Mode() != nsIIDBTransaction::VERSION_CHANGE) {
>+    // XXX Should be nsIIDBTransaction::NOT_ALLOWED_ERR.
>+    return NS_ERROR_NOT_AVAILABLE;
>   }

You need to also check that mTransaction == AsyncConnectionHelper::GetCurrentTransaction()

Same in RemoveIndex

>   if (!mTransaction->TransactionIsOpen()) {
>     return NS_ERROR_UNEXPECTED;
>   }

And then you can remove this test. Same in RemoveIndex.


>@@ -638,37 +631,50 @@ IDBTransaction::ObjectStore(const nsAStr
>+  if (mMode == nsIIDBTransaction::VERSION_CHANGE) {
>+    if (!ObjectStoreInfo::Get(mDatabase->Id(), aName, &info)) {
>+      info = nsnull;
>+    }
>+  }
>+  else {
>+    info = nsnull;
>+    PRUint32 count = mObjectStoreNames.Length();
>+    for (PRUint32 index = 0; index < count; index++) {
>+      nsString& name = mObjectStoreNames[index];
>+      if (name == aName) {
>+        if (!ObjectStoreInfo::Get(mDatabase->Id(), aName, &info)) {
>+          NS_ERROR("Don't know about this one?!");
>+        }
>+        break;
>       }
>-      break;
>     }
>   }

Can simplify to:

info = nsnull;
if (mMode == nsIIDBTransaction::VERSION_CHANGE ||
    mObjectStoreNames.Contains(aName)) {
  ObjectStoreInfo::Get(mDatabase->Id(), aName, &info);
}


r=me

Comment 4

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

Attachment: Patch 3, v1

This one handles transaction.abort() for version changes.

Comment 5

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

Comment on attachment 483617 [details] [diff] [review]
Patch 3, v1

>+EnumerateObjectStores(const nsAString& aKey,

Don't forget to remove this

>+IDBFactory::UpdateDatabaseMetadata(DatabaseInfo* aDatabaseInfo,
...
>+  for (PRUint32 index = 0; index < existingNames.Length(); index++) {
>+    nsString& existingName = existingNames[index];
>+
>+    bool found = false;
>+    for (PRUint32 index2 = 0; index2 < objectStores.Length(); index2++) {
>+      if (objectStores[index2]->name == existingName) {
>+        found = true;
>+        break;
>+      }
>+    }
>+
>+    if (!found) {
>+      ObjectStoreInfo::Remove(aDatabaseInfo->id, existingName);
>+    }
>+  }

Just change this to always remove all objectstore info for all names. What's not nuked here is nuked below anyway.

r=me

Comment 6

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

Attachment: Final patch

> Comment on attachment 482707 [details] [diff] [review]
> ...
> File a bug on making this prevent us from staying in the bfcache if a
> versionchange is requested. It needs to be a blocker :(

Filed bug 605019

> Seems strange that the caller of SetDatabaseVersion can count on the helper
> always getting called *unless* alreadyInProgress is set. Especially when that
> is the only reason for alreadyInProgress existing. Might be cleaner to have
> SetDatabaseVersion call the helper and remove the alreadyInProgress argument.

Fixed.

> This loop feels sort of inverted. Might make more sense to invert if-statement
> and avoid the continue.

Fixed.

> Please rename this function since it's no longer just waiting for clearing to
> be done.

Sigh.

> I would say this is not worth it. This only helps if there is more than one
> *other* IDBDatabase object open to the same database. That doesn't seem like
> the common case.

Removed.

> It'd be nice of you if you could change mCallback to be
> |nsRefPtr<AsyncConnectionHelper> mHelper| and just directly call
> DispatchToTransactionPool. The current indirection is a bit confusing.

Done.

> Maybe rename this function. Yes! What a great idea Jonas!!

Hmph.

> Alternatively, could you just run this immediately when we reach 0
> transactions, rather than dispatching.

Done.

> Comment on attachment 482708 [details] [diff] [review]
> ...
> Make these debug-only

All of that is gone now.

> Reverse these two so that the right exception is always thrown (when we fix
> that). Same in RemoveObjectStore/CreateIndex/RemoveIndex

I'll fix that when I do the exception audit.

> It's probably theoretically faster to delete based on ID rather than name.
> Doesn't really matter though.

Fixed.

> This temporary seems unneccesary.

Ok.

> Make sure you revert this.

Fixed.

> You need to also check that mTransaction ==
> AsyncConnectionHelper::GetCurrentTransaction()

Done.

> And then you can remove this test. Same in RemoveIndex.

Or better, assert it!

> Can simplify to:

Fixed.

> Comment on attachment 483617 [details] [diff] [review]
> ...
> Don't forget to remove this

Gone.

> Just change this to always remove all objectstore info for all names. What's
> not nuked here is nuked below anyway.

Ok.

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/5fc19997e7bf

Comment 8

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

Bah, forgot to fix chrome tests.

http://hg.mozilla.org/mozilla-central/rev/58002af0e673

Comment 9

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

Aaaaand it also needed this test fix:

http://hg.mozilla.org/mozilla-central/rev/234b438e9cd6