625071 - IndexedDB: Implement deleteDatabase

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

We won't have this for Firefox 4, but soon after we should add it.

Comment 1

[Shawn Gong]

Attachment: Initial implementation for deleteDatabase

Comment 2

[Shawn Gong]

Comment on attachment 537218 [details] [diff] [review]
Initial implementation for deleteDatabase

Hey Ben, could you take a look if I'm doing the right things? Thanks!

Comment 3

[Shawn Gong]

Attachment: Initial implementation for deleteDatabase

Comment 4

[Shawn Gong]

Attachment: Initial implementation for deleteDatabase

Now attached the test file

Comment 5

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

Comment on attachment 537279 [details] [diff] [review]
Initial implementation for deleteDatabase

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

Overall looking good! A few big things and a few little things need fixing.

::: dom/indexedDB/IDBFactory.cpp
@@ +376,5 @@
>      PRBool isDirectory;
>      rv = dbDirectory->IsDirectory(&isDirectory);
>      NS_ENSURE_SUCCESS(rv, rv);
>      NS_ENSURE_TRUE(isDirectory, NS_ERROR_UNEXPECTED);
> +  } else if (aCreateNewDirectory) {

Nit: else clauses need to start on their own line.

@@ +381,5 @@
>      rv = dbDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);
>      NS_ENSURE_SUCCESS(rv, rv);
> +  } else {  
> +    // no such database means we're good
> +    return NS_OK;

Hm. I see your plan here, but I think it's a little confusing to split this function the way you have. Basically, the old function did several things:

a. Figured out the file path given the database name and origin.
b. Created the directory structure.
c. Created a database file.

Now you've split the function in two:

1. GetDatabaseFile, which does (a) and (b) above.
2. CreateDatabaseConnection, which does (c) above.

It seems like a much better idea to move all the filesystem modifications into CreateDatabaseConnection. Then GetDatabaseFile can be responsible for simply figuring out the file path and you wouldn't have to have the extra complexity about creating the directories. Also, I really hate having to pass the mozIStorageServiceQuotaManagement parameter...

@@ +455,5 @@
>  
>    rv = dbFile->Append(filename);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  *aFile = dbFile.forget().get();

The correct way to do this is to use dbFile.forget(aFile), I think our template magic will resolve that.

@@ +1110,5 @@
>    return WrapNative(aCx, NS_ISUPPORTS_CAST(nsPIDOMEventTarget*, database),
>                      aVal);
>  }
> +
> +class DeleteDatabaseHelper : public AsyncConnectionHelper

Move class declaration into the anonymous namespace at the top of the file.

@@ +1146,5 @@
> +  NS_ASSERTION(!aConnection, "Huh?!");
> +
> +  if (IndexedDatabaseManager::IsShuttingDown()) {
> +    return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +  }

I think this is wrong, if someone has requested that we delete a database we should do it, even if we're about to shut down.

@@ +1159,5 @@
> +  }
> +
> +  PRBool exists;
> +  rv = dbFile->Exists(&exists);
> +  NS_ENSURE_SUCCESS(rv, rv);

We take care to only return special IndexedDB error codes from the database helpers (they all start with NS_ERROR_DOM_INDEXEDDB_). In this case I think it's safe to return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR

