[webkit-reviews] review denied: [Bug 183216] [iOS Debug] Multiple resourceLoadStatistics redirect tests are flaky timeouts : [Attachment 343265] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 183216: [iOS Debug] Multiple resourceLoadStatistics redirect tests are
flaky timeouts
https://bugs.webkit.org/show_bug.cgi?id=183216

Attachment 343265: Patch

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




--- Comment #7 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 343265
  --> https://bugs.webkit.org/attachment.cgi?id=343265
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1044
> +	       grandfatherExistingWebsiteData([protectedThis =
WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]()
mutable {

Yes, I think this is a good way to fix this. grandfatherExistingWebsiteData()
is called on the bg thread and calls its completion handler on the bg thread.
scheduleClearInMemoryAndPersistent() is called on the main thread so it is the
one doing the dispatch to the main thread before calling its completion
handler. The fix below is not consistent however, see my comment.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1234
> +void
WebResourceLoadStatisticsStore::updateCookiePartitioning(CompletionHandler<void
()>&& completionHandler)

Given that this method is called on the bg thread, I think it would be better
to call the completion handler on the same bg thread. If some call sites need
to call a completion handler on the main thread, they can dispatch themselves.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1260
> +	   RunLoop::main().dispatch([completionHandler =
WTFMove(completionHandler)] {

I do not think we should do this here. Let's do at the call site instead.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1302
> +	   completionHandler();

ditto, I don't think we should call the completion handler on the main thread
here.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1313
> +	   RunLoop::main().dispatch([completionHandler =
WTFMove(completionHandler)] {

updateCookiePartitioningForDomains() is called on the bg thread, so I think it
is awkward for it to call its completion handler on the different thread.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1348
> +	   RunLoop::main().dispatch([completionHandler =
WTFMove(completionHandler)] {

ditto.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1364
> +    RunLoop::main().dispatch([completionHandler =
WTFMove(completionHandler)] {

ditto.