Bug 189098

Summary: Crash under WebKit: WTF::Function<void ()>::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_1>::call()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, ddkilzer, ggaren, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2018-08-29 14:39:44 PDT 
Crash under WebKit: WTF::Function<void ()>::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_1>::call():
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   WebKit                        	0x00000002208e65c8 WTF::Function<void ()>::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_1>::call() + 76
1   WebKit                        	0x00000002208e65a8 WTF::Function<void ()>::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_1>::call() + 44
2   JavaScriptCore                	0x00000002181780f0 WTF::RunLoop::performWork() + 276
3   JavaScriptCore                	0x00000002181783b8 WTF::RunLoop::performWork(void*) + 36
4   CoreFoundation                	0x0000000210e365b8 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
5   CoreFoundation                	0x0000000210e36538 __CFRunLoopDoSource0 + 88
6   CoreFoundation                	0x0000000210e35e1c __CFRunLoopDoSources0 + 176
7   CoreFoundation                	0x0000000210e30ce8 __CFRunLoopRun + 1040
8   CoreFoundation                	0x0000000210e305b8 CFRunLoopRunSpecific + 436
9   GraphicsServices              	0x00000002130a3584 GSEventRunModal + 100
10  UIKitCore                     	0x000000023df892dc UIApplicationMain + 212
11  SafariViewService             	0x000000010006a8bc main + 244
12  libdyld.dylib                 	0x00000002108f0b94 start + 4
Comment 1 Chris Dumez 2018-08-29 14:39:56 PDT 
<rdar://problem/43179891>
Comment 2 Chris Dumez 2018-08-29 14:46:13 PDT 
Created attachment 348434 [details]
Patch
Comment 3 youenn fablet 2018-08-29 16:23:19 PDT 
Comment on attachment 348434 [details]
Patch

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

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:246
>  

It seems that callback here is a completion handler.
There is no guarantee though that it will be called if the workQueue is stopped. Maybe it should be WTF::Function().

Also, is it fine that callback be destroyed in the main thread?
From my reading of the code, it seems ok right now since the passed callbacks are capturing weakThis, but it would be quite easy to break that assumption.

Maybe removeDataRecords should not take a callback at all.
Instead, it could create the callback itself when being called, this would remove this double weakThis check and be more future proof.
Comment 4 Chris Dumez 2018-08-29 16:24:43 PDT 
(In reply to youenn fablet from comment #3)
> Comment on attachment 348434 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348434&action=review
> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:246
> >  
> 
> It seems that callback here is a completion handler.
> There is no guarantee though that it will be called if the workQueue is
> stopped. Maybe it should be WTF::Function().
> 
> Also, is it fine that callback be destroyed in the main thread?
> From my reading of the code, it seems ok right now since the passed
> callbacks are capturing weakThis, but it would be quite easy to break that
> assumption.
> 
> Maybe removeDataRecords should not take a callback at all.
> Instead, it could create the callback itself when being called, this would
> remove this double weakThis check and be more future proof.

If you do a WorkQueue::dispatch(c), c is *guaranteed* to run.
Comment 5 Chris Dumez 2018-08-29 16:41:41 PDT 
Comment on attachment 348434 [details]
Patch

Clearing flags on attachment: 348434

Committed r235487: <https://trac.webkit.org/changeset/235487>
Comment 6 Chris Dumez 2018-08-29 16:41:43 PDT 
All reviewed patches have been landed.  Closing bug.