Bug 221296 – SpeechRecognitionPermissionManager should not handle requests that are already cancelled

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Sihui Liu]

...

[Sihui Liu]

Created attachment 419087 [details]
Patch

[Sihui Liu]

Created attachment 419097 [details]
Patch

[Sihui Liu]

Created attachment 419099 [details]
Patch

[youenn fablet]

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

Created attachment 419239 [details]
Patch

[Sihui Liu]

(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 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]

Created attachment 419361 [details]
Patch

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[Sihui Liu]

Created attachment 419367 [details]
Patch for landing

[EWS]

commit-queue failed to commit attachment 419367 [details] to WebKit repository. To retry, please set cq+ flag again.

[Sihui Liu]

Created attachment 419410 [details]
Patch for landing

[EWS]

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]

<rdar://problem/74026953>