Bug 60208 - IndexedDB multiple calls to transaction.objectStore(name) should return the same instance
: IndexedDB multiple calls to transaction.objectStore(name) should return the s...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-05-04 13:54 PST by
Modified: 2011-12-19 16:57 PST (History)


Attachments
test case (2.68 KB, text/html)
2011-05-04 13:55 PST, Mark Pilgrim (Google)
no flags Details
Patch (19.20 KB, patch)
2011-12-06 12:29 PST, Joshua Bell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (20.15 KB, patch)
2011-12-06 15:36 PST, Joshua Bell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.78 KB, patch)
2011-12-16 16:03 PST, Joshua Bell
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.78 KB, patch)
2011-12-19 13:06 PST, Joshua Bell
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-05-04 13:54:58 PST
Original test: http://mxr.mozilla.org/mozilla2.0/source/dom/indexedDB/test/test_object_identity.html?force=1

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBTransactionSync-objectStore states "Every call to this function on the same IDBTransactionSync instance and with the same name returns the same IDBObjectStoreSync instance." Thus, if we have this code:

objectStore1 = a_valid_transaction.objectStore('foo');
objectStore2 = a_valid_transaction.objectStore('foo');

I would expect objectStore1 === objectStore2, but it doesn't. (In Mozilla it does.)
------- Comment #1 From 2011-05-04 13:55:21 PST -------
Created an attachment (id=92314) [details]
test case
------- Comment #2 From 2011-12-06 12:29:12 PST -------
Created an attachment (id=118091) [details]
Patch
------- Comment #3 From 2011-12-06 12:33:34 PST -------
Not ready for formal review, just wanted David and Hans to take a look:

Re: object identity
* I put in a name->object map in IDBTransaction for name->IDBObjectStore, and a similar map in IDBObjectStore for name->IDBIndex. This bookkeeping works, but... seems a little hokey. Better approaches? Maybe the backend (which would be a proxy in the multiprocess case) should hold a pointer to the frontend object?

Re: refcounting / object ownership - does this seem right?:
* IDBTransactions "own" IDBObjectStores, so I changed IDBObjectStore to have a raw pointer to the owning IDBTransaction to avoid reference cycles
* Similarly, IDBObjectStores "own" IDBIndexes, so I changed IDBIndex to have a raw pointer to the owning IDBObjectStore (and IDBTransaction) to avoid reference cycles
------- Comment #4 From 2011-12-06 15:36:48 PST -------
Created an attachment (id=118125) [details]
Patch
------- Comment #5 From 2011-12-06 15:37:48 PST -------
(From update of attachment 118125 [details])
Fixed tutorial.html layout test, which was asserting the exact opposite behavior of this fix.

A browser test is still failing, not sure why yet.
------- Comment #6 From 2011-12-06 16:14:38 PST -------
(In reply to comment #5)
> A browser test is still failing, not sure why yet.

Looks like it was a random flake on my machine. Disturbing, but no repro.
------- Comment #7 From 2011-12-07 11:27:18 PST -------
(From update of attachment 118125 [details])
In hindsight, this does avoid round-trips to the back-end when accessing stores/indexes a second time, so caching on the front-end object is probably the way to go.

So, review away!
------- Comment #8 From 2011-12-08 05:58:55 PST -------
(From update of attachment 118125 [details])
The changes to cache the objects seems fine.

I'm worried about removing the refcounting, though...

If IDBIndex only has a raw pointer to IDBObjectStore, does that mean that if the JS holds on to a IDBIndex object long enough, the rest of the objects, including the object store could become garbage collected, and IDBIndex now has a pointer to freed memory?
------- Comment #9 From 2011-12-08 16:34:51 PST -------
Yeah... after further thought, removing the refcounting is wrong. 

I played with having the "child" objects register/unregister from the "parent" manually in ctor/dtor. That would work, except that - names are not unique over the lifetime of the objects given VERSION_CHANGE transactions.

So... I'm going try hanging front-end pointers off the back end (proxy) objects. :P
------- Comment #10 From 2011-12-16 11:18:42 PST -------
I made a proposal to public-webapps to drop the requirement that "parent" objects can give you back "child" objects after the enclosing transaction is finished. This would allow us to clear out the downward-pointing references when the transaction finishes, breaking the cycles.
------- Comment #11 From 2011-12-16 16:03:29 PST -------
Created an attachment (id=119692) [details]
Patch
------- Comment #12 From 2011-12-19 07:16:51 PST -------
(From update of attachment 119692 [details])
Looks good to me.

View in context: https://bugs.webkit.org/attachment.cgi?id=119692&action=review

> Source/WebCore/storage/IDBObjectStore.cpp:223
> +    // Break reference cycles

missing period

> Source/WebCore/storage/IDBTransaction.cpp:192
> +    // Break reference cycles

period.
------- Comment #13 From 2011-12-19 13:06:10 PST -------
Created an attachment (id=119907) [details]
Patch
------- Comment #14 From 2011-12-19 13:26:52 PST -------
(From update of attachment 119907 [details])
tony@ - thanks! cq?

(I still need to send in my paperwork)
------- Comment #15 From 2011-12-19 16:57:23 PST -------
(From update of attachment 119907 [details])
Clearing flags on attachment: 119907

Committed r103283: <http://trac.webkit.org/changeset/103283>
------- Comment #16 From 2011-12-19 16:57:28 PST -------
All reviewed patches have been landed.  Closing bug.