Fix crash in NSSpeechSynthesizer by explicitly retaining NSString. (issue 12636005)

[dmaz...@chromium.org]

Reviewers: David Tseng, Nico,

Message:
dtseng: review speech logic
thakis: please check Obj-C style and idiosyncratic usage


Description:
Fix crash in NSSpeechSynthesizer by explicitly retaining NSString.

This definitely fixes one crash that was easy to repro by
speaking text at a high rate (~900 wpm) with the Samantha
voice on OS X 10.8.2. Retaining the NSString passed to
startSpeakingString fixes the problem, so this change
makes a one-time-use subclass of NSSpeechSynthesizer that
holds onto its utterance string while it's alive.

BUG=99116


Please review this at https://codereview.chromium.org/12636005/

SVN Base: http://git.chromium.org/chromium/src.git@master

Affected files:
M chrome/browser/speech/tts_mac.mm


Index: chrome/browser/speech/tts_mac.mm
diff --git a/chrome/browser/speech/tts_mac.mm
b/chrome/browser/speech/tts_mac.mm
index
44f5ed3419d5c258fd1e449184c4f5a513f82e24..ce02ebc9f2dc700c2ed2c2b3cce2e3b4eac2c1b0
100644
--- a/chrome/browser/speech/tts_mac.mm
+++ b/chrome/browser/speech/tts_mac.mm
@@ -25,6 +25,18 @@ class TtsPlatformImplMac;

@end

