701772 - IndexedDB: Autoincrement should not share counter between object stores

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

The spec states that each object store should use a separate counter which starts at 1 and is increased by on on each insertion.

Currently we share the counter between all objectStores for a given database.

While we're fixing this, we should rip out the separate auto-increment tables in sqlite. They are causing us nothing but pain.

Comment 1

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

Ben was going to take this one and do the schema changes too, I believe.

Comment 2

[Jan Varga [:janv]]

(In reply to Jonas Sicking (:sicking) from comment #0)
> While we're fixing this, we should rip out the separate auto-increment
> tables in sqlite. They are causing us nothing but pain.

uhm, are these really going away ?

Comment 3

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

Attachment: WIP

This is very much a work in progress. The main remaining part is the handling of the ObjectStoreInfo objects which currently doesn't work.

I also haven't verified that all the cursor stuff still passes tests, but it mostly should.

Comment 4

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

The changes to IDBFactory::UpdateDatabaseMetadata should be reverted

Comment 5

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

Comment on attachment 579971 [details] [diff] [review]
WIP

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

::: dom/indexedDB/IDBFactory.cpp
@@ +259,5 @@
>        rv = stmt->GetString(2, info->keyPath);
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>  
> +    info->nextAutoIncrementId = !!stmt->AsInt32(3);

This should be AsInt64, and you don't want those !! do you?

::: dom/indexedDB/IDBIndex.cpp
@@ +701,5 @@
>    mKeyRange->GetBindingClause(NS_LITERAL_CSTRING("value"), keyRangeClause);
>  
>    NS_ASSERTION(!keyRangeClause.IsEmpty(), "Huh?!");
>  
>    NS_NAMED_LITERAL_CSTRING(indexId, "index_id");

If you're not going to use this then more than once I'd rather you remove this and just put NS_LITERAL_STRING when you call Bind.

@@ +757,5 @@
>    mKeyRange->GetBindingClause(NS_LITERAL_CSTRING("value"), keyRangeClause);
>  
>    NS_ASSERTION(!keyRangeClause.IsEmpty(), "Huh?!");
>  
>    NS_NAMED_LITERAL_CSTRING(indexId, "index_id");

Here too.

@@ +826,5 @@
>      limitClause = NS_LITERAL_CSTRING(" LIMIT ");
>      limitClause.AppendInt(mLimit);
>    }
>  
>    NS_NAMED_LITERAL_CSTRING(indexId, "index_id");

And here.

@@ +926,3 @@
>    }
>  
>    NS_NAMED_LITERAL_CSTRING(indexId, "index_id");

And here.

@@ +941,5 @@
> +  nsCString query = NS_LITERAL_CSTRING("SELECT data FROM object_data "
> +                                       "INNER JOIN ") + indexTable +
> +                    NS_LITERAL_CSTRING(" AS index_table ON object_data.id = "
> +                                       "index_table.object_data_id "
> +                                       "WHERE indexId = :indexId") +

This is wrong, it's 'index_id'.

@@ +1010,2 @@
>  
>    NS_NAMED_LITERAL_CSTRING(value, "value");

This one too?

@@ +1070,5 @@
>  
>    // Now we need to make the query to get the next match.
> +  nsCAutoString queryStart = NS_LITERAL_CSTRING("SELECT value, object_data_key"
> +                                                " FROM ") + table +
> +                             NS_LITERAL_CSTRING(" WHERE index_id = :id");

Since you changed this let's stick with :index_id, not :id. Consistency is probably more important than anything here.

@@ +1082,5 @@
>                                queryStart);
>          mRangeKey = mKeyRange->Upper();
>        }
> +      mContinueQuery =
> +        queryStart +

Not sure what to do about this style of wrapping... I see why you did it, but lots of other code uses the other style. I'd rather we be consistent one way or the other.

@@ +1085,5 @@
> +      mContinueQuery =
> +        queryStart +
> +        NS_LITERAL_CSTRING(" AND value >= :current_key AND "
> +                           "( value > :current_key OR "
> +                           "  object_data_key > :object_key )") +

Nuke those extra spaces at the beginning of this line.

@@ +1122,5 @@
> +      mContinueQuery =
> +        queryStart +
> +        NS_LITERAL_CSTRING(" AND value <= :current_key AND "
> +                           "( value < :current_key OR "
> +                           "  object_data_key < :object_key )") +

