Bug 152966 – Modern IDB: Make MemoryIndex and MemoryObjectStore RefCounted

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modern IDB: Make MemoryIndex and MemoryObjectStore RefCounted

Some of the remaining layout tests crash/asserts are due to very subtle mismanagement of unique_ptrs vs raw pointers. Fixing them reliably will be difficult and fragile unless we make these objects RefCounted.

[Brady Eidson]

Created attachment 268665 [details]
Patch v1

[Alex Christensen]

Comment on attachment 268665 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=268665&action=review

> Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:224
>      for (auto& index : m_deletedIndexes.values()) {
>          MemoryObjectStore& objectStore = index->objectStore();
> -        objectStore.maybeRestoreDeletedIndex(WTFMove(index));
> +        objectStore.maybeRestoreDeletedIndex(*index);
>      }

I think we should null-check index.  There are values in m_deletedIndexes that are nullptr.  All other pointer dereferences in this patch are null checked.
Also, probably no need for a local variable for objectStore.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 268665 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268665&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:224
> >      for (auto& index : m_deletedIndexes.values()) {
> >          MemoryObjectStore& objectStore = index->objectStore();
> > -        objectStore.maybeRestoreDeletedIndex(WTFMove(index));
> > +        objectStore.maybeRestoreDeletedIndex(*index);
> >      }
> 
> I think we should null-check index.  There are values in m_deletedIndexes
> that are nullptr.  

I don't think you're right on this.

The only place a value is added to m_deletedIndexes is in MemoryBackingStoreTransaction::indexDeleted which takes a Ref<> as its argument.

i.e. - The index being added to m_deletedIndexes is non-null

It would definitely be a bug anytime we put a null value in any of these sets/maps.

> All other pointer dereferences in this patch are null checked.

I actually see very few (just 1?) null checks in this patch. And we definitely don't null check the values in any of the sets/maps of RefPtrs.

> Also, probably no need for a local variable for objectStore.

Indeed!

[Brady Eidson]

Fixing the local variable, but I'm going to land without the null check.

If we dig deeper and find a reason it needs to be there, I will followup promptly.

[Brady Eidson]

http://trac.webkit.org/changeset/194857

[Alex Christensen]

Comment on attachment 268665 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=268665&action=review

> Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:93
>      auto addResult = m_deletedIndexes.add(index->info().name(), nullptr);

Right here.  We add an entry in m_deletedIndexes with a value of nullptr.  Please add the null check.

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:152
> +    transaction.indexDeleted(*index);

I was talking about pointer dereferences like this one.  Earlier in this function (although not in this patch) we do check if index is null before dereferencing it.  This is safe.

[Brady Eidson]

(In reply to comment #6)
> Comment on attachment 268665 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268665&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp:93
> >      auto addResult = m_deletedIndexes.add(index->info().name(), nullptr);
> 
> Right here.  We add an entry in m_deletedIndexes with a value of nullptr. 
> Please add the null check.

As I mentioned on IRC, line 93 is meaningless without the context of lines 94 and 95. In fact, the whole method as checked in:

void MemoryBackingStoreTransaction::indexDeleted(Ref<MemoryIndex>&& index)
{
    m_indexes.remove(&index.get());

    auto addResult = m_deletedIndexes.add(index->info().name(), nullptr);
    if (addResult.isNewEntry)
        addResult.iterator->value = WTFMove(index);
}

The index passed in to the function is a Ref - it's never null.

Lines 93/94/95 are the typical "only set this value in the map if it's the first value" pattern.

After ::indexDeleted returns, the value in the map cannot ever be null.


> 
> > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:152
> > +    transaction.indexDeleted(*index);
> 
> I was talking about pointer dereferences like this one.  Earlier in this
> function (although not in this patch) we do check if index is null before
> dereferencing it.  This is safe.

That's because we get index earlier in the function like this:
    auto index = takeIndexByName(indexName);
... and not by directly looking it up in a map.