Bug 214771 - WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch
Summary: WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
Keywords: InRadar
Depends on:
Reported: 2020-07-24 17:10 PDT by Geoffrey Garen
Modified: 2020-07-28 10:03 PDT (History)
11 users (show)

See Also:

Patch (4.20 KB, patch)
2020-07-24 17:35 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (4.72 KB, patch)
2020-07-25 15:38 PDT, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff
Patch for landing (4.71 KB, patch)
2020-07-27 11:06 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2020-07-24 17:10:26 PDT
WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch
Comment 1 Geoffrey Garen 2020-07-24 17:35:24 PDT
Created attachment 405207 [details]
Comment 2 Alex Christensen 2020-07-24 19:55:54 PDT
Comment on attachment 405207 [details]

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

The code in WebCoreResourceHandleAsOperationQueueDelegate.mm took several iterations to get just right, and I don't think the code in WTF is equivalent.  Could you include a radar with a list of clients?

> Source/WTF/wtf/cf/RunLoopCF.cpp:91
> +        if (schedulePair->mode() == kCFRunLoopCommonModes

What about kCFRunLoopDefaultMode?
Comment 3 Geoffrey Garen 2020-07-25 15:00:07 PDT
> Could you include a radar with a list of clients?

I can assemble a list. But what would the Radar say?
> > Source/WTF/wtf/cf/RunLoopCF.cpp:91
> > +        if (schedulePair->mode() == kCFRunLoopCommonModes
> What about kCFRunLoopDefaultMode?

The kCFRunLoopDefaultMode was subtly incorrect. dispatch() dispatches in kCFRunLoopCommonModes, so if a client took care to first unschedule kCFRunLoopCommonModes (which WebKit schedules by default) and then schedule kCFRunLoopDefaultMode, applying this optimization would sometimes dispatch in the wrong modes.
Comment 4 Geoffrey Garen 2020-07-25 15:38:18 PDT
Created attachment 405233 [details]
Comment 5 Darin Adler 2020-07-25 22:10:30 PDT
Comment on attachment 405233 [details]

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

> Source/WTF/wtf/cf/RunLoopCF.cpp:92
> +        if (schedulePair->mode() == kCFRunLoopCommonModes
> +            && schedulePair->runLoop() == RunLoop::main().m_runLoop)

Easier to read as a long line.
Comment 6 Geoffrey Garen 2020-07-27 11:06:48 PDT
Created attachment 405295 [details]
Patch for landing
Comment 7 Geoffrey Garen 2020-07-27 16:45:44 PDT
Those failing API tests pass locally (Debug and Release), and the flakiness dashboard also reports that they fail regularly on bots. Not sure how to interpret these contradictory reasons to exonerate this patch. I guess I'll try landing and see what happens.
Comment 8 EWS 2020-07-27 16:48:21 PDT
Committed r264955: <https://trac.webkit.org/changeset/264955>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405295 [details].
Comment 9 Radar WebKit Bug Importer 2020-07-27 16:49:27 PDT
Comment 10 Aakash Jain 2020-07-28 03:46:10 PDT
(In reply to EWS from comment #8)
> Committed r264955: <https://trac.webkit.org/changeset/264955>
TestWebKitAPI.WKWebViewCloseAllMediaPresentations.PictureInPicture seems to have started to consistently timeout after this change.

History: https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WKWebViewCloseAllMediaPresentations.PictureInPicture
Comment 11 Karl Rackler 2020-07-28 10:03:07 PDT
Reverted r264955 for reason:

Reverting because this commit may have caused issues with tests. 

Committed r264993: <https://trac.webkit.org/changeset/264993>