Summary: | IndexedDB multiple calls to transaction.objectStore(name) should return the same instance | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Pilgrim (Google) <pilgrim> | ||||||||||||
Component: | New Bugs | Assignee: | 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
Mark Pilgrim (Google)
2011-05-04 13:54:58 PDT
Created attachment 92314 [details]
test case
Created attachment 118091 [details]
Patch
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 Created attachment 118125 [details]
Patch
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.
(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 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 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?
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 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. Created attachment 119692 [details]
Patch
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. Created attachment 119907 [details]
Patch
Comment on attachment 119907 [details]
Patch
tony@ - thanks! cq?
(I still need to send in my paperwork)
Comment on attachment 119907 [details] Patch Clearing flags on attachment: 119907 Committed r103283: <http://trac.webkit.org/changeset/103283> All reviewed patches have been landed. Closing bug. |