IndexedDB: Rework DatabaseInfo handling

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 4 obsolete attachments)

8.55 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
7.49 KB, patch
khuey
: review+
Details | Diff | Splinter Review
5.23 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
15.49 KB, patch
khuey
: review+
Details | Diff | Splinter Review
18.39 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
1001 bytes, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
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.
Attachment #571704 - Flags: review?(bent.mozilla) → review+
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 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.
Attachment #571706 - Flags: review?(bent.mozilla) → review+
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)
Attachment #571707 - Flags: review?(bent.mozilla) → review+
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.
(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.
(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 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 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
Attachment #572353 - Flags: review?(bent.mozilla) → review+
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.
Attachment #572354 - Flags: review?(bent.mozilla) → review+
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.
Attachment #572351 - Flags: review?(bent.mozilla) → review+
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla10 → ---
You need to log in before you can comment on or make changes to this bug.