Summary: | Modern IDB: Ref cycle between IDBObjectStore and IDBIndex | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||
Component: | WebCore Misc. | Assignee: | Brady Eidson <beidson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, commit-queue, ggaren | ||||||
Priority: | P2 | ||||||||
Version: | Safari 9 | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 149117, 154015 | ||||||||
Attachments: |
|
Description
Brady Eidson
2016-02-11 09:25:37 PST
This patch is going to be interesting. Created attachment 271111 [details]
Patch v1
Comment on attachment 271111 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=271111&action=review > Source/WebCore/Modules/indexeddb/IDBIndex.h:82 > + // We use our own ref/deref methods because Legacy IDB and Modern IDB have very different Nitpick: These are functions, not methods. > Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp:114 > + Ref<IDBRequest> request = m_objectStore.modernTransaction().requestOpenCursor(*context, *this, info); > return WTFMove(request); Just return it on the same line where we create it, and no need for WTFMove then. > Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp:220 > + Ref<IDBRequest> request = m_objectStore.modernTransaction().requestOpenCursor(*context, *this, info); > return WTFMove(request); Ditto. > Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp:362 > + // And then it must undo all of the refs it had already given its IDBObjectStore when the lifetimes were intertwined. > + for (unsigned i = 0; i < m_refCount; ++i) > + m_objectStore.deref(); This kind of thing is so horrible. I know we have it elsewhere. But it’s actually a loop to implement subtraction! Please tell me its temporary. > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:528 > + RefPtr<IDBIndex> refIndex = index.get(); Why not use Ref instead of RefPtr? > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:531 > - return WTFMove(index); > + return refIndex; Why the change? (In reply to comment #3) > Comment on attachment 271111 [details] All comments I don't address in this reply have been addressed in the path. > > Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp:362 > > + // And then it must undo all of the refs it had already given its IDBObjectStore when the lifetimes were intertwined. > > + for (unsigned i = 0; i < m_refCount; ++i) > > + m_objectStore.deref(); > > This kind of thing is so horrible. I know we have it elsewhere. But it’s > actually a loop to implement subtraction! Please tell me its temporary. This is not temporary. This is required as part of this very custom lifecycle management. Is your complaint that the loop counts from 0 to N-1, but is derefing? And resolving that could be looping from N-1 to 0, derefing? Or is your complaint that this is just gross and weird? I'll change for the former right now. I don't know of a way to fix the latter! > > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:528 > > + RefPtr<IDBIndex> refIndex = index.get(); > > Why not use Ref instead of RefPtr? > > > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:531 > > - return WTFMove(index); > > + return refIndex; > > Why the change? These two taken together: Previously, the IDBIndex created *was* a Ref<>. But the method returns a RefPtr<>. Hence, the WTFMove was how we converted the Ref to a RefPtr, which cannot happen automatically. With the new model, it just made sense to keep the local smart pointer as a RefPtr - instead of a Ref - since that was the return value for the method and would not require a WTFMove. Created attachment 271135 [details]
Patch v2
Even though Darin already r+'ed this, I'm going to hold off landing it until at least one other set of eyes takes a look (I've emailed them)
Comment on attachment 271135 [details]
Patch v2
This not being in the tree is holding up my progress fixing additional leaks/ref cycles.
So on second thought, I will cq+ this, but just make sure it gets looked at retroactively.
Comment on attachment 271135 [details] Patch v2 Clearing flags on attachment: 271135 Committed r196482: <http://trac.webkit.org/changeset/196482> I think you can do better, depending on the expected behavior. Is it possible to create an IDBIndex without first creating an IDBObjectStore? After creating an IDBObjectStore and an IDBIndex, and calling IDBObjectStore.deleteIndex(): Is it still possible to reference the IDBIndex from JavaScript (or elsewhere)? Is it still possible to retrieve the IDBObjectStore from the IDBIndex? Is it possible to create a new IDBIndex for the same IDBObjectStore? Comment on attachment 271135 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=271135&action=review > Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp:360 > + // It must undo all of the refs it had previously given its IDBObjectStore when the lifetimes were intertwined. > + for (unsigned i = m_refCount; i > 0; --i) > + m_objectStore.deref(); I think Darin's complaint is that this does m_refCount operations to achieve what could be done with one operation if we had an m_objectStore.derefThisManyTimesGuaranteedToLeaveAtLeastOneRef(m_refCount). We could also assert that m_objectStore has at least that many refs. (In reply to comment #9) > Comment on attachment 271135 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271135&action=review > > > Source/WebCore/Modules/indexeddb/client/IDBIndexImpl.cpp:360 > > + // It must undo all of the refs it had previously given its IDBObjectStore when the lifetimes were intertwined. > > + for (unsigned i = m_refCount; i > 0; --i) > > + m_objectStore.deref(); > > I think Darin's complaint is that this does m_refCount operations to achieve > what could be done with one operation if we had an > m_objectStore.derefThisManyTimesGuaranteedToLeaveAtLeastOneRef(m_refCount). > We could also assert that m_objectStore has at least that many refs. Based on our email conversation, I think the direction we're headed is simplifying this by not allowing the detach behavior, at the expense of having to hang on to all IDBIndexes until the IDBObjectStore goes away. Which is probably an okay tradeoff. |