Bug 200071 - Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call
Summary: Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatistic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 200085 200517
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-23 19:12 PDT by Chris Dumez
Modified: 2019-08-07 13:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.06 KB, patch)
2019-07-23 19:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.11 KB, patch)
2019-07-23 19:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2019-07-23 20:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2019-07-24 11:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-07-23 19:12:57 PDT
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
Comment 1 Chris Dumez 2019-07-23 19:13:11 PDT
<rdar://problem/53335583>
Comment 2 Chris Dumez 2019-07-23 19:23:02 PDT
Created attachment 374753 [details]
Patch
Comment 3 Chris Dumez 2019-07-23 19:26:30 PDT
Created attachment 374754 [details]
Patch
Comment 4 Brent Fulgham 2019-07-23 20:15:03 PDT
Comment on attachment 374754 [details]
Patch

Thank you for tracking this down, Chris. R=Me
Comment 5 youenn fablet 2019-07-23 20:21:37 PDT
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.
Comment 6 Chris Dumez 2019-07-23 20:40:36 PDT
(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.
Comment 7 Chris Dumez 2019-07-23 20:47:44 PDT
Created attachment 374757 [details]
Patch
Comment 8 WebKit Commit Bot 2019-07-23 21:29:15 PDT
Comment on attachment 374757 [details]
Patch

Clearing flags on attachment: 374757

Committed r247766: <https://trac.webkit.org/changeset/247766>
Comment 9 WebKit Commit Bot 2019-07-23 21:29:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Chris Dumez 2019-07-24 10:10:25 PDT
Reverted r247766 for reason:

Caused an API test failure

Committed r247769: <https://trac.webkit.org/changeset/247769>
Comment 11 Chris Dumez 2019-07-24 11:40:17 PDT
Created attachment 374791 [details]
Patch
Comment 12 WebKit Commit Bot 2019-07-24 12:21:55 PDT
Comment on attachment 374791 [details]
Patch

Clearing flags on attachment: 374791

Committed r247784: <https://trac.webkit.org/changeset/247784>
Comment 13 WebKit Commit Bot 2019-07-24 12:21:57 PDT
All reviewed patches have been landed.  Closing bug.