Fix getUserMedia so that failure is reported for invalid audio sources.

content/renderer/media/user_media_client_impl.cc

 // Copyright (c) 2012 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/renderer/media/user_media_client_impl.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 414 matching lines @@
   UserMediaRequestInfo* request_info = FindUserMediaRequestInfo(request_id);
   if (!request_info) {
     // This can happen if the request is canceled or the frame reloads while
     // MediaStreamDispatcher is processing the request.
     DVLOG(1) << "Request ID not found";
     OnStreamGeneratedForCancelledRequest(audio_array, video_array);
     return;
   }
   request_info->generated = true;
 
+  for (const auto* array : {&audio_array, &video_array}) {
+    for (const auto& info : *array) {
+      WebRtcLogMessage(base::StringPrintf("Request %d for device \"%s\"",
+                                          request_id,
+                                          info.device.name.c_str()));
+    }
+  }
+
   DCHECK(!request_info->request.isNull());
   blink::WebVector<blink::WebMediaStreamTrack> audio_track_vector(
       audio_array.size());
   CreateAudioTracks(audio_array, request_info->request.audioConstraints(),
                     &audio_track_vector, request_info);
 
   blink::WebVector<blink::WebMediaStreamTrack> video_track_vector(
       video_array.size());
   CreateVideoTracks(video_array, request_info->request.videoConstraints(),
                     &video_track_vector, request_info);
 
   blink::WebString webkit_id = blink::WebString::fromUTF8(label);
   blink::WebMediaStream* web_stream = &(request_info->web_stream);
 
-  web_stream->initialize(webkit_id, audio_track_vector,
-                         video_track_vector);
+  web_stream->initialize(webkit_id, audio_track_vector, video_track_vector);
   web_stream->setExtraData(new MediaStream());
 
   // Wait for the tracks to be started successfully or to fail.
   request_info->CallbackOnTracksStarted(
       base::Bind(&UserMediaClientImpl::OnCreateNativeTracksCompleted,
                  weak_factory_.GetWeakPtr()));
 }
 
 void UserMediaClientImpl::OnStreamGeneratedForCancelledRequest(
     const StreamDeviceInfoArray& audio_array,
     const StreamDeviceInfoArray& video_array) {
   // Only stop the device if the device is not used in another MediaStream.
   for (StreamDeviceInfoArray::const_iterator device_it = audio_array.begin();
        device_it != audio_array.end(); ++device_it) {
     if (!FindLocalSource(*device_it))
       media_stream_dispatcher_->StopStreamDevice(*device_it);
   }
 
   for (StreamDeviceInfoArray::const_iterator device_it = video_array.begin();
        device_it != video_array.end(); ++device_it) {
     if (!FindLocalSource(*device_it))
       media_stream_dispatcher_->StopStreamDevice(*device_it);
   }
 }
 