Nuke those extra spaces at the beginning of this line.

@@ +1189,3 @@
>    }
>  
> +  NS_NAMED_LITERAL_CSTRING(value, "index_table.value");

Think you can remove this one too.

@@ +1216,5 @@
>      default:
>        NS_NOTREACHED("Unknown direction!");
>    }
>  
>    NS_NAMED_LITERAL_CSTRING(id, "id");

Remove.

@@ +1284,5 @@
>        }
> +      mContinueQuery =
> +        queryStart +
> +        NS_LITERAL_CSTRING(" AND index_table.value >= :current_key AND "
> +                           "(index_table.value > :current_key OR "

Nit: Add a space after (

@@ +1318,5 @@
>        }
> +      mContinueQuery =
> +        queryStart +
> +        NS_LITERAL_CSTRING(" AND index_table.value <= :current_key AND "
> +                           "(index_table.value < :current_key OR "

Here too.

@@ +1319,5 @@
> +      mContinueQuery =
> +        queryStart +
> +        NS_LITERAL_CSTRING(" AND index_table.value <= :current_key AND "
> +                           "(index_table.value < :current_key OR "
> +                           " index_table.object_data_key < :object_key) ") +

And before )

@@ +1397,5 @@
>                              !mKeyRange->IsUpperOpen(), keyRangeClause);
>      }
>    }
>  
>    NS_NAMED_LITERAL_CSTRING(id, "id");

Remove.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +539,2 @@
>    objectStore->mDatabaseId = aStoreInfo->databaseId;
> +  objectStore->mInfo = aStoreInfo;

Hrm. I don't know why you're stashing this. The idea is that the objectstore can simply look this stuff up with its id... If you don't do that then there's no reason for objectStore to hold any other state, really.

@@ +1620,5 @@
> +  // The "&& !keyUnset" here is mostly a debugging tool. If a key isn't
> +  // specified we should never have a collision and so it shouldn't matter
> +  // if we allow overwrite or not. By not allowing overwrite we raise
> +  // detectable errors rather than corrupting data
> +  stmt = mTransaction->AddStatement(mOverwrite && !keyUnset);

Nit: Combine with decl above.

@@ +1635,5 @@
> +               "Should have key unless autoincrement");
> +
> +  if (mObjectStore->IsAutoIncrement()) {
> +    if (keyUnset) {
> +      mKey.SetFromInteger(mObjectStore->Info()->nextAutoIncrementId++);

I'd be ok with splitting this into two statements (++ and then SetFromInteger).

@@ +1671,5 @@
> +
> +  rv = stmt->Execute();
> +  if (rv == NS_ERROR_STORAGE_CONSTRAINT) {
> +    NS_ASSERTION(!keyUnset, "Generated key had a collision!?");
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;

So... This can totally happen, right? All the web page would have to do is make an autoincrement store, then add something with PR_INT64_MIN (or maybe 0, i can't remember if SQLite uses signed or unsigned, but I bet signed) as its key, and then add something with PR_INT64_MAX as its key. On the very next insert we'd hit this assertion. 

Not sure what we should do here. Does the spec promise monotonic keys? SQLite will loop back around and start from 0 I think, looking for the next highest unused id. Sounds very dumb and slow, but not sure if there are any better options?

@@ +1673,5 @@
> +  if (rv == NS_ERROR_STORAGE_CONSTRAINT) {
> +    NS_ASSERTION(!keyUnset, "Generated key had a collision!?");
> +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> +  }
> +  NS_ENSURE_SUCCESS(rv, rv);

rv won't be right here, you need NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR.

@@ +1714,4 @@
>  
>    NS_ASSERTION(!keyRangeClause.IsEmpty(), "Huh?!");
>  
>    NS_NAMED_LITERAL_CSTRING(osid, "osid");

Nuke this.

@@ +1766,4 @@
>  
>    NS_ASSERTION(!keyRangeClause.IsEmpty(), "Huh?!");
>  
>    NS_NAMED_LITERAL_CSTRING(osid, "osid");

And this.

@@ +1826,1 @@
>    NS_NAMED_LITERAL_CSTRING(id, "id");

Nuke this.

@@ +1927,5 @@
>    }
>  
> +  mContinueQuery = NS_LITERAL_CSTRING("SELECT key_value, data "
> +                                      "FROM object_data "
> +                                      "WHERE object_store_id = :id") +

This part is the exact same as below, you could combine into a common queryStart string.

@@ +2212,1 @@
>    NS_NAMED_LITERAL_CSTRING(osid, "osid");

I think you can get rid of this too.

@@ +2313,1 @@
>    NS_NAMED_LITERAL_CSTRING(osid, "osid");

Lose this one too.

::: dom/indexedDB/IDBObjectStore.h
@@ +151,5 @@
>    }
> +  
> +  // Warning! This method returns null after the transaction is
> +  // closed
> +  ObjectStoreInfo* Info()

See below for a way to avoid this maybe-null, but if you decide to keep things like this then rename to GetInfo then.

@@ +191,5 @@
>    nsString mKeyPath;
>    bool mAutoIncrement;
>    nsCOMPtr<nsIAtom> mDatabaseId;
> +  ObjectStoreInfo* mInfo; // Note! This can be a dangling pointer after the
> +                          // transaction is closed

Boo... The transaction knows all its objectStores, why not have it call some function to null this out? Dangling pointers are awful.

::: dom/indexedDB/IDBTransaction.cpp
@@ +353,5 @@
>    return NS_OK;
>  }
>  
>  already_AddRefed<mozIStorageStatement>
> +IDBTransaction::AddStatement(bool aOverwrite)

Now that this is so simple please just remove this and put the SQL inline in the one place it's used (once each for overwrite/non-overwrite).

@@ +368,5 @@
>    );
>  }
>  
>  already_AddRefed<mozIStorageStatement>
> +IDBTransaction::IndexDataInsertStatement(bool aUnique)

