Bug 60208 – IndexedDB multiple calls to transaction.objectStore(name) should return the same instance

• Component: New Bugs
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

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.)

[Mark Pilgrim (Google)]

Created attachment 92314 [details]
test case

[Joshua Bell]

Created attachment 118091 [details]
Patch

[Joshua Bell]

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

[Joshua Bell]

Created attachment 118125 [details]
Patch

[Joshua Bell]

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.

[Joshua Bell]

(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.

[Joshua Bell]

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!

[Hans Wennborg]

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?

[Joshua Bell]

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

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 119692 [details]
Patch

[Hans Wennborg]

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.

[Joshua Bell]

Created attachment 119907 [details]
Patch

[Joshua Bell]

Comment on attachment 119907 [details]
Patch

tony@ - thanks! cq?

(I still need to send in my paperwork)

[WebKit Review Bot]

Comment on attachment 119907 [details]
Patch

Clearing flags on attachment: 119907

Committed r103283: <http://trac.webkit.org/changeset/103283>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.