Web Speech API: Fix a race condition causing renderer crash.

Web Speech API: Fix a race condition causing renderer crash.

The introduction of the MediaRequestPermission (i.e. infobar) has
opened a race window which causes a recognition session to be aborted
twice, causing a consequent crash on the renderer.
The race window is the following:
1) Session 1 is started (SpeechRecognitionManagerImpl::StartSession)
2) Security checks for session 1 are started asynchronously
   (delegate_->CheckRecognitionIsAllowed).
3) Session 2 is started. This causes an immediate abort of session 1.
4) The oustanding security check for session 1 completes and returns
   a nack. The nack causes an abort of session 1 (in
   RecognitionAllowedCallback). However, session 1 was already aborted
   in 3).
5) The double abort is not tolerated by the renderer, which crashes.

This CL closes the race window with a not-so-elegant fix.
A refactoring of the speech_recognition_manager_impl.cc is STRONGLY
adviced (Hint: use and extend the already existing FSM to keep track
of the asynchronous completion of security checks. Don't introduce
extra state with extra variables).

BUG=296690, 116954
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226523

[Primiano Tucci (use gerrit), 2013-10-01 21:06:46]

PTAL

[jam, 2013-10-01 23:48:23]

why is chromium-reviews@chromium.org not cc'd?

[Primiano Tucci (use gerrit), 2013-10-02 04:48:29]

On 2013/10/01 23:48:23, jam wrote:
> why is mailto:chromium-reviews@chromium.org not cc'd?

uhm, I'm afraid it was my fault. I messed around with the private flag and then
restored the original cc list by copying and pasting from another cl. 
I suppose at some point I forgot to copy chromium-reviews.

[tommi (sloooow) - chröme, 2013-10-02 08:01:14]

lgtm % nits

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
content/browser/speech/speech_recognition_manager_impl.cc:188: DCHECK(iter !=
sessions_.end());
DCHECK_NE?

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
content/browser/speech/speech_recognition_manager_impl.cc:189: Session *session
= iter->second;
Session* session = iter->second;

[no longer working on chromium, 2013-10-02 09:05:06]

lgtm with one concern, please address it.

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
content/browser/speech/speech_recognition_manager_impl.cc:205: if (is_allowed) {
question, should't you do nothing if session->abort_requested is true here?

I am thinking about the case that the UI allows the permission, but the session
just gets aboarted, we probably should not trigger any EVENT_START event in such
case.

If my concern is valid, we can check session->abort_requested at the beginning
of this method.

[Primiano Tucci (use gerrit), 2013-10-02 14:09:00]

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
content/browser/speech/speech_recognition_manager_impl.cc:188: DCHECK(iter !=
sessions_.end());
On 2013/10/02 08:01:14, tommi wrote:
> DCHECK_NE?

Hmm I tried (see patchset 2 trybots) but DCHECK_NE templates don't seem to cope
well with the map::iterator cast operator. And doing the explicit task is less
readable. Reverting to the != DCHECK.

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
content/browser/speech/speech_recognition_manager_impl.cc:189: Session *session
= iter->second;
On 2013/10/02 08:01:14, tommi wrote:
> Session* session = iter->second;
Oh, right.

https://codereview.chromium.org/25550003/diff/1/content/browser/speech/speech...
content/browser/speech/speech_recognition_manager_impl.cc:205: if (is_allowed) {
On 2013/10/02 09:05:06, xians1 wrote:
> question, should't you do nothing if session->abort_requested is true here?
> 
> I am thinking about the case that the UI allows the permission, but the
session
> just gets aboarted, we probably should not trigger any EVENT_START event in
such
> case.
> 
> If my concern is valid, we can check session->abort_requested at the beginning
> of this method.

Oh right, good catch. If the permission gets cached we could end up having the
same problem in the upper branch.
Makes sense to me to move the abort check up.

[commit-bot: I haz the power, 2013-10-02 17:36:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/25550003/26001

[commit-bot: I haz the power, 2013-10-02 19:22:34]

Change committed as 226523