Bug 197761 - AX: Crash at WebKit: WebKit::WebSpeechSynthesisClient::speak
Summary: AX: Crash at WebKit: WebKit::WebSpeechSynthesisClient::speak
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-09 16:57 PDT by chris fleizach
Modified: 2019-05-11 20:22 PDT (History)
4 users (show)

See Also:


Attachments
patch (4.65 KB, patch)
2019-05-10 00:02 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-05-09 16:57:36 PDT
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*,
Comment 1 chris fleizach 2019-05-09 16:57:48 PDT
<rdar://problem/50237614>
Comment 2 Radar WebKit Bug Importer 2019-05-09 16:57:52 PDT
<rdar://problem/50644604>
Comment 3 chris fleizach 2019-05-10 00:02:30 PDT
Created attachment 369546 [details]
patch
Comment 4 Per Arne Vollan 2019-05-10 09:47:42 PDT
Comment on attachment 369546 [details]
patch

Nice! R=me.
Comment 5 WebKit Commit Bot 2019-05-10 10:19:31 PDT
Comment on attachment 369546 [details]
patch

Clearing flags on attachment: 369546

Committed r245178: <https://trac.webkit.org/changeset/245178>
Comment 6 WebKit Commit Bot 2019-05-10 10:19:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Daniel Bates 2019-05-10 20:14:29 PDT
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 "".
Comment 8 Daniel Bates 2019-05-10 20:15:15 PDT
(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...
Comment 9 chris fleizach 2019-05-11 09:10:02 PDT
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(),...
Comment 10 Daniel Bates 2019-05-11 09:40:31 PDT
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.
Comment 11 Daniel Bates 2019-05-11 09:43:43 PDT
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
Comment 12 Daniel Bates 2019-05-11 09:43:45 PDT
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
Comment 13 chris fleizach 2019-05-11 15:47:34 PDT
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()
Comment 14 Daniel Bates 2019-05-11 20:22:44 PDT
(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()