Crash in com.apple.WebCore: WebCore::IDBTransaction::pendingOperationTimerFired + 72

[Sihui Liu]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
=> if (!m_transactionOperationsInProgressQueue.isEmpty() && !m_transactionOperationsInProgressQueue.last()->nextRequestCanGoToServer())
0   com.apple.WebCore             	0x000000010b642788 WebCore::IDBTransaction::pendingOperationTimerFired() + 72
1   com.apple.WebCore             	0x000000010ab99bf8 WebCore::ThreadTimers::sharedTimerFiredInternal() + 168
2   com.apple.WebCore             	0x000000010ab99b3f WebCore::timerFired(__CFRunLoopTimer*, void*) + 31
3   com.apple.CoreFoundation      	0x7fff31d65fa5 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1638.11/CoreFoundation/RunLoop.subproj/CFRunLoop.c:1772)
4   com.apple.CoreFoundation      	0x7fff31d65b51 __CFRunLoopDoTimer + 864 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1638.11/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2357)
5   com.apple.CoreFoundation      	0x7fff31d6553e __CFRunLoopDoTimers + 320 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1638.11/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2512)

[Sihui Liu]

<rdar://problem/48461116>

[Sihui Liu]

Created attachment 363346 [details]
Patch

[Ryan Haddad]

Looks like this passed EWS. Ping for reviewers.

[Geoffrey Garen]

Comment on attachment 363346 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:105
> +        if (m_completeFunction) {
> +            m_completeFunction(data);
> +            // m_completeFunction might be holding the last ref to this TransactionOperation,
> +            // so we need to do this trick to null it out without first destroying it.
> +            Function<void(const IDBResultData&)> oldCompleteFunction;
> +            std::swap(m_completeFunction, oldCompleteFunction);
> +        }
>          m_transaction->operationCompletedOnClient(*this);

It seems like a bug that you reference m_transaction after this may have been deleted (by destroying m_completeFunction, which holds the last ref to this TransactionOperation.)

Probably the best solution is to use "auto protect = makeRef(this)" at the top of the function.

[Sihui Liu]

Comment on attachment 363346 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:105
>>          m_transaction->operationCompletedOnClient(*this);
> 
> It seems like a bug that you reference m_transaction after this may have been deleted (by destroying m_completeFunction, which holds the last ref to this TransactionOperation.)
> 
> Probably the best solution is to use "auto protect = makeRef(this)" at the top of the function.

In an expected case, operationCompletedOnClient should clear the last ref of a TransactionOperation by removing it from m_transactionOperationMap.

I think "the last ref" means if we don't clear the completeFunction here, completeFunction can hold the last ref of TransactionOperation after operationCompletedOnClient.

[Geoffrey Garen]

Comment on attachment 363346 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:105
>>>          m_transaction->operationCompletedOnClient(*this);
>> 
>> It seems like a bug that you reference m_transaction after this may have been deleted (by destroying m_completeFunction, which holds the last ref to this TransactionOperation.)
>> 
>> Probably the best solution is to use "auto protect = makeRef(this)" at the top of the function.
> 
> In an expected case, operationCompletedOnClient should clear the last ref of a TransactionOperation by removing it from m_transactionOperationMap.
> 
> I think "the last ref" means if we don't clear the completeFunction here, completeFunction can hold the last ref of TransactionOperation after operationCompletedOnClient.

Previously, this function deallocated m_completeFunction *after* loading and using this->m_transaction. This patch makes a change to deallocate m_completeFunction *before* loading and using this->m_transaction. If deallocating m_completeFunction might delete 'this', then this patch introduces a use of this->m_transaction after free.

[Sihui Liu]

Created attachment 363714 [details]
Patch

[Geoffrey Garen]

Comment on attachment 363714 [details]
Patch

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

r=me

> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:144
> +    bool m_doneComplete { false };

We usually use 'did' instead of 'done'. So, I would name this 'm_didComplete'.

[Sihui Liu]

Created attachment 363912 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 363912 [details]:

legacy-animation-engine/fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html bug 195431 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 363912 [details]
Patch for landing

Clearing flags on attachment: 363912

Committed r242608: <https://trac.webkit.org/changeset/242608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.