Bug 200071 – Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

<rdar://problem/53335583>

[Chris Dumez]

Created attachment 374753 [details]
Patch

[Chris Dumez]

Created attachment 374754 [details]
Patch

[Brent Fulgham]

Comment on attachment 374754 [details]
Patch

Thank you for tracking this down, Chris. R=Me

[youenn fablet]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 374757 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374757 [details]
Patch

Clearing flags on attachment: 374757

Committed r247766: <https://trac.webkit.org/changeset/247766>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reverted r247766 for reason:

Caused an API test failure

Committed r247769: <https://trac.webkit.org/changeset/247769>

[Chris Dumez]

Created attachment 374791 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374791 [details]
Patch

Clearing flags on attachment: 374791

Committed r247784: <https://trac.webkit.org/changeset/247784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.