| Summary: | Update media state for active speech recognition as it uses audio capture | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||
| Component: | Media | Assignee: | Sihui Liu <sihui_liu> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | cdumez, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer, youennf | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Sihui Liu
2021-01-15 13:42:12 PST
Created attachment 417737 [details]
Patch
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. Created attachment 417858 [details]
Patch
(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. (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. Created attachment 417882 [details]
Patch
(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 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? Created attachment 417939 [details]
Patch for landing
Committed r271636: <https://trac.webkit.org/changeset/271636> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417939 [details]. |