[webkit-reviews] review granted: [Bug 189098] Crash under WebKit: WTF::Function<void ()>::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_1>::call() : [Attachment 348434] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 189098: Crash under WebKit: WTF::Function<void
()>::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecor
ds(WTF::CompletionHandler<void ()>&&)::$_1>::call()
https://bugs.webkit.org/show_bug.cgi?id=189098

Attachment 348434: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 348434
  --> https://bugs.webkit.org/attachment.cgi?id=348434
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.