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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 21 13:52:23 PDT 2018

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

Attachment 343265: Patch


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

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
()>&& 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

> 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)] {


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


More information about the webkit-reviews mailing list