Bug 221296

Summary: SpeechRecognitionPermissionManager should not handle requests that are already cancelled
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: MediaAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Sihui Liu 2021-02-02 14:28:11 PST
...
Comment 1 Sihui Liu 2021-02-02 17:28:52 PST
Created attachment 419087 [details]
Patch
Comment 2 Sihui Liu 2021-02-02 20:33:53 PST
Created attachment 419097 [details]
Patch
Comment 3 Sihui Liu 2021-02-02 20:42:29 PST
Created attachment 419099 [details]
Patch
Comment 4 youenn fablet 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.
Comment 5 Sihui Liu 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.
Comment 6 Sihui Liu 2021-02-03 23:13:49 PST
Created attachment 419239 [details]
Patch
Comment 7 Sihui Liu 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.
Comment 8 youenn fablet 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.
Comment 9 Sihui Liu 2021-02-04 21:34:06 PST
Created attachment 419361 [details]
Patch
Comment 10 EWS 2021-02-04 22:51:08 PST
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Comment 11 Sihui Liu 2021-02-04 23:42:05 PST
Created attachment 419367 [details]
Patch for landing
Comment 12 EWS 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.
Comment 13 Sihui Liu 2021-02-05 08:20:37 PST
Created attachment 419410 [details]
Patch for landing
Comment 14 EWS 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].
Comment 15 Radar WebKit Bug Importer 2021-02-05 08:55:15 PST
<rdar://problem/74026953>