[webkit-reviews] review granted: [Bug 218476] Implement basic permission check for SpeechRecognition : [Attachment 413839] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 218476: Implement basic permission check for SpeechRecognition
https://bugs.webkit.org/show_bug.cgi?id=218476

Attachment 413839: Patch

https://bugs.webkit.org/attachment.cgi?id=413839&action=review




--- Comment #36 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 413839
  --> https://bugs.webkit.org/attachment.cgi?id=413839
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413839&action=review

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:73
> +    auto& document = downcast<Document>(*scriptExecutionContext());

Can we add a test trying to call start on an object created in an iframe that
is gone.
scriptExecutionContext might be null and I am not sure m_state and m_connection
checks cover this case.

> Source/WebKit/UIProcess/Cocoa/MediaPermissionUtilities.mm:94
> +    }

Should we use std::call_once here as well?

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:92
> +    m_speechRecognitionServiceCheck =
computeSpeechRecognitionServiceAccess();

I would tend to move these in startProcessingRequest and inline
checkMicrophoneAccess and checkSpeechRecognitionServiceAccess inside
startProcessingRequest.

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:149
> +	   m_microphoneCheck = CheckResult::Granted;

checkMicrophoneAccess is changing m_microphoneCheck while its name looks as if
it should be const.
I would inline this code in startNextRequest

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:164
> +    ASSERT(isMainThread());

No longer needed?

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:185
> +    ASSERT(isMainThread());

No longer needed?

> Source/WebKit/UIProcess/SpeechRecognitionPermissionManager.cpp:212
> +    if (!requestingOrigin->isSameOriginAs(topOrigin)) {

We should probably do that same origin check before the TCC prompts.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:45
> +    ASSERT(pendingRequest.isNewEntry);

In theory, we cannot really trust WebProcess and this clientIdentifier comes
straight from IPC.
Maybe we should add a message check instead.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:46
> +    pendingRequest.iterator->value =
makeUnique<WebCore::SpeechRecognitionRequest>(SpeechRecognitionRequestInfo {
clientIdentifier, WTFMove(lang), continuous, interimResults, maxAlternatives,
WTFMove(origin) });

I tend to prefer:
ASSERT(!m_pendingRequests.contains(clientIdentifier));
auto& pendingRequest = m_pendingRequests.add(clientIdentifier,
makeUnique<>...).iterator->value;

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:68
> +	   m_ongoingRequests.add(identifier, WTFMove(takenRequest));

Are we sure m_ongoingRequests does not have identifier?
It seems maybe we should ensure in start that m_pendingRequests and
m_ongoingIdentifiers do not contain identifier.
Or we could have just one map and have a status in the request object.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:75
> +    if (m_pendingRequests.contains(clientIdentifier)) {

I would write it as if (m_pendingRequests.remove(clientIdentifier)) to improve
efficiency.
Ditto below.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:81
> +    ASSERT(m_ongoingRequests.contains(clientIdentifier));

This also comes straight from IPC.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:93
> +    ASSERT(m_ongoingRequests.contains(clientIdentifier));

Ditto.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:102
> +    auto request = m_ongoingRequests.take(clientIdentifier);

There is no guarantee request is not null.

> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:127
> +	   update =
WebCore::SpeechRecognitionUpdate::createResult(clientIdentifier, *result);

We allocate a vector here for a very quick moment.
It might be better to just send all members in IPC directly and reconstruct the
object in WebProcess to remove count churn and Vector allocations.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1723
> +    }

If there is no page, there is no need to change the map. We should exit early.
There is no guarantee that m_speechRecognitionServerMap does not already
contain the identifier as well.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1725
> +    speechRecognitionServer.iterator->value =
makeUnique<SpeechRecognitionServer>(makeRef(*connection()), identifier,
[weakThis = makeWeakPtr(this), weakPage = makeWeakPtr(targetPage)](const
ClientOrigin& origin,
CompletionHandler<void(SpeechRecognitionPermissionDecision)>&&
completionHandler) mutable {

(auto& origin, auto&& completionHandler)

> Source/WebKit/UIProcess/WebProcessProxy.cpp:1727
> +	       return;

No need for weakThis, let's not capture it.