Bug 199988 - AX: CrashTracer: com.apple.WebKit.WebContent at WebKit: WebKit::WebSpeechSynthesisClient::speak
Summary: AX: CrashTracer: com.apple.WebKit.WebContent at WebKit: WebKit::WebSpeechSynt...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-21 03:01 PDT by chris fleizach
Modified: 2019-07-23 16:25 PDT (History)
5 users (show)

See Also:


Attachments
patch (2.85 KB, patch)
2019-07-21 12:32 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (13.71 KB, patch)
2019-07-21 12:35 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (13.77 KB, patch)
2019-07-21 12:43 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (5.72 KB, patch)
2019-07-22 14:21 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (14.44 KB, patch)
2019-07-22 14:22 PDT, chris fleizach
pvollan: review+
Details | Formatted Diff | Diff
patch (14.71 KB, patch)
2019-07-22 17:01 PDT, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2019-07-21 03:01:55 PDT
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>
Comment 1 chris fleizach 2019-07-21 03:03:10 PDT
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()
Comment 2 chris fleizach 2019-07-21 12:32:05 PDT
Created attachment 374573 [details]
patch
Comment 3 chris fleizach 2019-07-21 12:35:58 PDT
Created attachment 374574 [details]
patch
Comment 4 chris fleizach 2019-07-21 12:43:05 PDT
Created attachment 374575 [details]
patch
Comment 5 Per Arne Vollan 2019-07-22 11:28:06 PDT
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?
Comment 6 chris fleizach 2019-07-22 11:34:50 PDT
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
Comment 7 chris fleizach 2019-07-22 14:21:10 PDT
Created attachment 374633 [details]
patch
Comment 8 chris fleizach 2019-07-22 14:22:08 PDT
Created attachment 374634 [details]
patch
Comment 9 Per Arne Vollan 2019-07-22 16:32:45 PDT
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?
Comment 10 chris fleizach 2019-07-22 16:33:59 PDT
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
Comment 11 chris fleizach 2019-07-22 17:01:35 PDT
Created attachment 374656 [details]
patch
Comment 12 chris fleizach 2019-07-23 13:39:06 PDT
trying cq again
Comment 13 WebKit Commit Bot 2019-07-23 16:25:04 PDT
Comment on attachment 374656 [details]
patch

Clearing flags on attachment: 374656

Committed r247755: <https://trac.webkit.org/changeset/247755>
Comment 14 WebKit Commit Bot 2019-07-23 16:25:06 PDT
All reviewed patches have been landed.  Closing bug.