| Summary: | REGRESSION(r272337): crash under WebCore::SpeechRecognizer::setInactive() | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||
| Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | webkit-bug-importer, youennf | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Sihui Liu
2021-02-04 21:40:51 PST
0 com.apple.WebKit 0x000000010686353c WebCore::SpeechRecognizer::setInactive() + 12 (SpeechRecognizer.h:59) 1 com.apple.WebKit 0x00000001068634c0 auto WebKit::SpeechRecognitionServer::handleRequest(WTF::UniqueRef<WebCore::SpeechRecognitionRequest>&&)::$_0::operator()<WebCore::SpeechRecognitionUpdate const>(WebCore::SpeechRecognitionUpdate const&) const + 192 (SpeechRecognitionServer.cpp:104) 2 com.apple.WebKit 0x00000001068632b3 WTF::Detail::CallableWrapper<WebKit::SpeechRecognitionServer::handleRequest(WTF::UniqueRef<WebCore::SpeechRecognitionRequest>&&)::$_0, void, WebCore::SpeechRecognitionUpdate const&>::call(WebCore::SpeechRecognitionUpdate const&) + 51 (Function.h:52) 3 com.apple.WebCore 0x000000011457af3a WTF::Function<void (WebCore::SpeechRecognitionUpdate const&)>::operator()(WebCore::SpeechRecognitionUpdate const&) const + 154 (Function.h:83) 4 com.apple.WebCore 0x00000001162b3f22 WebCore::SpeechRecognizer::~SpeechRecognizer() + 130 (SpeechRecognizer.cpp:48) 5 com.apple.WebCore 0x00000001162b4065 WebCore::SpeechRecognizer::~SpeechRecognizer() + 21 (SpeechRecognizer.cpp:46) 6 com.apple.WebKit 0x0000000105d8501b std::__1::default_delete<WebCore::SpeechRecognizer>::operator()(WebCore::SpeechRecognizer*) const + 43 (memory:2368) 7 com.apple.WebKit 0x0000000105d84f9f std::__1::unique_ptr<WebCore::SpeechRecognizer, std::__1::default_delete<WebCore::SpeechRecognizer> >::reset(WebCore::SpeechRecognizer*) + 95 (memory:2623) 8 com.apple.WebKit 0x0000000105d84f39 std::__1::unique_ptr<WebCore::SpeechRecognizer, std::__1::default_delete<WebCore::SpeechRecognizer> >::~unique_ptr() + 25 (memory:2577) 9 com.apple.WebKit 0x0000000105d84bd5 std::__1::unique_ptr<WebCore::SpeechRecognizer, std::__1::default_delete<WebCore::SpeechRecognizer> >::~unique_ptr() + 21 (memory:2577) 10 com.apple.WebKit 0x0000000105d84b07 WebKit::SpeechRecognitionServer::~SpeechRecognitionServer() + 103 (SpeechRecognitionServer.h:52) 11 com.apple.WebKit 0x0000000105d84a25 WebKit::SpeechRecognitionServer::~SpeechRecognitionServer() + 21 (SpeechRecognitionServer.h:52) 12 com.apple.WebKit 0x0000000105d84a4c WebKit::SpeechRecognitionServer::~SpeechRecognitionServer() + 28 (SpeechRecognitionServer.h:52) 13 com.apple.WebKit 0x0000000106ae208f std::__1::default_delete<WebKit::SpeechRecognitionServer>::operator()(WebKit::SpeechRecognitionServer*) const + 47 (memory:2368) 14 com.apple.WebKit 0x0000000106ae204f std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >::reset(WebKit::SpeechRecognitionServer*) + 95 (memory:2623) 15 com.apple.WebKit 0x0000000106ae20b9 std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >::~unique_ptr() + 25 (memory:2577) 16 com.apple.WebKit 0x0000000106a6b2a5 std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >::~unique_ptr() + 21 (memory:2577) 17 com.apple.WebKit 0x0000000106a7f04e WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > >::~KeyValuePair() + 30 (KeyValuePair.h:33) 18 com.apple.WebKit 0x0000000106a7f015 WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > >::~KeyValuePair() + 21 (KeyValuePair.h:33) 19 com.apple.WebKit 0x0000000106a7efa1 WTF::HashTable<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashMap<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >::KeyValuePairTraits, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> > >::deallocateTable(WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > >*) + 97 (HashTable.h:1241) 20 com.apple.WebKit 0x0000000106a7ee66 WTF::HashTable<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashMap<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >::KeyValuePairTraits, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> > >::~HashTable() + 54 (HashTable.h:414) 21 com.apple.WebKit 0x0000000106a7ee25 WTF::HashTable<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashMap<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >::KeyValuePairTraits, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> > >::~HashTable() + 21 (HashTable.h:411) 22 com.apple.WebKit 0x0000000106a7ee05 WTF::HashMap<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >::~HashMap() + 21 (HashMap.h:35) 23 com.apple.WebKit 0x0000000106a5fad5 WTF::HashMap<WTF::ObjectIdentifier<WebCore::PageIdentifierType>, std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> >, WTF::DefaultHash<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WTF::HashTraits<std::__1::unique_ptr<WebKit::SpeechRecognitionServer, std::__1::default_delete<WebKit::SpeechRecognitionServer> > > >::~HashMap() + 21 (HashMap.h:35) 24 com.apple.WebKit 0x0000000106a5f354 WebKit::WebProcessProxy::~WebProcessProxy() + 1220 (WebProcessProxy.cpp:267) 25 com.apple.WebKit 0x0000000106a5fd55 WebKit::WebProcessProxy::~WebProcessProxy() + 21 (WebProcessProxy.cpp:230) 26 com.apple.WebKit 0x0000000106a5fddc WebKit::WebProcessProxy::~WebProcessProxy() + 28 (WebProcessProxy.cpp:230) Created attachment 419366 [details]
Patch
Comment on attachment 419366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419366&action=review > Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:98 > + return; How about storing in the lambda clientIdentifier?. Then we can simply do: if (update.clientIdentifier() != clientIdentifier). Also, do we really need to call sendUpdate before this check? It seems like we should not need to send updates if speech recognition server is no longer using the same recogniser. I would also be tempted to do something like: if (m_recognizer) { m_recognizer->abort(WebCore::SpeechRecognitionError { WebCore::SpeechRecognitionErrorType::Aborted, "Another request is started"_s }); m_recognizer = nullptr; } Also, looking at SpeechRecognizer::abort and SpeechRecognizer::stop, I do not really understand why we are going to Aborting and then waiting to send the End update in destructor. It seems it would be better to not send any update in the destructor. Couldn't we do something like: void SpeechRecognizer::abort(Optional<SpeechRecognitionError>&& error) { if (m_state == State::Aborting || m_state == State::Inactive) return; // Do we even need this one. m_state = State::Aborting; if (error) m_delegateCallback(SpeechRecognitionUpdate::createError(clientIdentifier(), *error)); stopCapture(); abortRecognition(); m_delegateCallback(SpeechRecognitionUpdate::create(clientIdentifier(), SpeechRecognitionUpdateType::End)); m_state = State::Inactive; } Ditto for stop. Comment on attachment 419366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419366&action=review >> Source/WebKit/UIProcess/SpeechRecognitionServer.cpp:98 >> + return; > > How about storing in the lambda clientIdentifier?. > Then we can simply do: > if (update.clientIdentifier() != clientIdentifier). > > Also, do we really need to call sendUpdate before this check? > It seems like we should not need to send updates if speech recognition server is no longer using the same recogniser. > > I would also be tempted to do something like: > if (m_recognizer) { > m_recognizer->abort(WebCore::SpeechRecognitionError { WebCore::SpeechRecognitionErrorType::Aborted, "Another request is started"_s }); > m_recognizer = nullptr; > } > > Also, looking at SpeechRecognizer::abort and SpeechRecognizer::stop, I do not really understand why we are going to Aborting and then waiting to send the End update in destructor. > It seems it would be better to not send any update in the destructor. > > Couldn't we do something like: > > void SpeechRecognizer::abort(Optional<SpeechRecognitionError>&& error) > { > if (m_state == State::Aborting || m_state == State::Inactive) > return; > // Do we even need this one. > m_state = State::Aborting; > > if (error) > m_delegateCallback(SpeechRecognitionUpdate::createError(clientIdentifier(), *error)); > > stopCapture(); > abortRecognition(); > m_delegateCallback(SpeechRecognitionUpdate::create(clientIdentifier(), SpeechRecognitionUpdateType::End)); > m_state = State::Inactive; > } > > Ditto for stop. Here's my thoughts: Why do we send end update in the destructor? 1) stop and abort are async: in WebSpeechRecognizerTask, we send end update when we receive SFSpeechRecognitionTaskDelegate callback. 2) we ditch current request/recognizer immediately after abort(), only if the current request is aborted because a new request is about to start (in our current implementation). 3) if recognizer is destroyed before end update and start update is sent before, we need to make sure an end update is sent to inform client the request ends so client can update state. Why do we setting State::Aborting for abort? 0) abort is async, so after abort recognizer may still be active(sending updates) until end (becoming inactive) 1) we abort the request when there is an error occurred and there can be error occurred during abort Created attachment 419438 [details]
Patch
> Why do we setting State::Aborting for abort?
> 0) abort is async, so after abort recognizer may still be active(sending
> updates) until end (becoming inactive)
> 1) we abort the request when there is an error occurred and there can be
> error occurred during abort
The typical way to do this might to add completion handlers to stop and abort.
Since that would be the last update, the completion handlers could potentially take the recogniser update callback.
Created attachment 419477 [details]
Patch for landing
Committed r272451: <https://trac.webkit.org/changeset/272451> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419477 [details]. |