Bug 154110 – Modern IDB: Ref cycle between IDBObjectStore and IDBIndex

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

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.

[Brady Eidson]

This patch is going to be interesting.

[Brady Eidson]

Created attachment 271111 [details]
Patch v1

[Darin Adler]

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?

[Brady Eidson]

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

[Brady Eidson]

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)

[Brady Eidson]

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.

[WebKit Commit Bot]

Comment on attachment 271135 [details]
Patch v2

Clearing flags on attachment: 271135

Committed r196482: <http://trac.webkit.org/changeset/196482>

[Geoffrey Garen]

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?

[Alex Christensen]

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.

[Brady Eidson]

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