Bug 197761 – AX: Crash at WebKit: WebKit::WebSpeechSynthesisClient::speak

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   WebKit                        	0x00000001a45de6a0 WebKit::WebSpeechSynthesisClient::speak(WTF::RefPtr<WebCore::PlatformSpeechSynthesisUtterance, WTF::DumbPtrTraits<WebCore::PlatformSpeechSynthesisUtterance> >) + 128 (PlatformSpeechSynthesisVoice.h:48)
1   WebKit                        	0x00000001a45de65c WebKit::WebSpeechSynthesisClient::speak(WTF::RefPtr<WebCore::PlatformSpeechSynthesisUtterance, WTF::DumbPtrTraits<WebCore::PlatformSpeechSynthesisUtterance> >) + 60 (Function.h:84)
2   WebCore                       	0x00000001b0e2641c WebCore::SpeechSynthesis::startSpeakingImmediately(WebCore::SpeechSynthesisUtterance&) + 124 (SpeechSynthesis.cpp:125)
3   WebCore                       	0x00000001b0b4a818 WebCore::jsSpeechSynthesisPrototypeFunctionSpeakBody(JSC::ExecState*, WebCore::JSSpeechSynthesis*,

[chris fleizach]

<rdar://problem/50237614>

[Radar WebKit Bug Importer]

<rdar://problem/50644604>

[chris fleizach]

Created attachment 369546 [details]
patch

[Per Arne Vollan]

Comment on attachment 369546 [details]
patch

Nice! R=me.

[WebKit Commit Bot]

Comment on attachment 369546 [details]
patch

Clearing flags on attachment: 369546

Committed r245178: <https://trac.webkit.org/changeset/245178>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 369546 [details]
patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61
> +    auto voiceURI = voice ? voice->voiceURI() : "";

Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "".

[Daniel Bates]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 369546 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369546&action=review
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61
> > +    auto voiceURI = voice ? voice->voiceURI() : "";
> 
> Ok as is. Could be made a tiny bit more optimal (especially for dumb
> compiler) by hoisting the it condition into one of condition with an else
> branch. Are the string WTF string? Or const char? If the former, you can
> probably tighten it up a teensy bit more by using the null string instead of
> empty string BUT only do this if it's correct to do so. Otherwise, use
> emptyString() instead of "".

Hoist the if condition into one if condition...

[chris fleizach]

Comment on attachment 369546 [details]
patch

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

>>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61
>>> +    auto voiceURI = voice ? voice->voiceURI() : "";
>> 
>> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "".
> 
> Hoist the if condition into one if condition...

will work on this when I get a chance. it means I'll have to have two lines of the form

if ()
   m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ...
else
  m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),...

[Daniel Bates]

Comment on attachment 369546 [details]
patch

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

>>>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61
>>>> +    auto voiceURI = voice ? voice->voiceURI() : "";
>>> 
>>> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "".
>> 
>> Hoist the if condition into one if condition...
> 
> will work on this when I get a chance. it means I'll have to have two lines of the form
> 
> if ()
>    m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ...
> else
>   m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),...

I think our wires are crossed.

Look at the lines you touched, they have “ voice ?” In the right hand side <—- extract that to avoid repetition.

[Daniel Bates]

Comment on attachment 369546 [details]
patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:60
>      auto voice = utterance->voice();

Just looked up and saw this, left hand side ok as is. In my opinion auto* nicer and move this expression into the if () if you take my earlier advice so that we scope this var

[Daniel Bates]

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

> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:60
>      auto voice = utterance->voice();

Just looked up and saw this, left hand side ok as is. In my opinion auto* nicer and move this expression into the if () if you take my earlier advice so that we scope this var

[chris fleizach]

Comment on attachment 369546 [details]
patch

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

>>>>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61
>>>>> +    auto voiceURI = voice ? voice->voiceURI() : "";
>>>> 
>>>> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "".
>>> 
>>> Hoist the if condition into one if condition...
>> 
>> will work on this when I get a chance. it means I'll have to have two lines of the form
>> 
>> if ()
>>    m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ...
>> else
>>   m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),...
> 
> I think our wires are crossed.
> 
> Look at the lines you touched, they have “ voice ?” In the right hand side <—- extract that to avoid repetition.

you mean like 

auto name = "";
if (voice) {
   name = voice->name()

[Daniel Bates]

(In reply to chris fleizach from comment #13)
> Comment on attachment 369546 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369546&action=review
> 
> >>>>> Source/WebKit/WebProcess/WebCoreSupport/WebSpeechSynthesisClient.cpp:61
> >>>>> +    auto voiceURI = voice ? voice->voiceURI() : "";
> >>>> 
> >>>> Ok as is. Could be made a tiny bit more optimal (especially for dumb compiler) by hoisting the it condition into one of condition with an else branch. Are the string WTF string? Or const char? If the former, you can probably tighten it up a teensy bit more by using the null string instead of empty string BUT only do this if it's correct to do so. Otherwise, use emptyString() instead of "".
> >>> 
> >>> Hoist the if condition into one if condition...
> >> 
> >> will work on this when I get a chance. it means I'll have to have two lines of the form
> >> 
> >> if ()
> >>    m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(), ...
> >> else
> >>   m_page.sendWithAsyncReply(Messages::WebPageProxy::SpeechSynthesisSpeak(utterance->text(), utterance->lang(),...
> > 
> > I think our wires are crossed.
> > 
> > Look at the lines you touched, they have “ voice ?” In the right hand side <—- extract that to avoid repetition.
> 
> you mean like 

Almost except I suggest just declaring the vars, initialize inside of or else branch  for chance at optimal assembly


const char* name;

if (auto* voice = ...) {
    name = voice->name();
     ...
} else {
    name = “”;
     ...
}

One question:

> 
> auto name = "";

^—— so lhs is const char* ? If that is a mistake and you meant lhs to be String then second question: can we use null string? If not then change “” to emptyString() in **my** code above.


> if (voice) {
>    name = voice->name()