Fix for ~ContentCaptureSubscription() after BrowserThreads are torn down.

While debugging flaky browser_tests, it was revealed that a
WebContentsVideoCaptureMachine could actually outlive the tear-down of
the BrowserThreadImpl for the UI thread.  Not having been explicitly
stopped first, destructors in the object graph would execute clean up
code that would attempt operations that are invalid after the
BrowserThreadImpl for the UI thread has been torn down.

This change also includes a bunch of minor clean-ups.

BUG=396413

Review URL: https://codereview.chromium.org/514073002

Cr-Commit-Position: refs/heads/master@{#292513}

content/browser/media/capture/content_video_capture_device_core.cc

@@ -297,9 +297,13 @@ void ContentVideoCaptureDeviceCore::CaptureStarted(bool success) {
 ContentVideoCaptureDeviceCore::ContentVideoCaptureDeviceCore(
     scoped_ptr<VideoCaptureMachine> capture_machine)
     : state_(kIdle),
-      capture_machine_(capture_machine.Pass()) {}
+      capture_machine_(capture_machine.Pass()) {
+  DCHECK(capture_machine_.get());
+}
 
 ContentVideoCaptureDeviceCore::~ContentVideoCaptureDeviceCore() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_NE(state_, kCapturing);
   // If capture_machine is not NULL, then we need to return to the UI thread to
   // safely stop the capture machine.
   if (capture_machine_) {

content/browser/media/capture/content_video_capture_device_core.h

@@ -119,12 +119,9 @@ class ThreadSafeCaptureOracle
 // UI BrowserThread.
 class VideoCaptureMachine {
  public:
-  VideoCaptureMachine() : started_(false) {}
+  VideoCaptureMachine() {}
   virtual ~VideoCaptureMachine() {}
 
-  // This should only be checked on the UI thread.
-  bool started() const { return started_; }
-
   // Starts capturing. Returns true if succeeded.
   // Must be run on the UI BrowserThread.
   virtual bool Start(const scoped_refptr<ThreadSafeCaptureOracle>& oracle_proxy,
@@ -134,9 +131,6 @@ class VideoCaptureMachine {
   // |callback| is invoked after the capturing has stopped.
   virtual void Stop(const base::Closure& callback) = 0;
 
- protected:
-  bool started_;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(VideoCaptureMachine);
 };
@@ -145,7 +139,7 @@ class VideoCaptureMachine {
 //
 // Separating this from the "shell classes" WebContentsVideoCaptureDevice and
 // DesktopCaptureDeviceAura allows safe destruction without needing to block any
-// threads (e.g., the IO BrowserThread), as well as code sharing.
+// threads, as well as code sharing.
 //
 // ContentVideoCaptureDeviceCore manages a simple state machine and the pipeline
 // (see notes at top of this file).  It times the start of successive captures
@@ -173,8 +167,8 @@ class CONTENT_EXPORT ContentVideoCaptureDeviceCore
 
   void TransitionStateTo(State next_state);
 
-  // Called on the IO thread in response to StartCaptureMachine().
-  // |success| is true if capture machine succeeded to start.
+  // Called back in response to StartCaptureMachine().  |success| is true if
+  // capture machine succeeded to start.
   void CaptureStarted(bool success);
 
   // Stops capturing and notifies client_ of an error state.
@@ -193,7 +187,7 @@ class CONTENT_EXPORT ContentVideoCaptureDeviceCore
 
   // Our thread-safe capture oracle which serves as the gateway to the video
   // capture pipeline. Besides the VideoCaptureDevice itself, it is the only
-  // component of the/ system with direct access to |client_|.
+  // component of the system with direct access to |client_|.
   scoped_refptr<ThreadSafeCaptureOracle> oracle_proxy_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentVideoCaptureDeviceCore);

content/browser/media/capture/desktop_capture_device_aura.cc

@@ -227,7 +227,6 @@ bool DesktopVideoCaptureMachine::Start(
                base::Bind(&DesktopVideoCaptureMachine::Capture, AsWeakPtr(),
                           false));
 
-  started_ = true;
   return true;
 }
 
@@ -246,8 +245,6 @@ void DesktopVideoCaptureMachine::Stop(const base::Closure& callback) {
   // Stop timer.
   timer_.Stop();
 
-  started_ = false;
-
   callback.Run();
 }
 

content/browser/media/capture/web_contents_video_capture_device.cc

@@ -371,7 +371,7 @@ ContentCaptureSubscription::ContentCaptureSubscription(
                         &delivery_log_),
       capture_callback_(capture_callback),
       timer_(true, true) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   RenderWidgetHostViewBase* view = static_cast<RenderWidgetHostViewBase*>(
       source.GetView());
@@ -399,7 +399,13 @@ ContentCaptureSubscription::ContentCaptureSubscription(
 }
 
 ContentCaptureSubscription::~ContentCaptureSubscription() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // If the BrowserThreads have been torn down, then the browser is in the final
+  // stages of exiting and it is dangerous to take any further action.  We must
+  // return early.  http://crbug.com/396413
+  if (!BrowserThread::IsMessageLoopValid(BrowserThread::UI))
+    return;
+
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (kAcceleratedSubscriberIsSupported) {
     RenderViewHost* source = RenderViewHost::FromID(render_process_id_,
                                                     render_view_id_);
@@ -416,7 +422,7 @@ void ContentCaptureSubscription::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(NOTIFICATION_RENDER_WIDGET_HOST_DID_UPDATE_BACKING_STORE, type);
 
   RenderWidgetHostImpl* rwh =
@@ -455,7 +461,7 @@ void ContentCaptureSubscription::Observe(
 }
 
 void ContentCaptureSubscription::OnTimer() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   TRACE_EVENT0("mirroring", "ContentCaptureSubscription::OnTimer");
 
   scoped_refptr<media::VideoFrame> frame;
@@ -575,18 +581,13 @@ WebContentsCaptureMachine::WebContentsCaptureMachine(int render_process_id,
       fullscreen_widget_id_(MSG_ROUTING_NONE),
       weak_ptr_factory_(this) {}
 
-WebContentsCaptureMachine::~WebContentsCaptureMachine() {
-  BrowserThread::PostBlockingPoolTask(
-      FROM_HERE,
-      base::Bind(&DeleteOnWorkerThread, base::Passed(&render_thread_),
-                 base::Bind(&base::DoNothing)));
-}
+WebContentsCaptureMachine::~WebContentsCaptureMachine() {}
 
 bool WebContentsCaptureMachine::Start(
     const scoped_refptr<ThreadSafeCaptureOracle>& oracle_proxy,
     const media::VideoCaptureParams& params) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(!started_);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(!weak_ptr_factory_.HasWeakPtrs());  // Should not be started.
 
   DCHECK(oracle_proxy.get());
   oracle_proxy_ = oracle_proxy;
@@ -605,12 +606,11 @@ bool WebContentsCaptureMachine::Start(
     return false;
   }
 
-  started_ = true;
   return true;
 }
 
 void WebContentsCaptureMachine::Stop(const base::Closure& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   subscription_.reset();
   if (web_contents()) {
     web_contents()->DecrementCapturerCount();
@@ -623,12 +623,12 @@ void WebContentsCaptureMachine::Stop(const base::Closure& callback) {
 
   // The render thread cannot be stopped on the UI thread, so post a message
   // to the thread pool used for blocking operations.
-  BrowserThread::PostBlockingPoolTask(
-      FROM_HERE,
-      base::Bind(&DeleteOnWorkerThread, base::Passed(&render_thread_),
-                 callback));
-
-  started_ = false;
+  if (render_thread_.get()) {
+    BrowserThread::PostBlockingPoolTask(
+        FROM_HERE,
+        base::Bind(&DeleteOnWorkerThread, base::Passed(&render_thread_),
+                   callback));
+  }
 }
 
 void WebContentsCaptureMachine::Capture(
@@ -636,7 +636,7 @@ void WebContentsCaptureMachine::Capture(
     const scoped_refptr<media::VideoFrame>& target,
     const RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback&
         deliver_frame_cb) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   RenderWidgetHost* rwh = GetTarget();
   RenderWidgetHostViewBase* view =
@@ -683,7 +683,7 @@ void WebContentsCaptureMachine::Capture(
 }
 
 gfx::Size WebContentsCaptureMachine::ComputeOptimalTargetSize() const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   gfx::Size optimal_size = oracle_proxy_->GetCaptureSize();
 
@@ -714,7 +714,7 @@ gfx::Size WebContentsCaptureMachine::ComputeOptimalTargetSize() const {
 }
 
 bool WebContentsCaptureMachine::StartObservingWebContents() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // Look-up the RenderFrameHost and, from that, the WebContents that wraps it.
   // If successful, begin observing the WebContents instance.
@@ -743,7 +743,7 @@ bool WebContentsCaptureMachine::StartObservingWebContents() {
 }
 
 void WebContentsCaptureMachine::WebContentsDestroyed() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   subscription_.reset();
   web_contents()->DecrementCapturerCount();
@@ -751,7 +751,7 @@ void WebContentsCaptureMachine::WebContentsDestroyed() {
 }
 
 RenderWidgetHost* WebContentsCaptureMachine::GetTarget() const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!web_contents())
     return NULL;
 
@@ -774,7 +774,7 @@ void WebContentsCaptureMachine::DidCopyFromBackingStore(
         deliver_frame_cb,
     bool success,
     const SkBitmap& bitmap) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   base::TimeTicks now = base::TimeTicks::Now();
   DCHECK(render_thread_.get());
@@ -797,7 +797,7 @@ void WebContentsCaptureMachine::DidCopyFromCompositingSurfaceToVideoFrame(
     const RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback&
         deliver_frame_cb,
     bool success) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   base::TimeTicks now = base::TimeTicks::Now();
 
   if (success) {
@@ -810,7 +810,7 @@ void WebContentsCaptureMachine::DidCopyFromCompositingSurfaceToVideoFrame(
 }
 
 void WebContentsCaptureMachine::RenewFrameSubscription() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // Always destroy the old subscription before creating a new one.
   subscription_.reset();
@@ -839,7 +839,7 @@ WebContentsVideoCaptureDevice::~WebContentsVideoCaptureDevice() {
 // static
 media::VideoCaptureDevice* WebContentsVideoCaptureDevice::Create(
     const std::string& device_id) {
-  // Parse device_id into render_process_id and render_view_id.
+  // Parse device_id into render_process_id and main_render_frame_id.
   int render_process_id = -1;
   int main_render_frame_id = -1;
   if (!WebContentsCaptureUtil::ExtractTabCaptureTarget(