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

content/browser/speech/speech_recognition_manager_impl.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/speech/speech_recognition_manager_impl.h"
 
 #include "base/bind.h"
 #include "content/browser/browser_main_loop.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/media/media_stream_ui_proxy.h"
@@ skipping 166 matching lines @@
   }
 }
 
 void SpeechRecognitionManagerImpl::RecognitionAllowedCallback(int session_id,
                                                               bool ask_user,
                                                               bool is_allowed) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!SessionExists(session_id))
     return;
 
+  SessionsTable::iterator iter = sessions_.find(session_id);
+  DCHECK(iter != sessions_.end());

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

DCHECK_NE?

[Primiano Tucci (use gerrit), 2013/10/02 14:09:01]

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.

+  Session *session = iter->second;

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

Session* session = iter->second;

[Primiano Tucci (use gerrit), 2013/10/02 14:09:01]

Oh, right. 

+
   if (ask_user) {
-    SessionsTable::iterator iter = sessions_.find(session_id);
-    DCHECK(iter != sessions_.end());
-    SpeechRecognitionSessionContext& context = iter->second->context;
+    SpeechRecognitionSessionContext& context = session->context;
     context.label = media_stream_manager_->MakeMediaAccessRequest(
         context.render_process_id,
         context.render_view_id,
         context.request_id,
         StreamOptions(MEDIA_DEVICE_AUDIO_CAPTURE, MEDIA_NO_SERVICE),
         GURL(context.context_name),
         base::Bind(
             &SpeechRecognitionManagerImpl::MediaRequestPermissionCallback,
             weak_factory_.GetWeakPtr(), session_id));
     return;
   }
 
   if (is_allowed) {

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

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:01]

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.

     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&SpeechRecognitionManagerImpl::DispatchEvent,
                    weak_factory_.GetWeakPtr(),
                    session_id,
                    EVENT_START));
-  } else {
+  } else if (!session->abort_requested) {
     OnRecognitionError(session_id, SpeechRecognitionError(
         SPEECH_RECOGNITION_ERROR_NOT_ALLOWED));
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&SpeechRecognitionManagerImpl::DispatchEvent,
                    weak_factory_.GetWeakPtr(),
                    session_id,
                    EVENT_ABORT));
   }
 }
@@ skipping 25 matching lines @@
 }
 
 void SpeechRecognitionManagerImpl::AbortSession(int session_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!SessionExists(session_id))
     return;
 
   SessionsTable::iterator iter = sessions_.find(session_id);
   iter->second->ui.reset();
 
+  if (iter->second->abort_requested)
+    return;
+
+  iter->second->abort_requested = true;
+
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&SpeechRecognitionManagerImpl::DispatchEvent,
                  weak_factory_.GetWeakPtr(),
                  session_id,
                  EVENT_ABORT));
 }
 
 void SpeechRecognitionManagerImpl::StopAudioCaptureForSession(int session_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ skipping 401 matching lines @@
 void SpeechRecognitionManagerImpl::ShowAudioInputSettings() {
   // Since AudioManager::ShowAudioInputSettings can potentially launch external
   // processes, do that in the FILE thread to not block the calling threads.
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                           base::Bind(&ShowAudioInputSettingsOnFileThread,
                                      audio_manager_));
 }
 
 SpeechRecognitionManagerImpl::Session::Session()
   : id(kSessionIDInvalid),
+    abort_requested(false),
     listener_is_active(true) {
 }
 
 SpeechRecognitionManagerImpl::Session::~Session() {
 }
 
 }  // namespace content