Same here.

@@ +385,5 @@
>    );
>  }
>  
>  already_AddRefed<mozIStorageStatement>
> +IDBTransaction::IndexDataDeleteStatement(bool aUnique)

And here.

@@ +468,5 @@
> +IDBTransaction::WriteAutoIncrementCounts(mozIStorageConnection* aConnection)
> +{
> +  nsCOMPtr<mozIStorageStatement> stmt;
> +  nsresult rv;
> +  for (PRUint32 i = 0; i < mCreatedObjectStores.Length(); i++) {

mCreatedObjectStores is a mainthread only thing... I think this technically can't race with the web page because we'd never be here if the transaction was still open (and we won't let you get more objectStores after the transaction is closed) but this is another footgun.

I think you should just get a list of ObjectStoreInfo* when you create and fire the CommitHelper and then loop over that. Then you can move all this logic into the CommitHelper, which seems like a better fit to me anyway.

@@ +885,5 @@
>    if (mConnection) {
>      IndexedDatabaseManager::SetCurrentWindow(database->Owner());
>  
>      if (!mAborted) {
> +      if (NS_FAILED(mTransaction->WriteAutoIncrementCounts(mConnection))) {

if (!mAborted && NS_FAILED())

@@ +895,2 @@
>        NS_NAMED_LITERAL_CSTRING(release, "COMMIT TRANSACTION");
>        if (NS_FAILED(mConnection->ExecuteSimpleSQL(release))) {

Can we switch this now to:

  if (NS_SUCCEEDED(release)) {
    // Commit autoincr
  }
  else {
    mAborted = true;
  }

::: dom/indexedDB/IDBTransaction.h
@@ +160,5 @@
>    }
>  
> +  bool IsDone() const
> +  {
> +    return mReadyState == nsIIDBTransaction::DONE;

This isn't ok... mReadyState is a main thread only thing so you should assert that this is only called on the main thread... Except you call it on the DB thread ;) Even if higher level logic means that you can call this safely I think we should still figure out a better way (it's a footgun otherwise).

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +835,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +UpgradeSchemaFrom8To9(mozIStorageConnection* aConnection)

Heh, I guess this will need to be reworked slightly now that snappy is in.

@@ +838,5 @@
>  nsresult
> +UpgradeSchemaFrom8To9(mozIStorageConnection* aConnection)
> +{
> +  mozStorageTransaction transaction(aConnection, false,
> +                                 mozIStorageConnection::TRANSACTION_IMMEDIATE);

No longer needed.

@@ +842,5 @@
> +                                 mozIStorageConnection::TRANSACTION_IMMEDIATE);
> +
> +  // Turn off foreign key constraints before we do anything here.
> +  nsresult rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "PRAGMA foreign_keys = OFF;"

No longer (never was) needed.

@@ +909,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT INTO index_data (index_id, value, object_data_key, object_data_id) "
> +      "SELECT ai_index_data.index_id, ai_index_data.value, ai_index_data.ai_object_data_id, object_data.id"

Needs a space at the end of the line! (pretty please test this!)

This also looks longer than 80 chars.

@@ +915,5 @@
> +      "INNER JOIN object_store_index ON "
> +        "object_store_index.id = ai_index_data.index_id "
> +      "INNER JOIN object_data ON "
> +        "object_data.object_store_id = object_store_index.object_store_id AND "
> +        "object_data.key_value = ai_index_data.ai_object_data_id;"

Double join?! Blegh, this is going to be slow... Maybe I'll have an amazing dream on this flight where I figure out a magic O(1) solution to this problem...

Comment 6

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

> ::: dom/indexedDB/IDBFactory.cpp
> @@ +259,5 @@
> >        rv = stmt->GetString(2, info->keyPath);
> >        NS_ENSURE_SUCCESS(rv, rv);
> >      }
> >  
> > +    info->nextAutoIncrementId = !!stmt->AsInt32(3);
> 
> This should be AsInt64, and you don't want those !! do you?

Good catch!

> ::: dom/indexedDB/IDBIndex.cpp
> @@ +701,5 @@
> >    mKeyRange->GetBindingClause(NS_LITERAL_CSTRING("value"), keyRangeClause);
> >  
> >    NS_ASSERTION(!keyRangeClause.IsEmpty(), "Huh?!");
> >  
> >    NS_NAMED_LITERAL_CSTRING(indexId, "index_id");
> 
> If you're not going to use this then more than once I'd rather you remove
> this and just put NS_LITERAL_STRING when you call Bind.

Done here and everywhere else where we create a NS_NAMED_LITERAL_CSTRING and only used it once.

> @@ +941,5 @@
> > +  nsCString query = NS_LITERAL_CSTRING("SELECT data FROM object_data "
> > +                                       "INNER JOIN ") + indexTable +
> > +                    NS_LITERAL_CSTRING(" AS index_table ON object_data.id = "
> > +                                       "index_table.object_data_id "
> > +                                       "WHERE indexId = :indexId") +
> 
> This is wrong, it's 'index_id'.

Fixed!

> @@ +1070,5 @@
> >  
> >    // Now we need to make the query to get the next match.
> > +  nsCAutoString queryStart = NS_LITERAL_CSTRING("SELECT value, object_data_key"
> > +                                                " FROM ") + table +
> > +                             NS_LITERAL_CSTRING(" WHERE index_id = :id");
> 
> Since you changed this let's stick with :index_id, not :id. Consistency is
> probably more important than anything here.

The cursor code currently expects to set a variable with name "id" which sometimes is an objectstore and sometimes an index?

> @@ +1082,5 @@
> >                                queryStart);
> >          mRangeKey = mKeyRange->Upper();
> >        }
> > +      mContinueQuery =
> > +        queryStart +
> 
> Not sure what to do about this style of wrapping... I see why you did it,
> but lots of other code uses the other style. I'd rather we be consistent one
> way or the other.

With the other way I end up having to do way too much wrapping of the actual query, and I'd rather leave the sql readable, so left this as-is.

We've never been consistent about line breaking when running out of columns, just do whatever produces the most readable code.

> @@ +1085,5 @@
> > +      mContinueQuery =
> > +        queryStart +
> > +        NS_LITERAL_CSTRING(" AND value >= :current_key AND "
> > +                           "( value > :current_key OR "
> > +                           "  object_data_key > :object_key )") +
> 
> Nuke those extra spaces at the beginning of this line.

I'd rather keep the sql readable.

> @@ +1284,5 @@
> >        }
> > +      mContinueQuery =
> > +        queryStart +
> > +        NS_LITERAL_CSTRING(" AND index_table.value >= :current_key AND "
> > +                           "(index_table.value > :current_key OR "
> 
> Nit: Add a space after (

Fixed these.

> ::: dom/indexedDB/IDBObjectStore.cpp
> @@ +539,2 @@
> >    objectStore->mDatabaseId = aStoreInfo->databaseId;
> > +  objectStore->mInfo = aStoreInfo;
> 
> Hrm. I don't know why you're stashing this. The idea is that the objectstore
> can simply look this stuff up with its id... If you don't do that then
> there's no reason for objectStore to hold any other state, really.

I did this to avoid having to look up in the hash all the time. Additionally, the objectstore could have been removed from the hash if someone does a lot of inserts/removes and then deletes the objectStore. This is actually a useful scenario if someone uses a temporary objectStore to perform calculations.

> @@ +1671,5 @@
> > +
> > +  rv = stmt->Execute();
> > +  if (rv == NS_ERROR_STORAGE_CONSTRAINT) {
> > +    NS_ASSERTION(!keyUnset, "Generated key had a collision!?");
> > +    return NS_ERROR_DOM_INDEXEDDB_CONSTRAINT_ERR;
> 
> So... This can totally happen, right? All the web page would have to do is
> make an autoincrement store, then add something with PR_INT64_MIN (or maybe
> 0, i can't remember if SQLite uses signed or unsigned, but I bet signed) as
> its key, and then add something with PR_INT64_MAX as its key. On the very
> next insert we'd hit this assertion. 
> 
> Not sure what we should do here. Does the spec promise monotonic keys?
> SQLite will loop back around and start from 0 I think, looking for the next
> highest unused id. Sounds very dumb and slow, but not sure if there are any
> better options?

We discussed this on the list, but I don't think the decision made it into the spec. The consensus was that once we reach 2^53 we should just stop allowing using the key generator since we can't generate consecutive numbers (above 2^53 IEEE754 floats has too low precision to represent all integers).

> ::: dom/indexedDB/IDBObjectStore.h
> @@ +151,5 @@
> >    }
> > +  
> > +  // Warning! This method returns null after the transaction is
> > +  // closed
> > +  ObjectStoreInfo* Info()
> 
> See below for a way to avoid this maybe-null, but if you decide to keep
> things like this then rename to GetInfo then.
> 
> @@ +191,5 @@
> >    nsString mKeyPath;
> >    bool mAutoIncrement;
> >    nsCOMPtr<nsIAtom> mDatabaseId;
> > +  ObjectStoreInfo* mInfo; // Note! This can be a dangling pointer after the
> > +                          // transaction is closed
> 
> Boo... The transaction knows all its objectStores, why not have it call some
> function to null this out? Dangling pointers are awful.

It doesn't know of objectstores which have been deleted. Such objectstores can still have queued requests which will use this info.

Hmm... If I could store the autoincrement-count, rather than the objectstoreinfo, in the objectstore then this would go away and a lot of things would be cleaner. The problem is that I don't know what the count is going to be when the objectstore is created :(

> @@ +468,5 @@
> > +IDBTransaction::WriteAutoIncrementCounts(mozIStorageConnection* aConnection)
> > +{
> > +  nsCOMPtr<mozIStorageStatement> stmt;
> > +  nsresult rv;
> > +  for (PRUint32 i = 0; i < mCreatedObjectStores.Length(); i++) {
> 
> mCreatedObjectStores is a mainthread only thing... I think this technically
> can't race with the web page because we'd never be here if the transaction
> was still open (and we won't let you get more objectStores after the
> transaction is closed) but this is another footgun.
> 
> I think you should just get a list of ObjectStoreInfo* when you create and
> fire the CommitHelper and then loop over that. Then you can move all this
> logic into the CommitHelper, which seems like a better fit to me anyway.

I'll give it a shot.


> ::: dom/indexedDB/IDBTransaction.h
> @@ +160,5 @@
> >    }
> >  
> > +  bool IsDone() const
> > +  {
> > +    return mReadyState == nsIIDBTransaction::DONE;
> 
> This isn't ok... mReadyState is a main thread only thing so you should
> assert that this is only called on the main thread... Except you call it on
> the DB thread ;) Even if higher level logic means that you can call this
> safely I think we should still figure out a better way (it's a footgun
> otherwise).

This is really only needed for belts-and-braces reason since I was worried about the dangling pointer... We should try to find a better way.

> @@ +909,5 @@
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> > +    "INSERT INTO index_data (index_id, value, object_data_key, object_data_id) "
> > +      "SELECT ai_index_data.index_id, ai_index_data.value, ai_index_data.ai_object_data_id, object_data.id"
> 
> Needs a space at the end of the line! (pretty please test this!)
> 
> This also looks longer than 80 chars.

Yeah, this needs real testing. I suspect there are more errors in there.

> @@ +915,5 @@
> > +      "INNER JOIN object_store_index ON "
> > +        "object_store_index.id = ai_index_data.index_id "
> > +      "INNER JOIN object_data ON "
> > +        "object_data.object_store_id = object_store_index.object_store_id AND "
> > +        "object_data.key_value = ai_index_data.ai_object_data_id;"
> 
> Double join?! Blegh, this is going to be slow... Maybe I'll have an amazing
> dream on this flight where I figure out a magic O(1) solution to this
> problem...

Let me know if you think of anything ;-)

I didn't spend too much time optimizing this since it's just upgrade code so few people will hit it.

Comment 7

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

Attachment: interdiff

Interdiff on top of previous patch. This passes all tests but is lacking new tests.

The big change here is that this makes ObjectStoreInfo's refcounted which avoids the whole mess of dangling pointers.

Additionally, this patch adds a few explicit calls to DatabaseInfo::Remove. This is needed since we can't count on DatabaseInfo's going away as easily as we used to. It used to be that for indexedDB.deleteDatabase, and killing the databases for an origin could rely on that calling IDBDatabase::Close/Invalidate would synchronously remove the databaseinfo from the hash since databases were the only think that held references to the databaseinfos. Now that transactions do too we have to explicitly remove things from the hash as needed. Note that all of this is unrelated to objectstoreinfos now being refcounted.

The one thing I'm debating is moving the nextAutoIncrementId member to IDBObjectStore. That removes the need to have any revert logic. If a transaction fails to commit we simply never touch the objectstoreinfos. Only on a successful commit do we copy the new counts from the IDBObjectStores to the objectstoreinfos.

Comment 8

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

Attachment: Full patch

Comment 9

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

Comment on attachment 580884 [details] [diff] [review]
interdiff

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

::: dom/indexedDB/DatabaseInfo.cpp
@@ +216,5 @@
>    }
>  }
>  
> +PLDHashOperator
> +EnumerateDatabasesRemoveOrigin(nsISupports* aId,

Please assert main thread.

@@ +222,5 @@
> +                               void* aUserArg)
> +{
> +  const nsACString* origin = static_cast<const nsACString*>(aUserArg);
> +  return aDatabaseInfo->origin.Equals(*origin) ?
> +    PL_DHASH_REMOVE : PL_DHASH_NEXT;

Nit: If it doesn't fit on one line,

  foo ?
  bar :
  baz;

@@ +227,5 @@
> +}
> +
> +// static
> +void
> +DatabaseInfo::RemoveAllForOrigin(const nsACString& aOrigin)

Please assert main thread.

::: dom/indexedDB/DatabaseInfo.h
@@ +85,4 @@
>    bool GetObjectStoreNames(nsTArray<nsString>& aNames);
>    bool ContainsStoreName(const nsAString& aName);
>  
> +  ObjectStoreInfo* GetObjectStore(const nsAString& aName);

Hm... This returns a weak ptr? Maybe indicate in the name.

@@ +144,5 @@
>    nsIAtom* databaseId;
>    nsTArray<IndexInfo> indexes;
> +
> +  // This is threadsafe since the ObjectStoreInfos are created on the database
> +  // thread but then used from the main thread. Ideal would be if we could

s/used/only used/ ?

::: dom/indexedDB/IDBObjectStore.cpp
@@ +652,5 @@
>  
>    if (aOverwrite) {
> +    stmt = aTransaction->GetCachedStatement(
> +      "DELETE FROM unique_index_data "
> +      "WHERE object_data_id = :object_data_id;"

Nit: Add an extra space at the end there.

@@ +677,5 @@
> +        "INSERT INTO unique_index_data "
> +          "(index_id, object_data_id, object_data_key, value) "
> +        "VALUES (:index_id, :object_data_id, :object_data_key, :value)") :
> +      aTransaction->GetCachedStatement(
> +        "INSERT OR IGNORE INTO index_data ("

Hm, remind me why we need OR IGNORE here?

@@ +1417,1 @@
>    if (!indexInfo) {

This is old code, now infallible right?

::: dom/indexedDB/IDBTransaction.cpp
@@ +526,5 @@
>    nsAutoTArray<nsString, 10> stackArray;
>    nsTArray<nsString>* arrayOfNames;
>  
>    if (mMode == IDBTransaction::VERSION_CHANGE) {
> +    if (!mDatabaseInfo->GetObjectStoreNames(stackArray)) {

Can this fail anymore?

@@ +716,5 @@
>  
>  CommitHelper::CommitHelper(IDBTransaction* aTransaction,
> +                           IDBTransactionListener* aListener,
> +                           const nsTArray<nsRefPtr<IDBObjectStore> >&
> +                             aUpdatedObjectStores)

Nit, please rework so |const nsTArray<nsRefPtr<IDBObjectStore> >& aUpdatedObjectStores| fits on one line, see IDBObjectStore::GetStructuredCloneDataFromStatement for an example.

@@ +791,5 @@
>    if (mConnection) {
>      IndexedDatabaseManager::SetCurrentWindow(database->Owner());
>  
> +    if (!mAborted &&
> +        NS_FAILED(WriteAutoIncrementCounts())) {

Nit, this will fit fine on a single line

@@ +858,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +CommitHelper::CommitAutoIncrementCounts()

This can be inlined in the header, right?

@@ +867,5 @@
> +  }
> +}
> +
> +void
> +CommitHelper::RevertAutoIncrementCounts()

This too.

Comment 10

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

> ::: dom/indexedDB/DatabaseInfo.h
> @@ +85,4 @@
> >    bool GetObjectStoreNames(nsTArray<nsString>& aNames);
> >    bool ContainsStoreName(const nsAString& aName);
> >  
> > +  ObjectStoreInfo* GetObjectStore(const nsAString& aName);
> 
> Hm... This returns a weak ptr? Maybe indicate in the name.

Get* functions that return a raw pointer should always be returning a weak pointer.

> @@ +677,5 @@
> > +        "INSERT INTO unique_index_data "
> > +          "(index_id, object_data_id, object_data_key, value) "
> > +        "VALUES (:index_id, :object_data_id, :object_data_key, :value)") :
> > +      aTransaction->GetCachedStatement(
> > +        "INSERT OR IGNORE INTO index_data ("
> 
> Hm, remind me why we need OR IGNORE here?

Due to multientry indexes. A multientry index might contain multiple entries with the same value, in which case we should just ignore the duplicates. We ignore the duplicates for unique indexes too, but we can't use "OR IGNORE" there so we do it manually.

> @@ +858,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +void
> > +CommitHelper::CommitAutoIncrementCounts()
> 
> This can be inlined in the header, right?

This leads to circular #includes unfortunately. I suspect we'll end up putting more stuff into these functions anyway. Any time we manage in-memory datastructures which cache stuff in the database we'll need to add here. I suspect we can use this to optimize FileInfo handling for example

Comment 11

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

Attachment: interdiff2

This fixes a few more issues. In particular, |1L << 53| doesn't work so well on 32-bit systems (evaluates to 0). |1LL << 53| works much better.

I also made the ObjectStoreInfo dtor private to make sure to catch all places where we were holding on to them using nsAutoPtrs.

This one passes on try!

Comment 12

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

Attachment: Full patch

Comment 13

[Jan Varga [:janv]]

I'm rebasing the patch using the files in idb patch.

Comment 14

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

Comment on attachment 581217 [details] [diff] [review]
interdiff2

Looks good!

Comment 15

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

Attachment: Tests + upgrade fixes

This is mostly tests, but also contains fixes to the upgrade queries. They actually work now!

Comment 16

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

Comment on attachment 581559 [details] [diff] [review]
Tests + upgrade fixes

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

::: dom/indexedDB/test/helpers.js
@@ +77,5 @@
>    {
>      is(event.type, "error", "Got an error event");
>      is(this._code, event.target.errorCode, "Expected error was thrown.");
>      event.preventDefault();
> +    event.stopPropagation();

Why is this needed?

Comment 17

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

Because otherwise the event bubbles up to the IDBDatabase object where there's an error handler registered. I looked through the current uses of ErrorExpected and in all cases we only register error handlers on the request and so they won't be affected by this change.

Comment 18

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

Checked in and bounced because of a leak. Found the leak (a remaining .forget() call) and got r=bent over irc. Now checked in as

https://hg.mozilla.org/integration/mozilla-inbound/rev/67eb3827b7eb

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/67eb3827b7eb

Comment 20

[Chris Mills [:cmills]]

Fixed, note added to

https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/autoIncrement
https://developer.mozilla.org/en-US/Firefox/Releases/11#IndexedDB