Bug 199988 – AX: CrashTracer: com.apple.WebKit.WebContent at WebKit: WebKit::WebSpeechSynthesisClient::speak

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

50 WebKit: WebKit::WebSpeechSynthesisClient::speak(WTF::RefPtr<WebCore::PlatformSpeechSynthesisUtterance, WTF::DumbPtrTraits<WebCore::PlatformSpeechSynthesisUtterance> >) <==
        49 WebKit: WebKit::WebSpeechSynthesisClient::speak(WTF::RefPtr<WebCore::PlatformSpeechSynthesisUtterance, WTF::DumbPtrTraits<WebCore::PlatformSpeechSynthesisUtterance> >)
        | 49 WebCore: WebCore::SpeechSynthesis::startSpeakingImmediately(WebCore::SpeechSynthesisUtterance&)
        |   26 WebCore: WebCore::jsSpeechSynthesisPrototypeFunctionSpeakBody(JSC::ExecState*, WebCore::JSSpeechSynthesis*, JSC::ThrowScope&)

<rdar://problem/53235469>

[chris fleizach]

One issue I don't know how to work around... the didFinish completion handler.. if you close a window while it's talking, obviously that handler won't be fired, but in debug we assert if the handlers are ever called

ASSERTION FAILED: Completion handler should always be called
!m_function
/Volumes/data/web/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/CompletionHandler.h(55) : WTF::CompletionHandler<void ()>::~CompletionHandler()
1   0x11162c1b9 WTFCrash
2   0x1173ab933 WTF::CompletionHandler<void ()>::~CompletionHandler()
3   0x1173a9fe5 WTF::CompletionHandler<void ()>::~CompletionHandler()
4   0x117d247e3 WebKit::WebPageProxy::SpeechSynthesisData::~SpeechSynthesisData()
5   0x117d017b5 WebKit::WebPageProxy::SpeechSynthesisData::~SpeechSynthesisData()
6   0x117d1024d WTF::Optional_base<WebKit::WebPageProxy::SpeechSynthesisData>::~Optional_base()

[chris fleizach]

Created attachment 374573 [details]
patch

[chris fleizach]

Created attachment 374574 [details]
patch

[chris fleizach]

Created attachment 374575 [details]
patch

[Per Arne Vollan]

Comment on attachment 374575 [details]
patch

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

> Source/WebCore/platform/PlatformSpeechSynthesizer.h:76
> +    virtual void resetState();

If this is overriding a virtual method, should we use the 'override' keyword instead of 'virtual'?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:261
> +    m_speechSynthesisData->speakingStartedCompletionHandler();

Is it possible for 'm_speechSynthesisData' to be null?

> Source/WebKit/UIProcess/WebPageProxy.cpp:9144
> +    synthesisData.speakingFinishedCompletionHandler = nullptr;

Maybe just use 'm_speechSynthesisData' here?

> Source/WebKit/UIProcess/WebPageProxy.cpp:9148
> +    synthesisData.synthesizer->resetState();

Is it possible for the 'synthesizer' member to be null?

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:82
> +    m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSetFinishedCallback(), WTFMove(finishedCompletionHandler));
> +    m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), utterance->volume(), utterance->rate(), utterance->pitch(), utterance->startTime(), voiceURI, name, lang, localService, isDefault), WTFMove(startedCompletionHandler));

Instead of creating a new message for setting the finished callback, could we add the completion handler as a parameter in the 'SpeechSynthesisSpeak' message? Also, didn't the WebProcess get a message from the UI process when speaking had finished, previously?

[chris fleizach]

Comment on attachment 374575 [details]
patch

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

>> Source/WebCore/platform/PlatformSpeechSynthesizer.h:76
>> +    virtual void resetState();
> 
> If this is overriding a virtual method, should we use the 'override' keyword instead of 'virtual'?

yes

>> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:261
>> +    m_speechSynthesisData->speakingStartedCompletionHandler();
> 
> Is it possible for 'm_speechSynthesisData' to be null?

it is possible that its null, but not expected. it would only be null if no speech job was ever started. however we wouldn't get these callbacks in that case.

perhaps we should call speechSynthesisData() in these callbacks instead, which ensures its created

>> Source/WebKit/UIProcess/WebPageProxy.cpp:9148
>> +    synthesisData.synthesizer->resetState();
> 
> Is it possible for the 'synthesizer' member to be null?

it shouldn't be, but we should check

>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:82
>> +    m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), utterance->volume(), utterance->rate(), utterance->pitch(), utterance->startTime(), voiceURI, name, lang, localService, isDefault), WTFMove(startedCompletionHandler));
> 
> Instead of creating a new message for setting the finished callback, could we add the completion handler as a parameter in the 'SpeechSynthesisSpeak' message? Also, didn't the WebProcess get a message from the UI process when speaking had finished, previously?

it did not seem possible with the format of this message passing API possible to have two asynchronous callbacks.

previously we passed in the didFinish callback to startSpeaking. but then we weren't getting the "real" start speaking callback. so I changed the startSpeaking method to take the start speaking callback

[chris fleizach]

Created attachment 374633 [details]
patch

[chris fleizach]

Created attachment 374634 [details]
patch

[Per Arne Vollan]

Comment on attachment 374634 [details]
patch

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

R=me.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:261
> +    speechSynthesisData().speakingStartedCompletionHandler();

Do we need to guard against the completion handler not being set?

[chris fleizach]

Comment on attachment 374634 [details]
patch

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

>> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:261
>> +    speechSynthesisData().speakingStartedCompletionHandler();
> 
> Do we need to guard against the completion handler not being set?

we probably have the issue for all of them. we should probably check

[chris fleizach]

Created attachment 374656 [details]
patch

[chris fleizach]

trying cq again

[WebKit Commit Bot]

Comment on attachment 374656 [details]
patch

Clearing flags on attachment: 374656

Committed r247755: <https://trac.webkit.org/changeset/247755>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.