Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WebPermissionControllerProxy::Query message reply should go to correct completion handler #5984

Merged
merged 1 commit into from Nov 3, 2022
Merged

WebPermissionControllerProxy::Query message reply should go to correct completion handler #5984

merged 1 commit into from Nov 3, 2022

Conversation

achristensen07
Copy link
Contributor

@achristensen07 achristensen07 commented Oct 31, 2022 •
edited by webkit-early-warning-system

0310eb3

WebPermissionControllerProxy::Query message reply should go to correct completion handler
https://bugs.webkit.org/show_bug.cgi?id=247295
rdar://100894959

Reviewed by Sihui Liu.

* Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:
(WebKit::WebPermissionController::tryProcessingRequests):
* Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h:

Canonical link: https://commits.webkit.org/256298@main

c9f3c29

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
  🧪 api-ios   🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 watch   🧪 mac-AS-debug-wk2
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@achristensen07 achristensen07 self-assigned this Oct 31, 2022
@achristensen07 achristensen07 added WebKit Local Build WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). labels Oct 31, 2022
@achristensen07 achristensen07 force-pushed the eng/WebPermissionControllerProxyQuery-message-reply-should-go-to-correct-completion-handler branch from 723d168 to 104a36d Compare October 31, 2022 22:47
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 104a36d)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
❌ 🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 watch ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 723d168)

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ⏳ 🛠 ios ⏳ 🛠 mac ❌ 🛠 wpe ⏳ 🛠 🧪 win
⏳ 🧪 bindings ✅ 🛠 ios-sim ⏳ 🛠 mac-debug ❌ 🛠 gtk ⏳ 🛠 wincairo
⏳ 🧪 webkitperl ⏳ 🧪 ios-wk2 ⏳ 🛠 mac-AS-debug ❌ 🧪 gtk-wk2
⏳ 🧪 webkitpy ⏳ 🧪 api-ios ⏳ 🧪 api-mac ❌ 🧪 api-gtk
⏳ 🛠 🧪 jsc ⏳ 🛠 tv ⏳ 🧪 mac-wk1
⏳ 🧪 services ⏳ 🛠 tv-sim ⏳ 🧪 mac-wk2
⏳ 🛠 watch ⏳ 🧪 mac-AS-debug-wk2
⏳ 🛠 watch-sim ⏳ 🧪 mac-wk2-stress

szewai
szewai reviewed Nov 1, 2022
View reviewed changes
Copy link
Contributor

@szewai szewai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch is fine for making requests non-blocking and the code simpler; but I am not sure it will fix the crash in the radar.

If we are going to make this change, maybe we should also remove m_requests (just send Query message in query())

johnwilander
johnwilander reviewed Nov 1, 2022
View reviewed changes
Copy link
Contributor

@johnwilander johnwilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to test this?

Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp

m_requests.takeFirst().completionHandler(state);
tryProcessingRequests();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will tryProcessingRequests() become dead code now? What did it do previously?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In existing implementation, tryProcessingRequests just sends one async message for the first request in queue. If the first request has already sent message and no reply, it will return. That means one unhandled request will block the other requests in queue.
When the reply is received, the first request will be taken out of queue and tryProcessingRequests` is called to proceed next message.

We had this design because WebPermissionController used to cache permission result, which means tryProcessingRequests might handle multiple requests at a time (if the result is in the cache), and receiving one message reply (caching the result) may benefit the subsequent requests.

But we removed the cache and send message for each request now, so we can send them when request is received, and finish CompletionHandler in message reply (as in this patch). And there is not need to store the requests in m_requests.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 2, 2022
@achristensen07 achristensen07 force-pushed the eng/WebPermissionControllerProxyQuery-message-reply-should-go-to-correct-completion-handler branch from 104a36d to c9f3c29 Compare November 3, 2022 04:32
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash c9f3c29)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
  🧪 api-ios   🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
✅ 🛠 watch   🧪 mac-AS-debug-wk2
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

szewai
szewai approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@szewai szewai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@achristensen07 achristensen07 added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Nov 3, 2022
@achristensen07
WebPermissionControllerProxy::Query message reply should go to correc…
0310eb3 
…t completion handler

https://bugs.webkit.org/show_bug.cgi?id=247295
rdar://100894959

Reviewed by Sihui Liu.

* Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:
(WebKit::WebPermissionController::tryProcessingRequests):
* Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h:

Canonical link: https://commits.webkit.org/256298@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/WebPermissionControllerProxyQuery-message-reply-should-go-to-correct-completion-handler branch from c9f3c29 to 0310eb3 Compare November 3, 2022 23:18
@webkit-commit-queue
Copy link
Collaborator

Committed 256298@main (0310eb3): https://commits.webkit.org/256298@main

Reviewed commits have been landed. Closing PR #5984 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 0310eb3 into WebKit:main Nov 3, 2022
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnwilander johnwilander johnwilander left review comments

@szewai szewai szewai approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@achristensen07 achristensen07

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
6 participants
@achristensen07 @webkit-early-warning-system @webkit-commit-queue @johnwilander @szewai @webkit-ews-buildbot