Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call: Thread 0 name: Dispatch queue: com.apple.main-thread Thread 0 Crashed ↩: 0 JavaScriptCore 0x00000001a229b470 WTF::WorkQueue::dispatch(WTF::Function<void ()>&&) + 20 (WorkQueueCocoa.cpp:36) 1 WebKit 0x000000019a6d2310 WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call() + 80 (ResourceLoadStatisticsMemoryStore.cpp:764) 2 WebKit 0x000000019a6d2310 WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call() + 80 (ResourceLoadStatisticsMemoryStore.cpp:764) 3 WebKit 0x000000019a6e04ac WebKit::WebResourceLoadStatisticsStore::callUpdatePrevalentDomainsToBlockCookiesForHandler(WTF::Vector<WebCore::RegistrableDomain, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::CompletionHandler<void ()>&&) + 76 (Function.h:79) 4 WebKit 0x000000019a6d21ac WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32, void>::call() + 100 (ResourceLoadStatisticsMemoryStore.cpp:763) 5 JavaScriptCore 0x00000001a2258690 WTF::RunLoop::performWork() + 276 (Function.h:79) 6 JavaScriptCore 0x00000001a2258950 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38) 7 CoreFoundation 0x00000001930b6f78 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24 (CFRunLoop.c:1913) 8 CoreFoundation 0x00000001930b6ed0 __CFRunLoopDoSource0 + 80 (CFRunLoop.c:1947) 9 CoreFoundation 0x00000001930b6668 __CFRunLoopDoSources0 + 180 (CFRunLoop.c:1983) 10 CoreFoundation 0x00000001930b17b4 __CFRunLoopRun + 1080 (CFRunLoop.c:2873) 11 CoreFoundation 0x00000001930b1054 CFRunLoopRunSpecific + 464 (CFRunLoop.c:3183) 12 Foundation 0x00000001933ef8c4 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 228 (NSRunLoop.m:374) 13 Foundation 0x00000001934292d4 -[NSRunLoop(NSRunLoop) run] + 88 (NSRunLoop.m:399) 14 libxpc.dylib 0x0000000192d11360 _xpc_objc_main + 304 (main.m:179) 15 libxpc.dylib 0x0000000192d13ca0 xpc_main + 148 (init.c:1568) 16 WebKit 0x000000019a749c6c WebKit::XPCServiceMain(int, char const**) + 360 (XPCServiceMain.mm:147) 17 libdyld.dylib 0x0000000192f3cc7c start + 4
<rdar://problem/53335583>
Created attachment 374753 [details] Patch
Created attachment 374754 [details] Patch
Comment on attachment 374754 [details] Patch Thank you for tracking this down, Chris. R=Me
LGTM too. View in context: https://bugs.webkit.org/attachment.cgi?id=374754&action=review > Source/WebKit/ChangeLog:27 > + m_statisticsStore / m_persistentStorage on the background queue, synchronously. The This is not new but this potentially blocks the main thread for the background thread to finish its I/O tasks. Could there be a way to unblock the main thread? > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:182 > ASSERT(RunLoop::isMain()); This assert is not needed given WebResourceLoadStatisticsStore is DestructionThread::Main > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:191 > + m_networkSession = nullptr; This change makes sense in terms of cleaning but is probably not strictly needed, right? Looking at WebResourceLoadStatisticsStore, it seems that WebResourceLoadStatisticsStore::requestStorageAccess uses m_networkSession without checking it before. I do not see how we can guarantee it to not be null. In the same method, we could also remove one unnecessary 'if (m_statisticsStore)' check.
(In reply to youenn fablet from comment #5) > LGTM too. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374754&action=review > > > Source/WebKit/ChangeLog:27 > > + m_statisticsStore / m_persistentStorage on the background queue, synchronously. The > > This is not new but this potentially blocks the main thread for the > background thread to finish its I/O tasks. > Could there be a way to unblock the main thread? Yes, this is pre-existing behavior. We need to guarantee somehow that everything gets flushed to disk. > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:182 > > ASSERT(RunLoop::isMain()); > > This assert is not needed given WebResourceLoadStatisticsStore is > DestructionThread::Main It is an assertion. It ensures this remains true and it documents the code. I do not think it hurts to keep it. > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:191 > > + m_networkSession = nullptr; > > This change makes sense in terms of cleaning but is probably not strictly > needed, right? Probably not given that m_networkSession is a WeakPtr and the NetworkSession destructor is currently running. However, it does feel cleaner to null it out at this point. It also makes sure that no code tries to interact with the network session after this point and before the NetworkSession destructor has finished running. > Looking at WebResourceLoadStatisticsStore, it seems that > WebResourceLoadStatisticsStore::requestStorageAccess uses m_networkSession > without checking it before. I do not see how we can guarantee it to not be > null. Yes, this looks like a pre-existing bug. m_networkSession is a WeakPtr and all accesses to it are null checked. I will fix before landing. > In the same method, we could also remove one unnecessary 'if > (m_statisticsStore)' check. Pre-existing code but you're right. I can fix before landing.
Created attachment 374757 [details] Patch
Comment on attachment 374757 [details] Patch Clearing flags on attachment: 374757 Committed r247766: <https://trac.webkit.org/changeset/247766>
All reviewed patches have been landed. Closing bug.
Reverted r247766 for reason: Caused an API test failure Committed r247769: <https://trac.webkit.org/changeset/247769>
Created attachment 374791 [details] Patch
Comment on attachment 374791 [details] Patch Clearing flags on attachment: 374791 Committed r247784: <https://trac.webkit.org/changeset/247784>