Bug 195214

Summary: Crash in com.apple.WebCore: WebCore::IDBTransaction::pendingOperationTimerFired + 72
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, commit-queue, ews-watchlist, ggaren, jsbell, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Sihui Liu
Reported 2019-03-01 09:43:59 PST
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)
Attachments
Patch (4.15 KB, patch)
2019-03-01 10:48 PST, Sihui Liu 		no flags
DetailsFormatted DiffDiff
Patch (4.03 KB, patch)
2019-03-05 17:32 PST, Sihui Liu 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.05 KB, patch)
2019-03-07 12:53 PST, Sihui Liu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Sihui Liu
Comment 1 2019-03-01 09:44:24 PST
<rdar://problem/48461116>
Sihui Liu
Comment 2 2019-03-01 10:48:29 PST
Created attachment 363346 [details] Patch
Ryan Haddad
Comment 3 2019-03-05 08:43:04 PST
Looks like this passed EWS. Ping for reviewers.
Geoffrey Garen
Comment 4 2019-03-05 11:52:12 PST
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 5 2019-03-05 12:18:20 PST
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 6 2019-03-05 14:28:01 PST
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
Comment 7 2019-03-05 17:32:24 PST
Created attachment 363714 [details] Patch
Geoffrey Garen
Comment 8 2019-03-06 16:48:32 PST
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
Comment 9 2019-03-07 12:53:27 PST
Created attachment 363912 [details] Patch for landing
WebKit Commit Bot
Comment 10 2019-03-07 13:42:35 PST
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: dino@apple.com and graouts@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 11 2019-03-07 13:43:16 PST
Comment on attachment 363912 [details] Patch for landing Clearing flags on attachment: 363912 Committed r242608: <https://trac.webkit.org/changeset/242608>
WebKit Commit Bot
Comment 12 2019-03-07 13:43:18 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.