Last Comment Bug 699468 - IndexedDB: Rework DatabaseInfo handling
: IndexedDB: Rework DatabaseInfo handling
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
Depends on:
Blocks: 625071 692865 693001
  Show dependency treegraph
 
Reported: 2011-11-03 10:45 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2012-03-22 11:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Move ObjectStoreInfoHash into DatabaseInfo (7.43 KB, patch)
2011-11-03 10:48 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually (6.38 KB, patch)
2011-11-03 10:49 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Part 3: Always go through the Database to get the DatabaseInfo (8.55 KB, patch)
2011-11-03 10:50 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review
Part 4: Always go through the DatabaseInfo to get the ObjectStoreInfo (15.44 KB, patch)
2011-11-03 10:51 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review
Part 5: Clone the canonical DatabaseInfo when a database is closed. (8.93 KB, patch)
2011-11-03 10:52 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Part 1: Move ObjectStoreInfoHash into DatabaseInfo (7.49 KB, patch)
2011-11-06 15:25 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
khuey: review+
Details | Diff | Splinter Review
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually (5.23 KB, patch)
2011-11-06 15:27 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review
Part 4: Always go through the DatabaseInfo to get the ObjectStoreInfo (15.49 KB, patch)
2011-11-06 15:29 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
khuey: review+
Details | Diff | Splinter Review
Part 5: Clone the canonical DatabaseInfo when a database is closed. (18.39 KB, patch)
2011-11-06 15:30 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review
Part 6: Make some DatabaseInfo stuff private. (1001 bytes, patch)
2011-11-06 15:30 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bent.mozilla: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 10:45:48 PDT

    
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 10:48:04 PDT
Created attachment 571704 [details] [diff] [review]
Part 1: Move ObjectStoreInfoHash into DatabaseInfo
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 10:49:06 PDT
Created attachment 571705 [details] [diff] [review]
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 10:50:15 PDT
Created attachment 571706 [details] [diff] [review]
Part 3: Always go through the Database to get the DatabaseInfo
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 10:51:35 PDT
Created attachment 571707 [details] [diff] [review]
Part 4: Always go through the DatabaseInfo to get the ObjectStoreInfo
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 10:52:42 PDT
Created attachment 571708 [details] [diff] [review]
Part 5: Clone the canonical DatabaseInfo when a database is closed.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-03 13:03:10 PDT
Comment on attachment 571704 [details] [diff] [review]
Part 1: Move ObjectStoreInfoHash into DatabaseInfo

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