+@interface SingleUseSpeechSynthesizer : NSSpeechSynthesizer {
+ @private
+ scoped_nsobject<NSString> utterance_;
+ bool didSpeak_;
+}
+
+- (id)initWithUtterance:(NSString*)utterance;
+- (bool)startSpeakingRetainedUtterance;
+- (bool)startSpeakingString:(NSString*)utterance;
+
+@end
+
class TtsPlatformImplMac : public TtsPlatformImpl {
public:
virtual bool PlatformImplAvailable() OVERRIDE {
@@ -57,7 +69,7 @@ class TtsPlatformImplMac : public TtsPlatformImpl {
TtsPlatformImplMac();
virtual ~TtsPlatformImplMac();

- scoped_nsobject<NSSpeechSynthesizer> speech_synthesizer_;
+ scoped_nsobject<SingleUseSpeechSynthesizer> speech_synthesizer_;
scoped_nsobject<ChromeTtsDelegate> delegate_;
int utterance_id_;
std::string utterance_;
@@ -78,20 +90,25 @@ bool TtsPlatformImplMac::Speak(
const std::string& utterance,
const std::string& lang,
const UtteranceContinuousParameters& params) {
+ // TODO: convert SSML to SAPI xml. http://crbug.com/88072
+ utterance_ = utterance;
+
+ NSString* utterance_nsstring =
+ [NSString stringWithUTF8String: utterance_.c_str()];
+
// Deliberately construct a new speech synthesizer every time Speak is
// called, otherwise there's no way to know whether calls to the delegate
// apply to the current utterance or a previous utterance. In
// experimentation, the overhead of constructing and destructing a
// NSSpeechSynthesizer is minimal.
- speech_synthesizer_.reset([[NSSpeechSynthesizer alloc] init]);
+ speech_synthesizer_.reset(
+ [[SingleUseSpeechSynthesizer alloc]
+ initWithUtterance:utterance_nsstring]);
[speech_synthesizer_ setDelegate:delegate_];

utterance_id_ = utterance_id;
sent_start_event_ = false;

- // TODO: convert SSML to SAPI xml. http://crbug.com/88072
- utterance_ = utterance;
-
// TODO: support languages other than the default: crbug.com/88059

if (params.rate >= 0.0) {
@@ -114,8 +131,7 @@ bool TtsPlatformImplMac::Speak(
forProperty:NSSpeechVolumeProperty error:nil];
}

- return [speech_synthesizer_ startSpeakingString:
- [NSString stringWithUTF8String: utterance.c_str()]];
+ return [speech_synthesizer_ startSpeakingRetainedUtterance];
}

bool TtsPlatformImplMac::StopSpeaking() {
@@ -206,3 +222,28 @@ TtsPlatformImplMac* TtsPlatformImplMac::GetInstance() {
}

@end
+
+@implementation SingleUseSpeechSynthesizer
+
+- (id)initWithUtterance:(NSString*)utterance {
+ self = [super init];
+ if (self) {
+ utterance_.reset([utterance retain]);
+ didSpeak_ = false;
+ }
+ return self;
+}
+
+- (bool)startSpeakingRetainedUtterance {
+ CHECK(!didSpeak_);
+ CHECK(utterance_);
+ didSpeak_ = true;
+ return [super startSpeakingString:utterance_];
+}
+
+- (bool)startSpeakingString:(NSString*)utterance {
+ CHECK(false);
+ return false;
+}
+
+@end

[tha...@chromium.org]

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm
File chrome/browser/speech/tts_mac.mm (right):

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm#newcode28
chrome/browser/speech/tts_mac.mm:28: @interface
SingleUseSpeechSynthesizer : NSSpeechSynthesizer {
Can you add a comment explaining why this exists?

Also, can you file a bug for this (ideally with a small, self-contained
repro) at bugreporter.apple.com – NSSpeechSynthesizer should be retaiing
the startSpeakingString: argument, and if it doesn't that's a bug.
(After you've filed, please copy the bug report to openradar.appspot.com
and include a link to that in the class comment)

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm#newcode97
chrome/browser/speech/tts_mac.mm:97: [NSString stringWithUTF8String:
utterance_.c_str()];
No space after ':'

(btw, clang_format can format objc)

https://codereview.chromium.org/12636005/

[dts...@chromium.org]

lgtm

https://codereview.chromium.org/12636005/

[dmaz...@chromium.org]

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm
File chrome/browser/speech/tts_mac.mm (right):

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm#newcode28
chrome/browser/speech/tts_mac.mm:28: @interface
SingleUseSpeechSynthesizer : NSSpeechSynthesizer {

On 2013/03/14 22:45:17, Nico wrote:
> Can you add a comment explaining why this exists?

> Also, can you file a bug for this (ideally with a small,
self-contained repro)

> at http://bugreporter.apple.com –&nbsp;NSSpeechSynthesizer should be

retaiing the
> startSpeakingString: argument, and if it doesn't that's a bug. (After
you've

> filed, please copy the bug report to http://openradar.appspot.com and

include a link to
> that in the class comment)

Done.

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm#newcode97
chrome/browser/speech/tts_mac.mm:97: [NSString stringWithUTF8String:
utterance_.c_str()];

On 2013/03/14 22:45:17, Nico wrote:
> No space after ':'

> (btw, clang_format can format objc)

Done.

https://codereview.chromium.org/12636005/

[tha...@chromium.org]

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm
File chrome/browser/speech/tts_mac.mm (right):

https://codereview.chromium.org/12636005/diff/1/chrome/browser/speech/tts_mac.mm#newcode28
chrome/browser/speech/tts_mac.mm:28: @interface
SingleUseSpeechSynthesizer : NSSpeechSynthesizer {
On 2013/03/14 22:45:17, Nico wrote:
> Can you add a comment explaining why this exists?

> Also, can you file a bug for this (ideally with a small,
self-contained repro)
> at http://bugreporter.apple.com –&nbsp;NSSpeechSynthesizer should be
retaiing the
> startSpeakingString: argument, and if it doesn't that's a bug. (After
you've
> filed, please copy the bug report to http://openradar.appspot.com and
include a link to
> that in the class comment)

I meant a link to the openradar one, rdar:// links can only be read by
apple people :-)

https://codereview.chromium.org/12636005/

[dmaz...@chromium.org]

On 2013/03/14 23:32:00, Nico wrote:
> I meant a link to the openradar one, rdar:// links can only be read by
> apple
> people :-)

Sorry! Done.


https://codereview.chromium.org/12636005/

[tha...@chromium.org]

lgtm

Thanks!

https://codereview.chromium.org/12636005/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmaz...@chromium.org/12636005/7002


https://chromiumcodereview.appspot.com/12636005/

[commi...@chromium.org]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=109270

https://chromiumcodereview.appspot.com/12636005/

[commi...@chromium.org]

[commi...@chromium.org]

Change committed as 188338

https://chromiumcodereview.appspot.com/12636005/