RESOLVED FIXED 221296
SpeechRecognitionPermissionManager should not handle requests that are already cancelled
https://bugs.webkit.org/show_bug.cgi?id=221296
Summary SpeechRecognitionPermissionManager should not handle requests that are alread...
Sihui Liu
Reported 2021-02-02 14:28:11 PST
...
Attachments
Patch (31.34 KB, patch)
2021-02-02 17:28 PST, Sihui Liu
no flags
Patch (28.38 KB, patch)
2021-02-02 20:33 PST, Sihui Liu
ews-feeder: commit-queue-
Patch (30.27 KB, patch)
2021-02-02 20:42 PST, Sihui Liu
no flags
Patch (17.00 KB, patch)
2021-02-03 23:13 PST, Sihui Liu
no flags
Patch (17.87 KB, patch)
2021-02-04 21:34 PST, Sihui Liu
no flags
Patch for landing (17.83 KB, patch)
2021-02-04 23:42 PST, Sihui Liu
no flags
Patch for landing (17.84 KB, patch)
2021-02-05 08:20 PST, Sihui Liu
no flags
Sihui Liu
Comment 1 2021-02-02 17:28:52 PST
Sihui Liu
Comment 2 2021-02-02 20:33:53 PST
Sihui Liu
Comment 3 2021-02-02 20:42:29 PST
youenn fablet
Comment 4 2021-02-03 01:35:38 PST
Comment on attachment 419099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419099&action=review > Source/WebCore/ChangeLog:9 > + user permission granting. We should be able to write a permission delegate that delays the user permission fo ever. Either through test runner or an API test. > Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:95 > + if (requestIterator == m_requests.begin() || requestIterator == m_requests.end()) Why do we need both, I would think if (requestIterator == m_requests.end()) should work? > Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:75 > + auto permissionRequestIdentifier = permissionManager->request(request.lang(), request.clientOrigin(), request.frameIdentifier(), [this, weakThis = makeWeakPtr(this), weakRequest = makeWeakPtr(request)](auto error) mutable { If we pass request directly to permissionManager, SpeechRecognitionPermissionRequest could take a WeakPtr<SpeechRecognitionRequest>. If its WeakPtr<SpeechRecognitionRequest> is null at the start of processing the permission request, we could just bail out. In that case, SpeechRecognitionServer::cancel would just need to destroy the request and would not need to call permissionManager->cancelRequest.
Sihui Liu
Comment 5 2021-02-03 09:49:47 PST
Comment on attachment 419099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419099&action=review >> Source/WebCore/ChangeLog:9 >> + user permission granting. > > We should be able to write a permission delegate that delays the user permission fo ever. > Either through test runner or an API test. Hmm, after considering about this I think it's possible. Let me try >> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:95 >> + if (requestIterator == m_requests.begin() || requestIterator == m_requests.end()) > > Why do we need both, I would think if (requestIterator == m_requests.end()) should work? If requestIterator == m_requests.begin(), it means the request already starts (request being queued gets processed immediately if there is no request ahead). During requesting handling process, SpeechRecognitionPermissionManager would just look at the first request in queue and think it's the request being handled, so here we just let started request run to the end and it will fail in completion handler. >> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:75 >> + auto permissionRequestIdentifier = permissionManager->request(request.lang(), request.clientOrigin(), request.frameIdentifier(), [this, weakThis = makeWeakPtr(this), weakRequest = makeWeakPtr(request)](auto error) mutable { > > If we pass request directly to permissionManager, SpeechRecognitionPermissionRequest could take a WeakPtr<SpeechRecognitionRequest>. > If its WeakPtr<SpeechRecognitionRequest> is null at the start of processing the permission request, we could just bail out. > In that case, SpeechRecognitionServer::cancel would just need to destroy the request and would not need to call permissionManager->cancelRequest. That should work too, will change.
Sihui Liu
Comment 6 2021-02-03 23:13:49 PST
Sihui Liu
Comment 7 2021-02-04 08:45:03 PST
(In reply to Sihui Liu from comment #5) > Comment on attachment 419099 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419099&action=review > > >> Source/WebCore/ChangeLog:9 > >> + user permission granting. > > > > We should be able to write a permission delegate that delays the user permission fo ever. > > Either through test runner or an API test. > > Hmm, after considering about this I think it's possible. Let me try Nope, current implementation also does not show user permission prompt if pending request is cancelled, because SpeechRecognitionPermissionManager remembers the user permission until top level document is changed. It's possible to present TCC prompt, if permission happens to be reset between requests, but it cannot be tested with WTR I guess. So the patch is mainly for reducing unnecessary workload of SpeechRecognitionPermissionManager and unnecessary TCC prompts I guess.
youenn fablet
Comment 8 2021-02-04 09:04:04 PST
Comment on attachment 419239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419239&action=review > Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:227 > + auto clientOrigin = recognitionRequest->clientOrigin(); Maybe pass recognitionRequest as a parameter to requestUserPermission. That will remove the need for the assert.
Sihui Liu
Comment 9 2021-02-04 21:34:06 PST
EWS
Comment 10 2021-02-04 22:51:08 PST
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Sihui Liu
Comment 11 2021-02-04 23:42:05 PST
Created attachment 419367 [details] Patch for landing
EWS
Comment 12 2021-02-05 00:11:46 PST
commit-queue failed to commit attachment 419367 [details] to WebKit repository. To retry, please set cq+ flag again.
Sihui Liu
Comment 13 2021-02-05 08:20:37 PST
Created attachment 419410 [details] Patch for landing
EWS
Comment 14 2021-02-05 08:54:05 PST
Committed r272417: <https://trac.webkit.org/changeset/272417> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419410 [details].
Radar WebKit Bug Importer
Comment 15 2021-02-05 08:55:15 PST
Note You need to log in before you can comment on or make changes to this bug.