::: dom/indexedDB/DatabaseInfo.cpp
@@ +125,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(aId, "Bad id!");
>  
>    if (gDatabaseHash) {
> +    if (gDatabaseHash->Get(aId, aInfo)) {

Nit: Do &&

@@ +233,5 @@
>  
>    if (gDatabaseHash) {
> +    DatabaseInfo* info;
> +    if (gDatabaseHash->Get(aDatabaseId, &info)) {
> +      if (info->objectStoreHash) {

Nit: && again.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-03 13:12:29 PDT
Comment on attachment 571705 [details] [diff] [review]
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually

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

::: dom/indexedDB/DatabaseInfo.cpp
@@ +70,5 @@
>  : nextObjectStoreId(1),
>    nextIndexId(1),
>    runningVersionChange(false)
>  {
>    MOZ_COUNT_CTOR(DatabaseInfo);

Nix these now that you use refcount logging instead.

@@ +111,2 @@
>  {
>    MOZ_COUNT_CTOR(IndexUpdateInfo);

Nix.

::: dom/indexedDB/DatabaseInfo.h
@@ -63,5 @@
> -#else
> -  DatabaseInfo()
> -  : nextObjectStoreId(1), nextIndexId(1), runningVersionChange(false)
> -  { }
> -#endif

These can be inlined now.

@@ +79,5 @@
>    bool runningVersionChange;
>  
>    nsAutoPtr<ObjectStoreInfoHash> objectStoreHash;
>  
> +  NS_INLINE_DECL_REFCOUNTING(DatabaseInfo)

So if we're going to do this then we should change the hash table getters to AddRef too, then use refptrs everywhere. No more mixed manual+auto.

@@ -96,5 @@
>    ~IndexInfo();
> -#else
> -  IndexInfo()
> -  : id(LL_MININT), unique(false), autoIncrement(false) { }
> -#endif

Leave inlined.

@@ -113,5 @@
>    ~ObjectStoreInfo();
> -#else
> -  ObjectStoreInfo()
> -  : id(0), autoIncrement(false), databaseId(0) { }
> -#endif

Leave inlined.

@@ -139,3 @@
>    IndexUpdateInfo();
>    ~IndexUpdateInfo();
> -#endif

Leave inlined.

::: dom/indexedDB/IDBDatabase.cpp
@@ +297,5 @@
>      if (!DatabaseInfo::Get(mDatabaseId, &info)) {
>        NS_ERROR("This should never fail!");
>      }
>  
> +    NS_RELEASE(info);

Boo. Use a refptr.

@@ +390,5 @@
>      if (!DatabaseInfo::Get(mDatabaseId, &info)) {
>        NS_ERROR("This should never fail!");
>      }
>  
> +    NS_RELEASE(info);

And here

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +998,5 @@
>  OpenDatabaseHelper::EnsureSuccessResult()
>  {
>    DatabaseInfo* dbInfo;
>    if (DatabaseInfo::Get(mDatabaseId, &dbInfo)) {
> +    NS_ADDREF(dbInfo);

And here (with getter_AddRefs)
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-03 13:15:39 PDT
Comment on attachment 571706 [details] [diff] [review]
Part 3: Always go through the Database to get the DatabaseInfo

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

Can we make DatabaseInfo::Get/Remove private and make IDBDatabase a friend class? Then we could be sure no one would do the wrong thing in the future.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-03 13:22:36 PDT
Comment on attachment 571707 [details] [diff] [review]
Part 4: Always go through the DatabaseInfo to get the ObjectStoreInfo

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

::: dom/indexedDB/IDBDatabase.cpp
@@ +158,4 @@
>    }
>  
>  private:
> +  nsRefPtr<IDBDatabase> mDatabase;

This is a stack class (AutoRemoveObjectStore)... shouldn't need to addref

@@ +663,5 @@
>      return NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR;
>    }
>  
>    ObjectStoreInfo* objectStoreInfo;
> +  if (!Info()->GetObjectStore(aName, &objectStoreInfo)) {

You use this twice, copy to stack var unless you think it can change (it shouldn't here).

::: dom/indexedDB/IDBObjectStore.cpp
@@ +392,4 @@
>    }
>  
>  private:
> +  nsRefPtr<IDBDatabase> mDatabase;

Same here, no need to addref (AutoRemoveIndex)
Comment 10 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-03 13:30:04 PDT
Comment on attachment 571708 [details] [diff] [review]
Part 5: Clone the canonical DatabaseInfo when a database is closed.

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

::: dom/indexedDB/DatabaseInfo.cpp
@@ +73,5 @@
> +
> +  nsAutoPtr<ObjectStoreInfo> newInfo(new ObjectStoreInfo(*aData));
> +
> +  if (!hash->Put(aKey, newInfo)) {
> +    NS_ERROR("Out of memory?");

This can fail in the real world, just warn.

@@ +79,5 @@
> +  }
> +
> +  newInfo.forget();
> +
> +  return PL_DHASH_NEXT;

Nit: Nix extra newline there.

@@ +98,5 @@
>  {
> +  // Clones are never in the hash.
> +  if (!cloned) {
> +    DatabaseInfo::Remove(id);
> +  }

I kinda don't like this... There should only be one time when the databaseInfo should be removed from the hash table, and it shouldnt have anything to do with a refcount. Imagine if someone leaks one of these some day, then the databaseinfo will stay around too long.

@@ +226,3 @@
>  
> +  return objectStoreHash &&
> +         objectStoreHash->Get(aName, nsnull);

Nit: Will this fit on one line?

@@ +300,5 @@
> +  // IndexedDatabaseManager scheduling still applies.
> +  if (runningVersionChange) {
> +    runningVersionChange = false;
> +    dbInfo->runningVersionChange = true;
> +  }

Hm, I don't like this happening in here. Seems like the caller should do this manually. Clone should just Clone imo.
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-04 07:30:14 PDT
(In reply to ben turner [:bent] from comment #10)
> @@ +98,5 @@
> >  {
> > +  // Clones are never in the hash.
> > +  if (!cloned) {
> > +    DatabaseInfo::Remove(id);
> > +  }
> 
> I kinda don't like this... There should only be one time when the
> databaseInfo should be removed from the hash table, and it shouldnt have
> anything to do with a refcount. Imagine if someone leaks one of these some
> day, then the databaseinfo will stay around too long.

This is exactly how the current setup works ...

> @@ +300,5 @@
> > +  // IndexedDatabaseManager scheduling still applies.
> > +  if (runningVersionChange) {
> > +    runningVersionChange = false;
> > +    dbInfo->runningVersionChange = true;
> > +  }
> 
> Hm, I don't like this happening in here. Seems like the caller should do
> this manually. Clone should just Clone imo.

Ok.  Really we should move the flag off of the DatabaseInfo and onto the IDBDatabase, but that can be done in a follow up.
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-04 08:08:08 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> This is exactly how the current setup works ...

Yes, but I was thinking that now that calling close() clones the databaseinfo we don't have to do that any more. Maybe we should just ignore that for now.

> Ok.  Really we should move the flag off of the DatabaseInfo and onto the
> IDBDatabase, but that can be done in a follow up.

Let's go ahead and fix it while we're here.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-06 15:25:53 PST
Created attachment 572350 [details] [diff] [review]
Part 1: Move ObjectStoreInfoHash into DatabaseInfo

Carrying forward r+.
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-06 15:27:03 PST
Created attachment 572351 [details] [diff] [review]
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-06 15:29:01 PST
Created attachment 572352 [details] [diff] [review]
Part 4: Always go through the DatabaseInfo to get the ObjectStoreInfo

Carrying forward review.
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-06 15:30:20 PST
Created attachment 572353 [details] [diff] [review]
Part 5: Clone the canonical DatabaseInfo when a database is closed.
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-06 15:30:58 PST
Created attachment 572354 [details] [diff] [review]
Part 6: Make some DatabaseInfo stuff private.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-07 14:54:58 PST
Comment on attachment 572351 [details] [diff] [review]
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually

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

::: dom/indexedDB/DatabaseInfo.cpp
@@ +70,2 @@
>  {
> +  DatabaseInfo::Remove(id);

This can still be inlined, right?

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +998,5 @@
>  OpenDatabaseHelper::EnsureSuccessResult()
>  {
>    DatabaseInfo* dbInfo;
>    if (DatabaseInfo::Get(mDatabaseId, &dbInfo)) {
> +    NS_ADDREF(dbInfo);

I thought we were going to fix this hash table to addref on the way out?
Comment 19 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-07 14:59:48 PST
Comment on attachment 572353 [details] [diff] [review]
Part 5: Clone the canonical DatabaseInfo when a database is closed.

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

::: dom/indexedDB/DatabaseInfo.cpp
@@ +161,5 @@
>    NS_ASSERTION(aId, "Bad id!");
>  
>    if (gDatabaseHash &&
>        gDatabaseHash->Get(aId, aInfo)) {
> +    NS_IF_ADDREF(*aInfo);

Ugh, again! Let's make this hash addref on the way out, and use getter_AddRefs
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-07 15:00:49 PST
Comment on attachment 572354 [details] [diff] [review]
Part 6: Make some DatabaseInfo stuff private.

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

::: dom/indexedDB/DatabaseInfo.h
@@ +62,5 @@
>  struct DatabaseInfo
>  {
> +  friend class IDBDatabase;
> +  friend class OpenDatabaseHelper;
> +private:

Nit: Add an extra line before private.
Comment 21 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-07 15:15:57 PST
Comment on attachment 572351 [details] [diff] [review]
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually

Ok, khuey convinced me. We'll leave as is for now.

Note You need to log in before you can comment on or make changes to this bug.