@@ +1162,5 @@
> +  rv = dbFile->Exists(&exists);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +    
> +  if (exists) {
> +    nsresult rv = dbFile->Remove(PR_FALSE);

You already have an rv variable.

@@ +1163,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +    
> +  if (exists) {
> +    nsresult rv = dbFile->Remove(PR_FALSE);
> +    NS_ENSURE_SUCCESS(rv, rv);

Same deal here with returning an IndexedDB error code.

@@ +1173,5 @@
> +nsresult
> +DeleteDatabaseHelper::GetSuccessResult(JSContext* aCx,
> +                                     jsval *aVal)
> +{
> +  return nsnull;

There are a few problems here. You're supposed to return an nsresult and you're returning 0 instead. Also, you're supposed to set aVal to the actual value that will be returned to JS code. I think the spec says you're supposed to set the result of the event to null?

@@ +1181,5 @@
> +IDBFactory::DeleteDatabase(const nsAString& aName,
> +                           JSContext* aCx,
> +                           nsIIDBRequest** _retval)
> +{
> +   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Nit: I think your indent is off here.

@@ +1221,5 @@
> +    if (origin.EqualsLiteral("null")) {
> +      NS_WARNING("IndexedDB databases not allowed for this principal!");
> +      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +    }
> +  }

All of this duplicated code should be moved to some new helper function.

@@ +1233,5 @@
> +
> +  nsRefPtr<IndexedDatabaseManager> mgr = IndexedDatabaseManager::GetOrCreate();
> +  NS_ENSURE_TRUE(mgr, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +  rv = mgr->WaitForDbsToClose(aName, origin, deleteHelper, request );

How about "Databases" rather than "Dbs". Also, you have an extra space after the last parameter.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +161,5 @@
>  
> +  VersionChangeEventsRunnable(
> +                            IDBRequest* aRequest,
> +                            nsTArray<nsRefPtr<IDBDatabase> >& aWaitingDatabases,
> +                            const nsAString& aVersion)

IDBVersionChangeRequest inherits IDBRequest so there's no need for two different members or two different constructors. I'd change the type of mRequest to be plain IDBRequest, and then change the constructor arg too. Then you could just pass null for aRequestingDatabase to tell the two apart.

@@ +422,5 @@
> +                                           const nsACString& aOrigin,
> +                                           AsyncConnectionHelper* aHelper,
> +                                           IDBRequest* aRequest)
> +{
> +  // IDBRequest is optional by W3 Spec.

I don't know what this means, there's always a request associated with a call to deleteDatabase right?

@@ +433,5 @@
> +  nsTArray<IDBDatabase*>* array;
> +  if (!mLiveDatabases.Get(aOrigin, &array)) {
> +     // No need to worry live dbs, dispatch the deleteHelper
> +    return aHelper->Dispatch(IOThread());
> +  }

This won't work, you need to block calls to openDatabase while your helper is running. Otherwise another thread could make a database file while you're deleting it.

@@ +451,5 @@
> +  }
> +
> +  nsAutoString version;
> +  nsRefPtr<VersionChangeEventsRunnable> eventsRunnable =
> +    new VersionChangeEventsRunnable(aRequest, liveDatabases, version);

no need for a version arg here, just pass EmptyString(), or change the constructor to make that optional.

::: dom/indexedDB/test/test_delete_database.html
@@ +18,5 @@
> +
> +      let request;
> +
> +      // delete in front should not cause crash
> +      request = mozIndexedDB.deleteDatabase(name, description);

deleteDatabase doesn't take a 'description' argument.

@@ +49,5 @@
> +      request.onerror = errorHandler;
> +      request.onsuccess = grabEventAndContinueHandler;
> +      let event = yield;
> +
> +      let db = event.target.result;

This will overwrite the first db... I suggest renaming them db1 and db2.

Comment 6

[Shawn Gong]

Attachment: Changes according to ben's

Thanks ben! And I expect to see many problems with this patch as well - thanks for the effort :)

Comment 7

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

Comment on attachment 537837 [details] [diff] [review]
Changes according to ben's

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

Still looks pretty good but there are a few problems. We can sit down and talk about them tomorrow to make sure we have a clear idea of what this code is supposed to be doing.

::: dom/indexedDB/IDBFactory.cpp
@@ +532,5 @@
>  }
>  
> +nsresult
> +RemoveDatabaseFile(const nsACString& aASCIIOrigin,
> +                const nsAString& aName)

So it looks to me like this is all more or less duplicated from CreateDatabaseConnection. We'll need to factor all that out into a single method, maybe something like this:

  already_AddRefed<nsIFile>
  GetDatabaseFile(const nsACString& aASCIIOrigin,
                  const nsAString& aName);

That method could do all of the path building but leave all of the creation for later.

Then CreateDatabaseConnection would change to be this:

  nsresult
  CreateDatabaseConnection(const nsACString& aASCIIOrigin,
                           const nsAString& aName,
                           nsAString& aDatabaseFilePath,
                           mozIStorageConnection** aConnection)
  {
    nsCOMPtr<nsIFile> dbFile =
      GetDatabaseFile(aASCIIOrigin, aName);

    // Actually create file and directory
    // Get parent, set quota on parent directory
    // Create storage connection, schema check, etc.
  }

And then the RemoveDatabaseFile would change to this:

  nsresult
  RemoveDatabaseFile(const nsACString& aASCIIOrigin,
                     const nsAString& aName)
  {
    nsCOMPtr<nsIFile> dbFile =
      GetDatabaseFile(aASCIIOrigin, aName);

    // Delete the file
  }

Make sense?

@@ +979,5 @@
> +IDBFactory::GetRequestAndIndexedDBManager(const nsAString& aName,
> +                                          nsCString& aOrigin, 
> +                                          IDBRequest** aRequest, 
> +                                          IndexedDatabaseManager** aManager,
> +                                          nsPIDOMWindow** aWindow = nsnull)

I think this is actually a little over-consolidated :)

