197761 2019-05-09 16:57:36 -0700 AX: Crash at WebKit: WebKit::WebSpeechSynthesisClient::speak 2019-05-11 20:22:44 -0700 1 1 1 Unclassified WebKit Accessibility Other All All RESOLVED FIXED InRadar P2 Normal --- 1 cfleizach cfleizach commit-queue dbates pvollan webkit-bug-importer oldest_to_newest 1534900 0 cfleizach 2019-05-09 16:57:36 -0700 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*, 1534901 1 cfleizach 2019-05-09 16:57:48 -0700 <rdar://problem/50237614> 1534902 2 webkit-bug-importer 2019-05-09 16:57:52 -0700 <rdar://problem/50644604> 1534958 3 369546 cfleizach 2019-05-10 00:02:30 -0700 Created attachment 369546 patch 1535034 4 369546 pvollan 2019-05-10 09:47:42 -0700 Comment on attachment 369546 patch Nice! R=me. 1535057 5 369546 commit-queue 2019-05-10 10:19:31 -0700 Comment on attachment 369546 patch Clearing flags on attachment: 369546 Committed r245178: <https://trac.webkit.org/changeset/245178> 1535058 6 commit-queue 2019-05-10 10:19:32 -0700 All reviewed patches have been landed. Closing bug. 1535310 7 369546 dbates 2019-05-10 20:14:29 -0700 Comment on attachment 369546 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 "". 1535311 8 dbates 2019-05-10 20:15:15 -0700 (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... 1535350 9 369546 cfleizach 2019-05-11 09:10:02 -0700 Comment on attachment 369546 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(),... 1535352 10 369546 dbates 2019-05-11 09:40:31 -0700 Comment on attachment 369546 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. 1535353 11 369546 dbates 2019-05-11 09:43:43 -0700 Comment on attachment 369546 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 1535354 12 dbates 2019-05-11 09:43:45 -0700 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 1535390 13 369546 cfleizach 2019-05-11 15:47:34 -0700 Comment on attachment 369546 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() 1535418 14 dbates 2019-05-11 20:22:44 -0700 (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() 369546 2019-05-10 00:02:30 -0700 2019-05-10 10:19:31 -0700 patch patch text/plain 4765 cfleizach SW5kZXg6IFNvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQvQ2hhbmdlTG9nCShyZXZpc2lvbiAyNDUxNzEpCisrKyBTb3VyY2UvV2ViS2l0L0NoYW5nZUxv Zwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE5LTA1LTEwICBDaHJpcyBGbGVp emFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IENyYXNoIGF0IFdlYktp dDogV2ViS2l0OjpXZWJTcGVlY2hTeW50aGVzaXNDbGllbnQ6OnNwZWFrCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTc3NjEKKyAgICAgICAgPHJkYXI6 Ly9wcm9ibGVtLzUwMjM3NjE0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIEl0J3MgdmFsaWQgZm9yIHRoZSBpbmNvbWluZyB2b2ljZSB0byBiZSBuaWws IHNvIHdlIG5lZWQgdG8gcHJvdGVjdCBhZ2FpbnN0IHRoYXQuCisKKyAgICAgICAgVGVzdHM6IGZh c3Qvc3BlZWNoc3ludGhlc2lzL3NwZWVjaC1zeW50aGVzaXMtcmVhbC1jbGllbnQtdmVyc2lvbi5o dG1sCisKKyAgICAgICAgKiBXZWJQcm9jZXNzL1dlYkNvcmVTdXBwb3J0L1dlYlNwZWVjaFN5bnRo ZXNpc0NsaWVudC5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYlNwZWVjaFN5bnRoZXNpc0NsaWVu dDo6c3BlYWspOgorCiAyMDE5LTA1LTA5ICBSb2IgQnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CiAK ICAgICAgICAgTGltaXQgbnVtYmVyIG9mIHByZWZldGNoZXMgb2YgYSBnaXZlbiBwYWdlCkluZGV4 OiBTb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvV2ViU3BlZWNoU3ludGhl c2lzQ2xpZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2Vi Q29yZVN1cHBvcnQvV2ViU3BlZWNoU3ludGhlc2lzQ2xpZW50LmNwcAkocmV2aXNpb24gMjQ1MTQ3 KQorKysgU291cmNlL1dlYktpdC9XZWJQcm9jZXNzL1dlYkNvcmVTdXBwb3J0L1dlYlNwZWVjaFN5 bnRoZXNpc0NsaWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTU4LDggKzU4LDE0IEBACiAgICAg fTsKIAogICAgIGF1dG8gdm9pY2UgPSB1dHRlcmFuY2UtPnZvaWNlKCk7Ci0gICAgbV9wYWdlLnNl bmRXaXRoQXN5bmNSZXBseShNZXNzYWdlczo6V2ViUGFnZVByb3h5OjpTcGVlY2hTeW50aGVzaXNT cGVhayh1dHRlcmFuY2UtPnRleHQoKSwgdXR0ZXJhbmNlLT5sYW5nKCksIHV0dGVyYW5jZS0+dm9s dW1lKCksIHV0dGVyYW5jZS0+cmF0ZSgpLCB1dHRlcmFuY2UtPnBpdGNoKCksIHV0dGVyYW5jZS0+ c3RhcnRUaW1lKCksIHZvaWNlLT52b2ljZVVSSSgpLCB2b2ljZS0+bmFtZSgpLCB2b2ljZS0+bGFu ZygpLCB2b2ljZS0+bG9jYWxTZXJ2aWNlKCksIHZvaWNlLT5pc0RlZmF1bHQoKSksIFdURk1vdmUo Y29tcGxldGlvbkhhbmRsZXIpKTsKKyAgICBhdXRvIHZvaWNlVVJJID0gdm9pY2UgPyB2b2ljZS0+ dm9pY2VVUkkoKSA6ICIiOworICAgIGF1dG8gbmFtZSA9IHZvaWNlID8gdm9pY2UtPm5hbWUoKSA6 ICIiOworICAgIGF1dG8gbGFuZyA9IHZvaWNlID8gdm9pY2UtPmxhbmcoKSA6ICIiOworICAgIGF1 dG8gbG9jYWxTZXJ2aWNlID0gdm9pY2UgPyB2b2ljZS0+bG9jYWxTZXJ2aWNlKCkgOiBmYWxzZTsK KyAgICBhdXRvIGlzRGVmYXVsdCA9IHZvaWNlID8gdm9pY2UtPmlzRGVmYXVsdCgpIDogZmFsc2U7 CiAKKyAgICBtX3BhZ2Uuc2VuZFdpdGhBc3luY1JlcGx5KE1lc3NhZ2VzOjpXZWJQYWdlUHJveHk6 OlNwZWVjaFN5bnRoZXNpc1NwZWFrKHV0dGVyYW5jZS0+dGV4dCgpLCB1dHRlcmFuY2UtPmxhbmco KSwgdXR0ZXJhbmNlLT52b2x1bWUoKSwgdXR0ZXJhbmNlLT5yYXRlKCksIHV0dGVyYW5jZS0+cGl0 Y2goKSwgdXR0ZXJhbmNlLT5zdGFydFRpbWUoKSwgdm9pY2VVUkksIG5hbWUsIGxhbmcsIGxvY2Fs U2VydmljZSwgaXNEZWZhdWx0KSwgV1RGTW92ZShjb21wbGV0aW9uSGFuZGxlcikpOworCiAgICAg bV9wYWdlLmNvcmVQYWdlKCktPnNwZWVjaFN5bnRoZXNpc0NsaWVudCgpLT5vYnNlcnZlcigpLT5k aWRTdGFydFNwZWFraW5nKCk7CiB9CiAKSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDI0NTE0NykKKysrIExh eW91dFRlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE5 LTA1LTEwICBDaHJpcyBGbGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CisKKyAgICAgICAg QVg6IENyYXNoIGF0IFdlYktpdDogV2ViS2l0OjpXZWJTcGVlY2hTeW50aGVzaXNDbGllbnQ6OnNw ZWFrCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTc3 NjEKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzUwMjM3NjE0PgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogZmFzdC9zcGVlY2hzeW50aGVzaXMvc3Bl ZWNoLXN5bnRoZXNpcy1yZWFsLWNsaWVudC12ZXJzaW9uLWV4cGVjdGVkLnR4dDogQWRkZWQuCisg ICAgICAgICogZmFzdC9zcGVlY2hzeW50aGVzaXMvc3BlZWNoLXN5bnRoZXNpcy1yZWFsLWNsaWVu dC12ZXJzaW9uLmh0bWw6IEFkZGVkLgorCiAyMDE5LTA1LTA5ICBEYW5pZWwgQmF0ZXMgIDxkYWJh dGVzQGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TXSBVbmFibGUgdG8gY29tbWl0IHNlYXJjaCBv biBNU04uY29tLCBxcS5jb20sIG9yIHNpbmEuY29tLmNuIHVzaW5nIGVudGVyIGtleSAoaGFyZHdh cmUgb3Igc29mdHdhcmUga2V5Ym9hcmQpCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L3NwZWVjaHN5 bnRoZXNpcy9zcGVlY2gtc3ludGhlc2lzLXJlYWwtY2xpZW50LXZlcnNpb24tZXhwZWN0ZWQudHh0 Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3Qvc3BlZWNoc3ludGhlc2lzL3NwZWVjaC1z eW50aGVzaXMtcmVhbC1jbGllbnQtdmVyc2lvbi1leHBlY3RlZC50eHQJKG5vbmV4aXN0ZW50KQor KysgTGF5b3V0VGVzdHMvZmFzdC9zcGVlY2hzeW50aGVzaXMvc3BlZWNoLXN5bnRoZXNpcy1yZWFs LWNsaWVudC12ZXJzaW9uLWV4cGVjdGVkLnR4dAkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDEw IEBACitUaGlzIHRlc3RzIHRoYXQgd2UgY2FuIGRpc3BhdGNoIGEgc3BlZWNoIGpvYiB1c2luZyB0 aGUgcmVhbCBzeW50aGVzaXplciB3aXRob3V0IGNyYXNoaW5nLgorCitPbiBzdWNjZXNzLCB5b3Ug d2lsbCBzZWUgYSBzZXJpZXMgb2YgIlBBU1MiIG1lc3NhZ2VzLCBmb2xsb3dlZCBieSAiVEVTVCBD T01QTEVURSIuCisKKworUEFTUyBzcGVlY2hTeW50aGVzaXMuc3BlYWtpbmcgaXMgZmFsc2UKK1BB U1Mgc3VjY2Vzc2Z1bGx5UGFyc2VkIGlzIHRydWUKKworVEVTVCBDT01QTEVURQorCkluZGV4OiBM YXlvdXRUZXN0cy9mYXN0L3NwZWVjaHN5bnRoZXNpcy9zcGVlY2gtc3ludGhlc2lzLXJlYWwtY2xp ZW50LXZlcnNpb24uaHRtbAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0L3NwZWVjaHN5 bnRoZXNpcy9zcGVlY2gtc3ludGhlc2lzLXJlYWwtY2xpZW50LXZlcnNpb24uaHRtbAkobm9uZXhp c3RlbnQpCisrKyBMYXlvdXRUZXN0cy9mYXN0L3NwZWVjaHN5bnRoZXNpcy9zcGVlY2gtc3ludGhl c2lzLXJlYWwtY2xpZW50LXZlcnNpb24uaHRtbAkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDMz IEBACis8IURPQ1RZUEUgSFRNTCBQVUJMSUMgIi0vL0lFVEYvL0RURCBIVE1MLy9FTiI+Cis8aHRt bD4KKzxoZWFkPgorPHNjcmlwdCBzcmM9Ii4uLy4uL3Jlc291cmNlcy9qcy10ZXN0LXByZS5qcyI+ PC9zY3JpcHQ+Cis8L2hlYWQ+Cis8Ym9keSBpZD0iYm9keSI+CisKKzxkaXYgaWQ9ImNvbnNvbGUi PjwvZGl2PgorCis8c2NyaXB0PgorCisgICAgZGVzY3JpcHRpb24oIlRoaXMgdGVzdHMgdGhhdCB3 ZSBjYW4gZGlzcGF0Y2ggYSBzcGVlY2ggam9iIHVzaW5nIHRoZSByZWFsIHN5bnRoZXNpemVyIHdp dGhvdXQgY3Jhc2hpbmcuIik7CisKKyAgICBpZiAod2luZG93LnRlc3RSdW5uZXIpCisgICAgICAg IHRlc3RSdW5uZXIud2FpdFVudGlsRG9uZSgpOworCisgICAgd2luZG93LmpzVGVzdElzQXN5bmMg PSB0cnVlOworCisgICAgLy8gU3RhcnQgYSB2ZXJ5IHNob3J0IHNwZWFraW5nIGpvYiB0aGF0IHdp bGwgZmluaXNoIHF1aWNrbHkuCisgICAgdmFyIHUgPSBuZXcgU3BlZWNoU3ludGhlc2lzVXR0ZXJh bmNlKCJhIik7CisKKyAgICB1LmFkZEV2ZW50TGlzdGVuZXIoJ2VuZCcsIGZ1bmN0aW9uKGUpIHsg CisgICAgICAgZmluaXNoSlNUZXN0KCk7CisgICAgfSk7CisKKyAgICBzaG91bGRCZUZhbHNlKCJz cGVlY2hTeW50aGVzaXMuc3BlYWtpbmciKTsKKyAgICBzcGVlY2hTeW50aGVzaXMuc3BlYWsodSk7 CisKKzwvc2NyaXB0PgorCis8c2NyaXB0IHNyYz0iLi4vLi4vcmVzb3VyY2VzL2pzLXRlc3QtcG9z dC5qcyI+PC9zY3JpcHQ+Cis8L2JvZHk+Cis8L2h0bWw+Cg==