Fix crash if utterance is garbage-collected before speech ends.

Fix crash if utterance is garbage-collected before speech ends.

BUG=359130, 348863
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171077

[sof, 2014-04-08 08:16:04]

https://codereview.chromium.org/228133002/diff/1/Source/modules/speech/Speech...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/228133002/diff/1/Source/modules/speech/Speech...
Source/modules/speech/SpeechSynthesis.cpp:157: RefPtr<SpeechSynthesisUtterance>
saveFirstUtterance;
Wouldn't

   RefPtrWillBeRawPtr<SpeechSynthesisUtterance> protect(utterance);

be equivalent? i.e., protect the utterance across the event dispatch.

[dmazzoni, 2014-04-08 17:35:50]

On 2014/04/08 08:16:04, sof wrote:
>
https://codereview.chromium.org/228133002/diff/1/Source/modules/speech/Speech...
> File Source/modules/speech/SpeechSynthesis.cpp (right):
> 
>
https://codereview.chromium.org/228133002/diff/1/Source/modules/speech/Speech...
> Source/modules/speech/SpeechSynthesis.cpp:157:
RefPtr<SpeechSynthesisUtterance>
> saveFirstUtterance;
> Wouldn't
> 
>    RefPtrWillBeRawPtr<SpeechSynthesisUtterance> protect(utterance);
> 
> be equivalent? i.e., protect the utterance across the event dispatch.

Sure, done.

[sof, 2014-04-08 17:52:10]

On 2014/04/08 17:35:50, dmazzoni wrote:
> On 2014/04/08 08:16:04, sof wrote:
> >
>
https://codereview.chromium.org/228133002/diff/1/Source/modules/speech/Speech...
> > File Source/modules/speech/SpeechSynthesis.cpp (right):
> > 
> >
>
https://codereview.chromium.org/228133002/diff/1/Source/modules/speech/Speech...
> > Source/modules/speech/SpeechSynthesis.cpp:157:
> RefPtr<SpeechSynthesisUtterance>
> > saveFirstUtterance;
> > Wouldn't
> > 
> >    RefPtrWillBeRawPtr<SpeechSynthesisUtterance> protect(utterance);
> > 
> > be equivalent? i.e., protect the utterance across the event dispatch.
> 
> Sure, done.

Thanks, looks safe to me now - abarth to approve? (I'm assuming there is a good
reason why the event is fired after the queue removal.)

[dmazzoni, 2014-04-08 17:54:11]

On 2014/04/08 17:52:10, sof wrote:
> Thanks, looks safe to me now - abarth to approve? (I'm assuming there is a
good
> reason why the event is fired after the queue removal.)

Yes - because when the 'end' event fires, speechSynthesis.speaking should return
false, but it wouldn't if the utterance was still in the queue. That's caught by
a test.

[eseidel, 2014-04-08 17:56:33]

https://codereview.chromium.org/228133002/diff/20001/Source/modules/speech/Sp...
File Source/modules/speech/SpeechSynthesis.cpp (right):

https://codereview.chromium.org/228133002/diff/20001/Source/modules/speech/Sp...
Source/modules/speech/SpeechSynthesis.cpp:161: m_utteranceQueue.removeFirst();
How did this work before if "utterance" can be deleted here?

Seems like a fine fix, but the previous code looks obviously wrong?  I wonder if
we shouldn't name "protect" somehow differnetly to be explicit that "utterance"
could be the first in the queue.

[dmazzoni, 2014-04-08 18:02:58]

On Tue, Apr 8, 2014 at 10:56 AM, <eseidel@chromium.org> wrote:

>
> https://codereview.chromium.org/228133002/diff/20001/
> Source/modules/speech/SpeechSynthesis.cpp
> File Source/modules/speech/SpeechSynthesis.cpp (right):
>
> https://codereview.chromium.org/228133002/diff/20001/
> Source/modules/speech/SpeechSynthesis.cpp#newcode161
> Source/modules/speech/SpeechSynthesis.cpp:161:
> m_utteranceQueue.removeFirst();
> How did this work before if "utterance" can be deleted here?
>

Totally my fault. I broke it while fixing a previous bug. It didn't show up
in my own tests because the SpeechSynthesisUtterance existed in JavaScript
and didn't get garbage-collected until later when it went out of scope. See
the test I attached - I couldn't repro it in a test without explicitly
calling gc(). (Not saying it couldn't repro, just that it wasn't obvious.)


> Seems like a fine fix, but the previous code looks obviously wrong?  I
> wonder if we shouldn't name "protect" somehow differnetly to be explicit
> that "utterance" could be the first in the queue.
>

My first patch tried to make it more clear that we're protecting the first
item in the queue. Which do you prefer?


> https://codereview.chromium.org/228133002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[eseidel, 2014-04-08 18:04:59]

lgtm.  I would just add a comment to the protector.  something like:

// m_uteranceQueue may be holding the last refernece to "utterance", or
something.

[dmazzoni, 2014-04-08 18:15:11]

On 2014/04/08 18:04:59, eseidel wrote:
> lgtm.  I would just add a comment to the protector.  something like:
> 
> // m_uteranceQueue may be holding the last refernece to "utterance", or
> something.

Comment added, thanks.

[dmazzoni, 2014-04-08 18:15:20]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-04-08 18:15:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/228133002/30001

[commit-bot: I haz the power, 2014-04-08 19:21:36]

Change committed as 171077