Bug 154110 - Modern IDB: Ref cycle between IDBObjectStore and IDBIndex
Summary: Modern IDB: Ref cycle between IDBObjectStore and IDBIndex
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117 154015
  Show dependency treegraph
 
Reported: 2016-02-11 09:25 PST by Brady Eidson
Modified: 2016-02-12 12:54 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (31.51 KB, patch)
2016-02-11 17:24 PST, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Patch v2 (31.85 KB, patch)
2016-02-11 21:58 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-02-11 09:25:37 PST
Modern IDB: Ref cycle between IDBObjectStore and IDBIndex

If one is reachable, the other has to remain reachable.

Currently they do this by Ref'ing each other.

Let's make that work another way.
Comment 1 Brady Eidson 2016-02-11 10:40:09 PST
This patch is going to be interesting.
Comment 2 Brady Eidson 2016-02-11 17:24:05 PST
Created attachment 271111 [details]
Patch v1
Comment 3 Darin Adler 2016-02-11 17:51:29 PST
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?
Comment 4 Brady Eidson 2016-02-11 21:47:03 PST
(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.
Comment 5 Brady Eidson 2016-02-11 21:58:37 PST
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 6 Brady Eidson 2016-02-12 09:40:11 PST
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 7 WebKit Commit Bot 2016-02-12 10:30:45 PST
Comment on attachment 271135 [details]
Patch v2

Clearing flags on attachment: 271135

Committed r196482: <http://trac.webkit.org/changeset/196482>
Comment 8 Geoffrey Garen 2016-02-12 10:43:50 PST
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 9 Alex Christensen 2016-02-12 11:05:58 PST
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.
Comment 10 Brady Eidson 2016-02-12 12:51:05 PST
(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.