Bug 221451 – REGRESSION(r272337): crash under WebCore::SpeechRecognizer::setInactive()

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

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)

[Sihui Liu]

Created attachment 419366 [details]
Patch

[youenn fablet]

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.

[Sihui Liu]

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

[Sihui Liu]

Created attachment 419438 [details]
Patch

[youenn fablet]

> 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.

[Sihui Liu]

Created attachment 419477 [details]
Patch for landing

[EWS]

Committed r272451: <https://trac.webkit.org/changeset/272451>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419477 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74047313>