The default bug view has changed. See this FAQ.

IndexedDB: Rework DatabaseInfo handling

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 4 obsolete attachments)

8.55 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
7.49 KB, patch
khuey
: review+
Details | Diff | Splinter Review
5.23 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
15.49 KB, patch
khuey
: review+
Details | Diff | Splinter Review
18.39 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
1001 bytes, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 571704 [details] [diff] [review]
Part 1: Move ObjectStoreInfoHash into DatabaseInfo
Attachment #571704 - Flags: review?(bent.mozilla)
Created attachment 571705 [details] [diff] [review]
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually
Attachment #571705 - Flags: review?(bent.mozilla)
Created attachment 571706 [details] [diff] [review]
Part 3: Always go through the Database to get the DatabaseInfo
Attachment #571706 - Flags: review?(bent.mozilla)
Created attachment 571707 [details] [diff] [review]
Part 4: Always go through the DatabaseInfo to get the ObjectStoreInfo
Attachment #571707 - Flags: review?(bent.mozilla)
Created attachment 571708 [details] [diff] [review]
Part 5: Clone the canonical DatabaseInfo when a database is closed.
Attachment #571708 - Flags: review?(bent.mozilla)
Blocks: 625071, 693001, 692865
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.
Created attachment 572350 [details] [diff] [review]
Part 1: Move ObjectStoreInfoHash into DatabaseInfo

Carrying forward r+.
Attachment #571704 - Attachment is obsolete: true
Attachment #572350 - Flags: review+
Created attachment 572351 [details] [diff] [review]
Part 2: Give DatabaseInfo an AddRef/Release instead of twiddling the refcount manually
Attachment #571705 - Attachment is obsolete: true
Attachment #571705 - Flags: review?(bent.mozilla)
Attachment #572351 - Flags: review?(bent.mozilla)
Created attachment 572352 [details] [diff] [review]
Part 4: Always go through the DatabaseInfo to get the ObjectStoreInfo

Carrying forward review.
Attachment #571707 - Attachment is obsolete: true
Attachment #572352 - Flags: review+
Created attachment 572353 [details] [diff] [review]
Part 5: Clone the canonical DatabaseInfo when a database is closed.
Attachment #571708 - Attachment is obsolete: true
Attachment #571708 - Flags: review?(bent.mozilla)
Attachment #572353 - Flags: review?(bent.mozilla)
Created attachment 572354 [details] [diff] [review]
Part 6: Make some DatabaseInfo stuff private.
Attachment #572354 - Flags: review?(bent.mozilla)
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+
https://hg.mozilla.org/mozilla-central/rev/0d63a5e24889
https://hg.mozilla.org/mozilla-central/rev/be1907a18f5b
https://hg.mozilla.org/mozilla-central/rev/0b28e3f60544
https://hg.mozilla.org/mozilla-central/rev/b978e3a3f467
https://hg.mozilla.org/mozilla-central/rev/4a0214e37148
https://hg.mozilla.org/mozilla-central/rev/6d4788dab957
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla10 → ---
You need to log in before you can comment on or make changes to this bug.