Debug: Assertion failure: aIndex < Length() (invalid array index), at nsTArray.h:984 (Called by mozilla::dom::SpeechSynthesis::Pause) ASan: global-buffer-overflow past nsTArrayHeader::sEmptyHdr [@ mozilla::dom::SpeechSynthesis::Pause] Release: Does not crash for me

So Pause() does: 199 if (mCurrentTask && 200 mSpeechQueue.ElementAt(0)->GetState() != SpeechSynthesisUtterance::STATE_ENDED) { Sounds like mSpeechQueue is empty?

I was trying to look at the attachment, and the browser kept freezing. doh!

It sounds like it isn't an unbounded read, but better safe than sorry, so I'm going to mark it sec-high.

Comment on attachment 8688703 [details] [diff] [review] Check that mSpeechQueue is not empty before referencing first element. oops. Looks like we do check IsEmpty in other cases in that file. But, should we do something to the mCurrentTask? Like, should SpeechSynthesis::Cancel() set mCurrentTask to nullptr after mCurrentTask->Cancel() call or something? Though, I guess OnEnd's MOZ_ASSERT(mCurrentTask == aTask); might fire then. However, we're missing to clear mCurrentTask somewhere, right? Hmm, do we end up using InitIndirectAudio() so nsSpeechTask::Cancel() never ends up triggering OnEnd() (under DispatchEndInner() ) ? Or am I missing something here?

(In reply to Olli Pettay [:smaug] from comment #7) > Comment on attachment 8688703 [details] [diff] [review] > Check that mSpeechQueue is not empty before referencing first element. > > oops. Looks like we do check IsEmpty in other cases in that file. > > But, should we do something to the mCurrentTask? > Like, should SpeechSynthesis::Cancel() set mCurrentTask to nullptr after > mCurrentTask->Cancel() call or something? Cancel() is only a request of the engine to end the utterance. The utterance/task should still be considered active until we get a call for OnEnd. > Though, I guess OnEnd's MOZ_ASSERT(mCurrentTask == aTask); might fire then. > However, we're missing to clear mCurrentTask somewhere, right? > Right, it is cleared on OnEnd. > Hmm, do we end up using InitIndirectAudio() so nsSpeechTask::Cancel() never > ends up triggering OnEnd() (under DispatchEndInner() ) ? > Indirect audio services must call task->DispatchEnd themselves after receiving a cancel request. > Or am I missing something here? Did I answer your question? Not sure I did..

So DispatchEnd may happen async and we may have cleared the array already at that point?

(In reply to Olli Pettay [:smaug] from comment #9) > So DispatchEnd may happen async and we may have cleared the array already at > that point? We clear the array on Cancel() (if it didn't start speaking yet), but we only clear mCurrentTask in OnEnd(). So in this case, Pause() is called in between those two calls where mCurrentTask still exists but the array is cleared.

(In reply to Olli Pettay [:smaug] from comment #9) > So DispatchEnd may happen async and we may have cleared the array already at > that point? The array is cleared immediately, DispatchEnd is async and clears mCurrentTask.

Comment on attachment 8688703 [details] [diff] [review] Check that mSpeechQueue is not empty before referencing first element. Ok, I guess I can live with this then.

The patch for this sec-high security bug landed without sec-approval https://wiki.mozilla.org/Security/Bug_Approval_Process This is marked as a regression, and Boris put it as blocking 1187105. Is that the regressing bug? If so that means this bug affects shipping Firefox 42. * Is that the correct regressor? * Where else do we need to back-port this? Filling out the sec-approval form would have asked these and other questions we need to manage security releases and assess risk should someone reverse-engineer an exploit from the patch. Please request retroactive approval so we can finish that process.

This is only preffed on by default in Nightly. So I don't know how much of a risk this is or isn't. This patch is very easily back-ported since it is a one-liner, so it might not hurt..