...
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].
<rdar://problem/73387089>