Bug 60208

Summary: IndexedDB multiple calls to transaction.objectStore(name) should return the same instance
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: dgrogan, fishd, hans, jsbell, pilgrim, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test case
none
Patch
none
Patch
none
Patch
none
Patch none

Description Mark Pilgrim (Google) 2011-05-04 13:54:58 PDT
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 Mark Pilgrim (Google) 2011-05-04 13:55:21 PDT
Created attachment 92314 [details]
test case
Comment 2 Joshua Bell 2011-12-06 12:29:12 PST
Created attachment 118091 [details]
Patch
Comment 3 Joshua Bell 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 Joshua Bell 2011-12-06 15:36:48 PST
Created attachment 118125 [details]
Patch
Comment 5 Joshua Bell 2011-12-06 15:37:48 PST
Comment on attachment 118125 [details]
Patch

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 Joshua Bell 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 Joshua Bell 2011-12-07 11:27:18 PST
Comment on attachment 118125 [details]
Patch

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 Hans Wennborg 2011-12-08 05:58:55 PST
Comment on attachment 118125 [details]
Patch

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 Joshua Bell 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 Joshua Bell 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 Joshua Bell 2011-12-16 16:03:29 PST
Created attachment 119692 [details]
Patch
Comment 12 Hans Wennborg 2011-12-19 07:16:51 PST
Comment on attachment 119692 [details]
Patch

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 Joshua Bell 2011-12-19 13:06:10 PST
Created attachment 119907 [details]
Patch
Comment 14 Joshua Bell 2011-12-19 13:26:52 PST
Comment on attachment 119907 [details]
Patch

tony@ - thanks! cq?

(I still need to send in my paperwork)
Comment 15 WebKit Review Bot 2011-12-19 16:57:23 PST
Comment on attachment 119907 [details]
Patch

Clearing flags on attachment: 119907

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