Make this instead more simple:

  nsresult
  IDBFactory::GetOrigin(nsCString& aOrigin)
  {
    // Get origin from mWindow.
    // Cache the origin so we don't have to do this again.
  }

We've never had two places where we needed the origin so it wasn't cached before. I think it should be now. Also, don't worry about passing out the IndexedDatabaseManager or IDBRequest or nsPIDOMWindow, that just makes these more complicated.

@@ +1258,5 @@
> +nsresult
> +DeleteDatabaseHelper::GetSuccessResult(JSContext* aCx,
> +                                     jsval *aVal)
> +{
> +  nsRefPtr<IndexedDatabaseManager> mgr = IndexedDatabaseManager::GetOrCreate();

GetOrCreate is wrong here, the manager must exist if you've gotten here. Just do Get() and assert mgr.

@@ +1261,5 @@
> +{
> +  nsRefPtr<IndexedDatabaseManager> mgr = IndexedDatabaseManager::GetOrCreate();
> +  NS_ENSURE_TRUE(mgr, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +  mgr->DatabaseSuccessfullyDeleted(mName);

Hm, this only gets called on success... It's always possible that something could fail, and if it did you'd never call DatabaseSuccessfullyDeleted. I think you need your DeleteDatabaseHelper to also override ReleaseMainThreadObjects and call this function there.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +142,5 @@
>  public:
>    VersionChangeEventsRunnable(
>                              IDBDatabase* aRequestingDatabase,
> +                            IDBVersionChangeRequest* aVersionChangeRequest,
> +                            IDBRequest* aRequest,

No need for two args here, you're storing to the same member anyway.

@@ +437,5 @@
> +  NS_ERROR("Didn't find the corresponding DeleteDatabaseRunnable!");
> +}
> +
> +nsresult
> +IndexedDatabaseManager::WaitForDatabasesToClose(const nsAString& aName,

Looks like all of this is copied from SetDatabaseVersion, we'll want to make some kind of helper to get all live databases given name and origin, something like this:

  nsresult
  IndexedDatabaseManager::GetAllLiveDatabases(
              const nsAString& aName,
              const nsACString& aOrigin,
              nsTArray<nsRefPtr<IDBDatabase> >& aLiveDatabases);

@@ +483,5 @@
> +
> +  nsresult rv = NS_DispatchToCurrentThread(eventsRunnable);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Make a new entry for this origin in mOriginClearRunnables.

Comment is wrong.

@@ +492,5 @@
> +    mDeleteDatabaseRunnables.AppendElement(runnable);
> +  NS_ENSURE_TRUE(newRunnable, NS_ERROR_OUT_OF_MEMORY);
> +
> +  // Dispatch the deleteHelper
> +  return aHelper->Dispatch(IOThread());

This is wrong... You're dispatching the thing that will delete the database file before your VersionChangeEventsRunnable has completed, so other pages that are currently using the database will suddenly be touching a deleted file.

I think I see what's confusing you, you're following the example of SetDatabaseVersion. That one works very differently from what you want here, I'd recommend looking at the way that ClearDatabasesForURI works. We can sit down and go over it tomorrow.

Comment 8

[Shawn Gong]

Attachment: Patch 1

Comment 9

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

Comment on attachment 539056 [details] [diff] [review]
Patch 1

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

::: dom/indexedDB/IDBFactory.cpp
@@ +378,5 @@
>  
>  nsresult
> +GetDatabaseFile(const nsACString& aASCIIOrigin,
> +                const nsAString& aName,
> +                nsIFile** aFile)

Let's change this to:

  already_AddRefed<nsIFile>
  GetDatabaseFile(const nsACString& aASCIIOrigin,
                  const nsAString& aName);

@@ +388,5 @@
>    nsCOMPtr<nsIFile> dbDirectory;
>    nsresult rv = IDBFactory::GetDirectory(getter_AddRefs(dbDirectory));
>  
>    PRBool exists;
>    rv = dbDirectory->Exists(&exists);

There's no need to do this exists check in this function.

@@ +422,1 @@
>      rv = dbDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);

Hm, you're still doing file creation in this function. Let's make sure all that goes into CreateDatabaseConnection.

@@ +430,1 @@
>    rv = dbDirectory->Append(NS_LITERAL_STRING("*"));

This isn't needed here.

@@ +466,5 @@
> +CreateDatabaseConnection(const nsACString& aASCIIOrigin,
> +                         const nsAString& aName,
> +                         nsAString& aDatabaseFilePath,
> +                         mozIStorageConnection** aConnection)
> +{

Please assert that we're not on the main thread here, and that the arguments are non-empty, just as before.

@@ +467,5 @@
> +                         const nsAString& aName,
> +                         nsAString& aDatabaseFilePath,
> +                         mozIStorageConnection** aConnection)
> +{
> +  aDatabaseFilePath.Truncate();

Nit: Newline after this.

@@ +477,2 @@
>    rv = dbFile->Exists(&exists);
>    NS_ENSURE_SUCCESS(rv, rv);

This is not necessary, you're not checking the result anyway.

@@ +480,5 @@
> +  nsCOMPtr<mozIStorageServiceQuotaManagement> ss =
> +    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
> +  NS_ENSURE_TRUE(ss, NS_ERROR_FAILURE);
> +
> +  nsCString pattern;

Nit: Move this down a few lines right before GetNativePath.

@@ +482,5 @@
> +  NS_ENSURE_TRUE(ss, NS_ERROR_FAILURE);
> +
> +  nsCString pattern;
> +  nsCOMPtr<nsIFile> dbDirectory;
> +  dbFile->GetParent(getter_AddRefs(dbDirectory));

Here you need to check the return value.

@@ +489,5 @@
> +  
> +  if (gIndexedDBQuota > 0) {
> +    PRUint64 quota = PRUint64(gIndexedDBQuota) * 1024 * 1024;
> +    nsRefPtr<QuotaCallback> callback(new QuotaCallback());
> +    rv = ss->SetQuotaForFilenamePattern(pattern, quota, callback, nsnull);

This is wrong, you're not appending the '*' like before.

@@ +1105,5 @@
>                      aVal);
>  }
> +
> +nsresult
> +DeleteDatabaseHelper::DoDatabaseWork(mozIStorageConnection* aConnection)

Nit: Move all these DeleteDatabaseHelper member functions to the end of the file.

@@ +1120,5 @@
> +  NS_ASSERTION(!aConnection, "Huh?!");
> +
> +  nsCOMPtr<nsIFile> dbFile;
> +  nsresult rv = GetDatabaseFile(mASCIIOrigin, mName, getter_AddRefs(dbFile));
> +  NS_ENSURE_SUCCESS(rv, rv);

Returning rv here is incorrect, you should return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR.

@@ +1124,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  PRBool exists;
> +  rv = dbFile->Exists(&exists);
> +  NS_ENSURE_SUCCESS(rv, rv);

Same here.

@@ +1126,5 @@
> +  PRBool exists;
> +  rv = dbFile->Exists(&exists);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

Extra check of rv is unnecessary.

@@ +1147,5 @@
> +}
> +
> +nsresult
> +DeleteDatabaseHelper::GetSuccessResult(JSContext* aCx,
> +                                     jsval *aVal)

Nit: Indent is off here.

@@ +1154,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +IDBFactory::GetOrigin()

I think we should rename this function. It's not returning the origin so 'GetOrigin' is a little misleading. How about 'EnsureCachedOrigin'.

@@ +1155,5 @@
> +}
> +
> +nsresult
> +IDBFactory::GetOrigin()
> +{

Please assert that this is being called on the main thread only.

@@ +1216,5 @@
> +  nsRefPtr<IDBVersionChangeRequest> request = 
> +    IDBVersionChangeRequest::Create(this, context, window, nsnull);
> +  NS_ENSURE_TRUE(request, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +   nsRefPtr<DeleteDatabaseHelper> deleteHelper =

Nit: Indent is off here.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +148,2 @@
>                              nsTArray<nsRefPtr<IDBDatabase> >& aWaitingDatabases,
> +                            const nsAString& aVersion = EmptyString())

This optional version thing seems wrong. We don't need a version passed in here, we can use EmptyString when we fire the version change events.

@@ +154,5 @@
>      NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    NS_ASSERTION(aHelper, "helper is null!");
> +    mHelper = aHelper;
> +    NS_ASSERTION(aRequest, "IDBRequest is null!");
> +    mRequest = aRequest;

Put all of these assignments in the initializer list.

@@ +215,5 @@
> +      }
> +    }
> +
> +    // Now set up our callback so that we know when they have finished.
> +    TransactionThreadPool* pool = TransactionThreadPool::GetOrCreate();

