WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(28.38 KB, patch)
2021-02-02 20:33 PST
,
Sihui Liu
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(30.27 KB, patch)
2021-02-02 20:42 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(17.00 KB, patch)
2021-02-03 23:13 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch
(17.87 KB, patch)
2021-02-04 21:34 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch for landing
(17.83 KB, patch)
2021-02-04 23:42 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Patch for landing
(17.84 KB, patch)
2021-02-05 08:20 PST
,
Sihui Liu
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Sihui Liu
Comment 1
2021-02-02 17:28:52 PST
Created
attachment 419087
[details]
Patch
Sihui Liu
Comment 2
2021-02-02 20:33:53 PST
Created
attachment 419097
[details]
Patch
Sihui Liu
Comment 3
2021-02-02 20:42:29 PST
Created
attachment 419099
[details]
Patch
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
Created
attachment 419239
[details]
Patch
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
Created
attachment 419361
[details]
Patch
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
<
rdar://problem/74026953
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug