Bug 220667 - Update media state for active speech recognition as it uses audio capture
Summary: Update media state for active speech recognition as it uses audio capture
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-15 13:42 PST by Sihui Liu
Modified: 2021-01-19 20:34 PST (History)
6 users (show)

See Also:


Attachments
Patch (11.83 KB, patch)
2021-01-15 14:15 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (11.94 KB, patch)
2021-01-18 20:48 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (18.37 KB, patch)
2021-01-19 09:48 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (17.81 KB, patch)
2021-01-19 19:37 PST, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2021-01-15 13:42:12 PST
...
Comment 1 Sihui Liu 2021-01-15 14:15:03 PST
Created attachment 417737 [details]
Patch
Comment 2 youenn fablet 2021-01-18 09:38:44 PST
Comment on attachment 417737 [details]
Patch

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

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

What is the guarantee to have a valid scriptExecutionContext() here?
If we want to ensure that, we might want to remove SpeechRecognition from m_clientMap when being stopped.

> Source/WebCore/Modules/speech/SpeechRecognition.cpp:142
> +    document.setActiveSpeechRecognition(nullptr);

Ditto.
Comment 3 Sihui Liu 2021-01-18 20:48:46 PST
Created attachment 417858 [details]
Patch
Comment 4 Sihui Liu 2021-01-18 20:51:38 PST
(In reply to youenn fablet from comment #2)
> Comment on attachment 417737 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417737&action=review
> 
> > Source/WebCore/Modules/speech/SpeechRecognition.cpp:112
> > +    auto& document = downcast<Document>(*scriptExecutionContext());
> 
> What is the guarantee to have a valid scriptExecutionContext() here?
> If we want to ensure that, we might want to remove SpeechRecognition from
> m_clientMap when being stopped.

Do you mean removing SpeechRecognition from m_clientMap when it is already stopped?

How about just checking if the context is null (like in the updated patch)?

> 
> > Source/WebCore/Modules/speech/SpeechRecognition.cpp:142
> > +    document.setActiveSpeechRecognition(nullptr);
> 
> Ditto.
Comment 5 youenn fablet 2021-01-19 02:03:09 PST
(In reply to Sihui Liu from comment #4)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 417737 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=417737&action=review
> > 
> > > Source/WebCore/Modules/speech/SpeechRecognition.cpp:112
> > > +    auto& document = downcast<Document>(*scriptExecutionContext());
> > 
> > What is the guarantee to have a valid scriptExecutionContext() here?
> > If we want to ensure that, we might want to remove SpeechRecognition from
> > m_clientMap when being stopped.
> 
> Do you mean removing SpeechRecognition from m_clientMap when it is already
> stopped?

Right, idea would be to remove it proactively in SpeechRecognition::stop (the ActiveDOMObject version). Currently, this is done lazily: m_clientMap could in theory grow without ever decreasing.

> How about just checking if the context is null (like in the updated patch)?

Right, this would work as well.
Comment 6 Sihui Liu 2021-01-19 09:48:32 PST
Created attachment 417882 [details]
Patch
Comment 7 Sihui Liu 2021-01-19 10:00:33 PST
(In reply to youenn fablet from comment #5)
> (In reply to Sihui Liu from comment #4)
> > (In reply to youenn fablet from comment #2)
> > > Comment on attachment 417737 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=417737&action=review
> > > 
> > > > Source/WebCore/Modules/speech/SpeechRecognition.cpp:112
> > > > +    auto& document = downcast<Document>(*scriptExecutionContext());
> > > 
> > > What is the guarantee to have a valid scriptExecutionContext() here?
> > > If we want to ensure that, we might want to remove SpeechRecognition from
> > > m_clientMap when being stopped.
> > 
> > Do you mean removing SpeechRecognition from m_clientMap when it is already
> > stopped?
> 
> Right, idea would be to remove it proactively in SpeechRecognition::stop
> (the ActiveDOMObject version). Currently, this is done lazily: m_clientMap
> could in theory grow without ever decreasing.

I see, updated.

> 
> > How about just checking if the context is null (like in the updated patch)?
> 
> Right, this would work as well.
Comment 8 youenn fablet 2021-01-19 11:53:33 PST
Comment on attachment 417882 [details]
Patch

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

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

I am not sure this change is needed, we will anyway get a nullptr crash below.

> Source/WebCore/Modules/speech/SpeechRecognition.h:89
>      void suspend(ReasonForSuspension);

final as well?
Comment 9 Sihui Liu 2021-01-19 19:37:30 PST
Created attachment 417939 [details]
Patch for landing
Comment 10 EWS 2021-01-19 20:33:17 PST
Committed r271636: <https://trac.webkit.org/changeset/271636>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417939 [details].
Comment 11 Radar WebKit Bug Importer 2021-01-19 20:34:15 PST
<rdar://problem/73387089>