You don't want GetOrCreate here... You just want Get. If it hasn't been created yet then we don't have to wait!

@@ +219,5 @@
> +    TransactionThreadPool* pool = TransactionThreadPool::GetOrCreate();
> +    NS_ENSURE_TRUE(pool, NS_ERROR_FAILURE);
> +
> +    // Use the DeleteDatabaseHelper as the callback.
> +    if (!pool->WaitForAllDatabasesToComplete(mWaitingDatabases, mHelper)) {

This is wrong. The thread pool will run the thing you pass to it on the main thread, and your helper is supposed to run on the IO thread. You need one more level of indirection here.

This means that the code to delete the actual file is never running. We need to identify some way to test this behavior.

@@ +268,5 @@
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +    // Fire version change events at all of the databases that are not already
> +    // closed. Also kick bfcached documents out of bfcache.
> +    for (PRUint32 index = 0; index < mWaitingDatabases.Length(); index++) {

Since all of this code is identical I think we should separate it out into a base class method. Then we won't have to duplicate it.

Comment 10

[Shawn Gong]

Attachment: Patch 2

Here you go - appreciate the effort! (:

Comment 11

[Shawn Gong]

Attachment: Patch 3

minor updates

Comment 12

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

Comment on attachment 539355 [details] [diff] [review]
Patch 2

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

::: dom/indexedDB/IDBFactory.cpp
@@ +386,3 @@
>  
>    nsCOMPtr<nsIFile> dbDirectory;
>    nsresult rv = IDBFactory::GetDirectory(getter_AddRefs(dbDirectory));

Need to check the result here.

@@ +395,4 @@
>  
>    nsCOMPtr<nsIFile> dbFile;
>    rv = dbDirectory->Clone(getter_AddRefs(dbFile));
> +  NS_ENSURE_SUCCESS(rv, nsnull);

This clone is unnecessary (after this you never use dbDirectory, right?). Just rename the first comptr dbFile and then skip the clone.

@@ +438,5 @@
> +  NS_ASSERTION(!aASCIIOrigin.IsEmpty() && !aName.IsEmpty(), "Bad arguments!");
> +  
> +  aDatabaseFilePath.Truncate();
> +
> +  nsCOMPtr<nsIFile> dbFile = GetDatabaseFile(aASCIIOrigin, aName);

dbFile may be null if something failed. Need to check the result and return NS_ERROR_FAILURE if it is null.

@@ +1085,5 @@
> +
> +nsresult
> +IDBFactory::EnsureCachedOrigin()
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Nit: Newline here.

@@ +1173,5 @@
> +  }
> +#endif
> +  NS_ASSERTION(!aConnection, "Huh?!");
> +
> +  nsCOMPtr<nsIFile> dbFile = GetDatabaseFile(mASCIIOrigin, mName);

This can be null again, you need to check.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +143,5 @@
> +  
> +  NS_IMETHOD Run()
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    // Otherwise dispatch to the IO thread to actually compute the usage.

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

@@ +145,5 @@
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    // Otherwise dispatch to the IO thread to actually compute the usage.
> +    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
> +    NS_ASSERTION(mgr, "This should never be null!");

Nit: Newline here.

@@ +147,5 @@
> +    // Otherwise dispatch to the IO thread to actually compute the usage.
> +    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
> +    NS_ASSERTION(mgr, "This should never be null!");
> +    nsresult rv = mgr->IOThread()->Dispatch(mRunnable, NS_DISPATCH_NORMAL);
> +    NS_ENSURE_SUCCESS(rv, rv);

Nit: and here.

@@ +183,5 @@
>    {
>      NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
> +    nsresult rv = mgr->FireVersionChangeEvent(mWaitingDatabases);

Hm. I like that this code got consolidated, but I don't think it needs to live in the manager. How about you just put it into a base class for this runnable? We can talk about this a bit tomorrow.

@@ +1322,5 @@
> +  // SetVersion transaction to complete.
> +  for (PRUint32 index = 0; index < mDelayedRunnables.Length(); index++) {
> +    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
> +    NS_ASSERTION(mgr, "This should never be null!");
> +    nsresult rv = mgr->IOThread()->Dispatch(mDelayedRunnables[index], NS_DISPATCH_NORMAL);

Any queued runnables were destined for the main thread, not the IO thread. You're sending them to the wrong thread :)

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +185,5 @@
> +    nsString mName;
> +    nsTArray<nsCOMPtr<nsIRunnable> > mDelayedRunnables;
> +  };
> + 
> +  inline void OnDeleteDatabaseRunnableComplete(DeleteDatabaseRunnable* aRunnable);

This isn't inlined.

Comment 13

[Shawn Gong]

Attachment: Patch 4

Here you go!

Comment 14

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

Attachment: Patch

Needs some more tests around interactions with origin clearing, but this should be fine.

Comment 15

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

Note that this patch requires the patches in Bug 699468 to not assert a bunch after deleting a database.

Comment 16

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

Comment on attachment 571162 [details] [diff] [review]
Patch

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

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +652,5 @@
>  
> +  template <class T>
> +  static
> +  void QueueVersionChange(nsTArray<nsRefPtr<IDBDatabase> >& aDatabases,
> +                          void* aClosure);

Make aClosure be T* here.

@@ +856,5 @@
>    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
>    NS_ASSERTION(mgr, "This should never be null!");
>  
>    rv = mgr->AcquireExclusiveAccess(mDatabase, helper,
> +            &VersionChangeEventsRunnable::QueueVersionChange<SetVersionHelper>,

Then you won't have to be explicit like this.

@@ +933,3 @@
>  
> +    switch (mState) {
> +    case eSetVersionCompleted: {

Nit: extra level of indentation here.

@@ +1239,3 @@
>  // static
>  void
> +VersionChangeEventsRunnable::QueueVersionChange(

Nit: put the template line below the comment

@@ +1302,5 @@
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +  NS_ASSERTION(dbFile, "What?");
> +  rv = dbFile->Remove(false);
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

This is a little harsh, how about doing an exists() check first, and only try removing it if it exists.

::: dom/indexedDB/test/test_deleteDatabase.html
@@ +13,5 @@
> +
> +  function testSteps()
> +  {
> +    const READ_WRITE = Components.interfaces.nsIIDBTransaction.READ_WRITE;
> +    const VERSION_CHANGE = Components.interfaces.nsIIDBTransaction.VERSION_CHANGE;

FYI these could be 'IDBTransaction.READ_WRITE' etc.

@@ +56,5 @@
> +    is(db2.objectStoreNames.length, 1, "Expect an objectStore here");
> +
> +    var onversionchangecalled = false;
> +
> +    function closeDBs(event) {

Hm, shouldn't this be called twice? Maybe just do event.target.close()?

@@ +60,5 @@
> +    function closeDBs(event) {
> +      onversionchangecalled = true;
> +      ok(event instanceof IDBVersionChangeEvent, "expect a versionchange event");
> +      is(event.oldVersion, 10, "oldVersion should be 10");
> +      todo(event.newVersion, null, "newVersion should be null");

why is this todo?

Comment 17

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

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

Comment 18

[Jean-Yves Perrier [:teoli]]

Documented in https://developer.mozilla.org/en/IndexedDB/IDBFactory
and
https://developer.mozilla.org/en/Firefox_10_for_developers