+// static
+void UserMediaClientImpl::OnAudioSourceStartedOnAudioThread(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    base::WeakPtr<UserMediaClientImpl> weak_ptr,
+    MediaStreamSource* source,
+    MediaStreamRequestResult result,
+    const blink::WebString& result_name) {
+  task_runner->PostTask(FROM_HERE,
+                        base::Bind(&UserMediaClientImpl::OnAudioSourceStarted,
+                                   weak_ptr, source, result, result_name));
+}
+
+void UserMediaClientImpl::OnAudioSourceStarted(
+    MediaStreamSource* source,
+    MediaStreamRequestResult result,
+    const blink::WebString& result_name) {
+  DCHECK(CalledOnValidThread());
+
+  for (auto it = pending_local_sources_.begin();
+       it != pending_local_sources_.end(); ++it) {
+    MediaStreamSource* const source_extra_data =
+        static_cast<MediaStreamSource*>((*it).getExtraData());
+    if (source_extra_data == source) {

[miu, 2017/01/11 20:51:11]

style nit:

  if (source_extra_data != source)
    continue;
  ...rest of code one indent level less...

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

Done.

+      if (result == MEDIA_DEVICE_OK)
+        local_sources_.push_back((*it));
+      pending_local_sources_.erase(it);
+      // Since a request object might get deleted and removed from the

[miu, 2017/01/11 20:51:11]

This comment leads me to believe the pointer may become invalid. How do we know
all the pointers we're copying to the temp vector will be valid? Or, is it that
*only* the object being notified could be deleted (i.e., inside our
OnAudioSourceStarted() call here)? If that's the case, let's clarify that in the
comment.

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

Indeed, the pointer to the request object will become invalid. The call sequence
is like this:

[this is the request object]
OnAudioSourceStarted()
  -> OnTrackStarted
    -> CheckAllTracksStarted
      -> (the request fires ready_callback_.Run())
        [|this| now becomes UserMediaClientImpl]
        -> UserMediaClientImpl::OnCreateNativeTracksCompleted
          -> UserMediaClientImpl::DeleteUserMediaRequestInfo(request)
            -> delete request
      ...now unwind stack with a deleted request object...

I didn't really like this so my first approach was to defer (Post) the deletion
of the request object.  However, that caused several content_unittests to fail
as they expected the request object to be deleted straight after
OnCreateNativeTracksCompleted.  So I decided to add documentation to make this
clear and change the new code to adapt to the existing design.  See difference
between patch set 2 and 3.
Correct, only the request that receives the notification can be deleted.  Added
clarification to comment.

+      // |user_media_requests_| array while we iterate through it, we need
+      // to jump through this hoop here, copy pointers to the objects we're
+      // notifying and avoid using user_media_requests_ as we iterate+notify.
+      std::vector<UserMediaRequestInfo*> requests;

[miu, 2017/01/11 20:51:11]

Suggest simplifying by just using the vector copy ctor here:

  const std::vector<UserMediaRequestInfo*> notifyees(user_media_requests_);
  for (auto* request : notifyees)
    request->OnAudioSourceStarted(...);

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

Unfortunately user_media_requests_ is a ScopedVector<UserMediaRequestInfo>.

[miu, 2017/01/12 21:25:37]

Ah! This might be a good time to change the type (to stop using the deprecated
ScopedVector class). The new way is:

  std::vector<std::unique_ptr<Foo>> user_media_requests_;

Then, implicit casting between std::unique_ptr<Foo> and Foo* might allow you to
use a copy ctor (either vector(const vector<U>&) or maybe the vector(iter begin,
iter end) form?). Simple to try, and if it doesn't work out, at least you've
stopped using the ScopedVector. ;)

[tommi (sloooow) - chröme, 2017/01/13 18:43:52]

Type changed.

I gave the conversion a try and the only way I got it to work was by passing the
size to the ctor and then use std::transform() to copy the values from one array
to the other.  Due to the verbosity of that approach, with git cl format, it
takes 5 lines of code instead of the current 4 which include the 'reserve' step.
 So I figured I'd just keep it as is to not cause confusion for the next reader.

Still, at least we stopped using ScopedVector.

+      requests.reserve(user_media_requests_.size());
+      for (const auto& request : user_media_requests_)
+        requests.push_back(request);
+      for (const auto& request : requests)

[miu, 2017/01/11 20:51:11]

nit: The elements are pointers (plain old data types), so just "auto*" is all
that's needed here.

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

Done.

+        request->OnAudioSourceStarted(source, result, result_name);
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
 void UserMediaClientImpl::FinalizeEnumerateDevices(
     blink::WebMediaDevicesRequest request,
     const EnumerationResult& result) {
   DCHECK_EQ(static_cast<size_t>(NUM_MEDIA_DEVICE_TYPES), result.size());
 
   blink::WebVector<blink::WebMediaDeviceInfo> devices(
       result[MEDIA_DEVICE_TYPE_AUDIO_INPUT].size() +
       result[MEDIA_DEVICE_TYPE_VIDEO_INPUT].size() +
       result[MEDIA_DEVICE_TYPE_AUDIO_OUTPUT].size());
   size_t index = 0;
@@ skipping 47 matching lines @@
     // as the underlying media device is unplugged from the system.
     return;
   }
   // By creating |source| it is guaranteed that the blink::WebMediaStreamSource
   // object is valid during the cleanup.
   blink::WebMediaStreamSource source(*source_ptr);
   StopLocalSource(source, false);
   RemoveLocalSource(source);
 }
 
-void UserMediaClientImpl::InitializeSourceObject(
+void UserMediaClientImpl::InitializeVideoSourceObject(
     const StreamDeviceInfo& device,
-    blink::WebMediaStreamSource::Type type,
     const blink::WebMediaConstraints& constraints,
     blink::WebMediaStreamSource* webkit_source) {
-  const blink::WebMediaStreamSource* existing_source =
-      FindLocalSource(device);
-  if (existing_source) {
-    *webkit_source = *existing_source;
-    DVLOG(1) << "Source already exist. Reusing source with id "
-             << webkit_source->id().utf8();
+  DCHECK(CalledOnValidThread());
+
+  *webkit_source = FindOrInitializeSourceObject(device);
+  if (webkit_source->getExtraData())
+    return;
+
+  webkit_source->setExtraData(CreateVideoSource(
+      device, base::Bind(&UserMediaClientImpl::OnLocalSourceStopped,
+                         weak_factory_.GetWeakPtr())));
+  local_sources_.push_back(*webkit_source);
+}
+
+void UserMediaClientImpl::InitializeAudioSourceObject(
+    const StreamDeviceInfo& device,
+    const blink::WebMediaConstraints& constraints,
+    blink::WebMediaStreamSource* webkit_source,
+    bool* source_initialized) {
+  DCHECK(CalledOnValidThread());
+
+  *webkit_source = FindOrInitializeSourceObject(device);
+  if (webkit_source->getExtraData()) {
+    *source_initialized = true;
     return;
   }
 
-  webkit_source->initialize(blink::WebString::fromUTF8(device.device.id), type,
-                            blink::WebString::fromUTF8(device.device.name),
-                            false /* remote */);
+  *source_initialized = false;
 
-  DVLOG(1) << "Initialize source object :"
-           << "id = " << webkit_source->id().utf8()
-           << ", name = " << webkit_source->name().utf8();
+  // See if the source is already being initialized.
+  auto* pending = FindPendingLocalSource(device);
+  if (pending) {
+    *webkit_source = *pending;
+    return;
+  }
 
-  if (type == blink::WebMediaStreamSource::TypeVideo) {
-    webkit_source->setExtraData(
-        CreateVideoSource(
-            device,
-            base::Bind(&UserMediaClientImpl::OnLocalSourceStopped,
-                       weak_factory_.GetWeakPtr())));
-  } else {
-    DCHECK_EQ(blink::WebMediaStreamSource::TypeAudio, type);
-    MediaStreamAudioSource* const audio_source =
-        CreateAudioSource(device, constraints);
-    audio_source->SetStopCallback(
-        base::Bind(&UserMediaClientImpl::OnLocalSourceStopped,
-                   weak_factory_.GetWeakPtr()));
-    webkit_source->setExtraData(audio_source);  // Takes ownership.
-  }
-  local_sources_.push_back(*webkit_source);
+  // While sources are being initialized, keep them in a separate array.
+  // Once they've finished initialized, they'll be moved over to local_sources_.
+  // See OnAudioSourceStarted for more details.
+  pending_local_sources_.push_back(*webkit_source);
+
+  MediaStreamSource::ConstraintsCallback source_ready = base::Bind(
+      &UserMediaClientImpl::OnAudioSourceStartedOnAudioThread,
+      base::ThreadTaskRunnerHandle::Get(), weak_factory_.GetWeakPtr());
+
+  MediaStreamAudioSource* const audio_source =
+      CreateAudioSource(device, constraints, source_ready);
+  audio_source->SetStopCallback(base::Bind(
+      &UserMediaClientImpl::OnLocalSourceStopped, weak_factory_.GetWeakPtr()));
+  webkit_source->setExtraData(audio_source);  // Takes ownership.
 }
 
 MediaStreamAudioSource* UserMediaClientImpl::CreateAudioSource(
     const StreamDeviceInfo& device,
-    const blink::WebMediaConstraints& constraints) {
+    const blink::WebMediaConstraints& constraints,
+    const MediaStreamSource::ConstraintsCallback& source_ready) {
+  DCHECK(CalledOnValidThread());
   // If the audio device is a loopback device (for screen capture), or if the
   // constraints/effects parameters indicate no audio processing is needed,
   // create an efficient, direct-path MediaStreamAudioSource instance.
   if (IsScreenCaptureMediaType(device.device.type) ||
       !MediaStreamAudioProcessor::WouldModifyAudio(
           constraints, device.device.input.effects)) {
     return new LocalMediaStreamAudioSource(RenderFrameObserver::routing_id(),
-                                           device);
+                                           device, source_ready);
   }
 
   // The audio device is not associated with screen capture and also requires
   // processing.
   ProcessedLocalAudioSource* source = new ProcessedLocalAudioSource(
-      RenderFrameObserver::routing_id(), device, dependency_factory_);
-  source->SetSourceConstraints(constraints);
+      RenderFrameObserver::routing_id(), device, constraints, source_ready,
+      dependency_factory_);
   return source;
 }
 
 MediaStreamVideoSource* UserMediaClientImpl::CreateVideoSource(
     const StreamDeviceInfo& device,
     const MediaStreamSource::SourceStoppedCallback& stop_callback) {
+  DCHECK(CalledOnValidThread());
   content::MediaStreamVideoCapturerSource* ret =
       new content::MediaStreamVideoCapturerSource(stop_callback, device,
                                                   render_frame());
   return ret;
 }
 
 void UserMediaClientImpl::CreateVideoTracks(
     const StreamDeviceInfoArray& devices,
     const blink::WebMediaConstraints& constraints,
     blink::WebVector<blink::WebMediaStreamTrack>* webkit_tracks,
     UserMediaRequestInfo* request) {
+  DCHECK(CalledOnValidThread());
   DCHECK_EQ(devices.size(), webkit_tracks->size());
 
   for (size_t i = 0; i < devices.size(); ++i) {
     blink::WebMediaStreamSource webkit_source;
-    InitializeSourceObject(devices[i],
-                           blink::WebMediaStreamSource::TypeVideo,
-                           constraints,
-                           &webkit_source);
+    InitializeVideoSourceObject(devices[i], constraints, &webkit_source);
     (*webkit_tracks)[i] =
         request->CreateAndStartVideoTrack(webkit_source, constraints);
   }
 }
 
 void UserMediaClientImpl::CreateAudioTracks(
     const StreamDeviceInfoArray& devices,
     const blink::WebMediaConstraints& constraints,
     blink::WebVector<blink::WebMediaStreamTrack>* webkit_tracks,
     UserMediaRequestInfo* request) {
+  DCHECK(CalledOnValidThread());
   DCHECK_EQ(devices.size(), webkit_tracks->size());
 
-  // Log the device names for this request.
-  for (StreamDeviceInfoArray::const_iterator it = devices.begin();
-       it != devices.end(); ++it) {
-    WebRtcLogMessage(base::StringPrintf(
-        "Generated media stream for request id %d contains audio device name"
-        " \"%s\"",
-        request->request_id,
-        it->device.name.c_str()));
-  }
-
   StreamDeviceInfoArray overridden_audio_array = devices;
   if (!request->enable_automatic_output_device_selection) {
     // If the GetUserMedia request did not explicitly set the constraint
     // kMediaStreamRenderToAssociatedSink, the output device parameters must
     // be removed.
-    for (StreamDeviceInfoArray::iterator it = overridden_audio_array.begin();
-         it != overridden_audio_array.end(); ++it) {
-      it->device.matched_output_device_id = "";
-      it->device.matched_output = MediaStreamDevice::AudioDeviceParameters();
+    for (auto& device_info : overridden_audio_array) {
+      device_info.device.matched_output_device_id = "";
+      device_info.device.matched_output =
+          MediaStreamDevice::AudioDeviceParameters();
     }
   }
 
   for (size_t i = 0; i < overridden_audio_array.size(); ++i) {
     blink::WebMediaStreamSource webkit_source;
-    InitializeSourceObject(overridden_audio_array[i],
-                           blink::WebMediaStreamSource::TypeAudio,
-                           constraints,
-                           &webkit_source);
+    bool source_initialized = true;
+    InitializeAudioSourceObject(overridden_audio_array[i], constraints,
+                                &webkit_source, &source_initialized);
     (*webkit_tracks)[i].initialize(webkit_source);
-    request->StartAudioTrack((*webkit_tracks)[i]);
+    request->StartAudioTrack((*webkit_tracks)[i], source_initialized);

[miu, 2017/01/11 20:51:11]

It's a bit confusing to pass in a bool turning "StartAudioTrack" into a "maybe
start the audio track, if the bool is true, except for Android." So, could we
move that decision here, and make "StartAudioTrack" truly an "always start the
audio track" (like it was before)? Combining that, and the changes to
InitializeAudioSourceObject() proposed in other comments, the body of this loop
becomes:

  for (...) {
    blink::WebMediaStreamSource webkit_source;
    const bool is_already_capturing = InitializeAudioSourceObject(...);
    (*webkit_tracks)[i].initialize(webkit_source);
    if (is_already_capturing)
      request->StartAudioTrack((*webkit_tracks)[i])
  }

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

The variable name I chose caused the confusion.  I changed the name of the bool
to now be |is_pending|.  The semantics of starting the audio track are unchanged
as far as Start() goes, since it always starts the audio track.

The difference is that for local audio tracks, Start() may not complete the
process.  If |is_pending| is true, then we need to wait for the
OnAudioSourceStarted event (or a failure to be reported).  Aside from that, the
behavior is the same.

   }
 }
 
 void UserMediaClientImpl::OnCreateNativeTracksCompleted(
     UserMediaRequestInfo* request,
     MediaStreamRequestResult result,
     const blink::WebString& result_name) {
+  DCHECK(CalledOnValidThread());
   DVLOG(1) << "UserMediaClientImpl::OnCreateNativeTracksComplete("
            << "{request_id = " << request->request_id << "} "
            << "{result = " << result << "})";
 
   if (result == content::MEDIA_DEVICE_OK) {
     GetUserMediaRequestSucceeded(request->web_stream, request->request);
   } else {
     GetUserMediaRequestFailed(request->request, result, result_name);
 
     blink::WebVector<blink::WebMediaStreamTrack> tracks;
@@ skipping 126 matching lines @@
   request_info.requestFailed();
 }
 
 void UserMediaClientImpl::EnumerateDevicesSucceded(
     blink::WebMediaDevicesRequest* request,
     blink::WebVector<blink::WebMediaDeviceInfo>& devices) {
   request->requestSucceeded(devices);
 }
 
 const blink::WebMediaStreamSource* UserMediaClientImpl::FindLocalSource(
+    const LocalStreamSources& sources,
     const StreamDeviceInfo& device) const {
-  for (LocalStreamSources::const_iterator it = local_sources_.begin();
-       it != local_sources_.end(); ++it) {
+  for (const auto& local_source : sources) {
     MediaStreamSource* const source =
-        static_cast<MediaStreamSource*>(it->getExtraData());
+        static_cast<MediaStreamSource*>(local_source.getExtraData());
     const StreamDeviceInfo& active_device = source->device_info();
-    if (IsSameDevice(active_device, device)) {
-      return &(*it);
-    }
+    if (IsSameDevice(active_device, device))
+      return &local_source;
   }
-  return NULL;
+  return nullptr;
+}
+
+blink::WebMediaStreamSource UserMediaClientImpl::FindOrInitializeSourceObject(
+    const StreamDeviceInfo& device) {
+  const blink::WebMediaStreamSource* existing_source = FindLocalSource(device);
+  if (existing_source) {
+    DVLOG(1) << "Source already exists. Reusing source with id "
+             << existing_source->id().utf8();
+    return *existing_source;
+  }
+
+  blink::WebMediaStreamSource::Type type =
+      IsAudioInputMediaType(device.device.type)
+          ? blink::WebMediaStreamSource::TypeAudio
+          : blink::WebMediaStreamSource::TypeVideo;
+
+  blink::WebMediaStreamSource source;
+  source.initialize(blink::WebString::fromUTF8(device.device.id), type,
+                    blink::WebString::fromUTF8(device.device.name),
+                    false /* remote */);
+
+  DVLOG(1) << "Initialize source object :"
+           << "id = " << source.id().utf8()
+           << ", name = " << source.name().utf8();
+  return source;
 }
 
 bool UserMediaClientImpl::RemoveLocalSource(
     const blink::WebMediaStreamSource& source) {
-  bool device_found = false;
+  DCHECK(CalledOnValidThread());
+
   for (LocalStreamSources::iterator device_it = local_sources_.begin();
        device_it != local_sources_.end(); ++device_it) {
     if (IsSameSource(*device_it, source)) {
-      device_found = true;
       local_sources_.erase(device_it);
-      break;
+      return true;
     }
   }
-  return device_found;
+
+  // Check if the source was pending.
+  for (LocalStreamSources::iterator device_it = pending_local_sources_.begin();
+       device_it != pending_local_sources_.end(); ++device_it) {
+    if (IsSameSource(*device_it, source)) {
+      MediaStreamSource* const source_extra_data =
+          static_cast<MediaStreamSource*>(source.getExtraData());
+      for (const auto& request : user_media_requests_) {

[miu, 2017/01/11 20:51:11]

Does this suffer from the same problem as above, where |user_media_requests_|
could be mutated, or requests destroyed, because we're calling
OnAudioSourceStarted()?

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

Good catch - it must. I factored out the code that iterates through the requests
into a separate function that I call from here and the other place.

+        request->OnAudioSourceStarted(source_extra_data,
+                                      MEDIA_DEVICE_TRACK_START_FAILURE,
+                                      "Failed to access audio capture device");
+      }
+      pending_local_sources_.erase(device_it);
+      return true;
+    }
+  }
+
+  return false;
 }
 
 UserMediaClientImpl::UserMediaRequestInfo*
 UserMediaClientImpl::FindUserMediaRequestInfo(int request_id) {
+  DCHECK(CalledOnValidThread());
   UserMediaRequests::iterator it = user_media_requests_.begin();
   for (; it != user_media_requests_.end(); ++it) {
     if ((*it)->request_id == request_id)
       return (*it);
   }
   return NULL;
 }
 
 UserMediaClientImpl::UserMediaRequestInfo*
 UserMediaClientImpl::FindUserMediaRequestInfo(
     const blink::WebUserMediaRequest& request) {
+  DCHECK(CalledOnValidThread());
   UserMediaRequests::iterator it = user_media_requests_.begin();
   for (; it != user_media_requests_.end(); ++it) {
     if ((*it)->request == request)
       return (*it);
   }
   return NULL;
 }
 
 void UserMediaClientImpl::DeleteUserMediaRequestInfo(
     UserMediaRequestInfo* request) {
+  DCHECK(CalledOnValidThread());
   UserMediaRequests::iterator it = user_media_requests_.begin();
   for (; it != user_media_requests_.end(); ++it) {
     if ((*it) == request) {
       user_media_requests_.erase(it);
       return;
     }
   }
   NOTREACHED();
 }
 
@@ skipping 87 matching lines @@
       request(request),
       request_result_(MEDIA_DEVICE_OK),
       request_result_name_("") {
 }
 
 UserMediaClientImpl::UserMediaRequestInfo::~UserMediaRequestInfo() {
   DVLOG(1) << "~UserMediaRequestInfo";
 }
 
 void UserMediaClientImpl::UserMediaRequestInfo::StartAudioTrack(
-    const blink::WebMediaStreamTrack& track) {
+    const blink::WebMediaStreamTrack& track,
+    bool source_initialized) {
   DCHECK(track.source().getType() == blink::WebMediaStreamSource::TypeAudio);
   MediaStreamAudioSource* native_source =
       MediaStreamAudioSource::From(track.source());
-  DCHECK(native_source);
+  // Add the source as pending since OnTrackStarted will expect it to be there.
+  sources_waiting_for_callback_.push_back(native_source);
 
   sources_.push_back(track.source());
-  sources_waiting_for_callback_.push_back(native_source);
-  if (native_source->ConnectToTrack(track))
+  bool connected = native_source->ConnectToTrack(track);
+  if (source_initialized) {
+    OnTrackStarted(
+        native_source,
+        connected ? MEDIA_DEVICE_OK : MEDIA_DEVICE_TRACK_START_FAILURE, "");
+#if defined(OS_ANDROID)
+  } else if (connected) {
+    CHECK(native_source->is_local_source());

[miu, 2017/01/11 20:51:11]

Suggest moving the Android "readiness" state hack to
InitializeAudioSourceObject(), where we determine whether the source is already
capturing. (i.e., have that method return true for Android unconditionally)

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

I think I've addressed this in the other comments.

+    // On Android, we won't get the callback indicating the device readyness.
+    // TODO(tommi): Update the android implementation to support the
+    // OnAudioSourceStarted notification.  http://crbug.com/679302
     OnTrackStarted(native_source, MEDIA_DEVICE_OK, "");
-  else
-    OnTrackStarted(native_source, MEDIA_DEVICE_TRACK_START_FAILURE, "");
+#endif
+  }

[miu, 2017/01/11 20:51:11]

So, basically, with all the other comments applied, StartAudioTrack() shouldn't
need any changes.

[tommi (sloooow) - chröme, 2017/01/11 22:40:58]

Same here and I think StartAudioTrack still needs to be aware of whether the
Start() operation will be complete synchronously or not.

 }
 
 blink::WebMediaStreamTrack
 UserMediaClientImpl::UserMediaRequestInfo::CreateAndStartVideoTrack(
     const blink::WebMediaStreamSource& source,
     const blink::WebMediaConstraints& constraints) {
   DCHECK(source.getType() == blink::WebMediaStreamSource::TypeVideo);
   MediaStreamVideoSource* native_source =
       MediaStreamVideoSource::GetVideoSource(source);
   DCHECK(native_source);
@@ skipping 30 matching lines @@
     request_result_ = result;
     request_result_name_ = result_name;
   }
 
   CheckAllTracksStarted();
 }
 
 void UserMediaClientImpl::UserMediaRequestInfo::CheckAllTracksStarted() {
   if (!ready_callback_.is_null() && sources_waiting_for_callback_.empty()) {
     ready_callback_.Run(this, request_result_, request_result_name_);
+    // NOTE: |this| might now be deleted.
   }
 }
 
-bool UserMediaClientImpl::UserMediaRequestInfo::IsSourceUsed(
-    const blink::WebMediaStreamSource& source) const {
-  for (std::vector<blink::WebMediaStreamSource>::const_iterator source_it =
-           sources_.begin();
-       source_it != sources_.end(); ++source_it) {
-    if (source_it->id() == source.id())
-      return true;
-  }
-  return false;
-}
-
-void UserMediaClientImpl::UserMediaRequestInfo::RemoveSource(
-    const blink::WebMediaStreamSource& source) {
-  for (std::vector<blink::WebMediaStreamSource>::iterator it =
-           sources_.begin();
-       it != sources_.end(); ++it) {
-    if (source.id() == it->id()) {
-      sources_.erase(it);
-      return;
-    }
-  }
-}
-
 bool UserMediaClientImpl::UserMediaRequestInfo::HasPendingSources() const {
   return !sources_waiting_for_callback_.empty();
 }
 
+void UserMediaClientImpl::UserMediaRequestInfo::OnAudioSourceStarted(
+    MediaStreamSource* source,
+    MediaStreamRequestResult result,
+    const blink::WebString& result_name) {
+  // Check if we're waiting to be notified of this source.  If not, then we'll
+  // ignore the notification.
+  auto found = std::find(sources_waiting_for_callback_.begin(),
+                         sources_waiting_for_callback_.end(), source);
+  if (found != sources_waiting_for_callback_.end())
+    OnTrackStarted(source, result, result_name);
+}
+
 void UserMediaClientImpl::OnDestruct() {
   delete this;
 }
 
 }  // namespace content