Attempt to fixed crash on stop mirroring by stopping render thread on a worker thread.

content/browser/renderer_host/media/web_contents_video_capture_device.cc

Index: content/browser/renderer_host/media/web_contents_video_capture_device.cc
diff --git a/content/browser/renderer_host/media/web_contents_video_capture_device.cc b/content/browser/renderer_host/media/web_contents_video_capture_device.cc
index 4f215398230fad03079605dbb1950823ffb53d2c..76850f4b7cdefc1d459ebe4b855b964f36a9cf5d 100644
--- a/content/browser/renderer_host/media/web_contents_video_capture_device.cc
+++ b/content/browser/renderer_host/media/web_contents_video_capture_device.cc
@@ -108,6 +108,22 @@ void InvokeCaptureFrameCallback(
   capture_frame_cb.Run(timestamp, frame_captured);
 }
 
+void StopOnWorkerThread(base::Thread* render_thread,
+                        const base::Closure& callback) {
+  if (render_thread) {

[Alpha Left Google, 2014/01/09 01:37:07]

DCHECK that render_thread is not NULL instead of doing if a if(){}.

[imcheng, 2014/01/09 23:31:09]

Done.

+    render_thread->Stop();
+
+    // After thread join call the callback on UI thread.
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+  }
+}
+
+void DeleteOnWorkerThread(scoped_ptr<base::Thread> render_thread) {
+  if (render_thread) {

[Alpha Left Google, 2014/01/09 01:37:07]

Don't need this. Just call reset.

[imcheng, 2014/01/09 23:31:09]

Done.

+    render_thread.reset();
+  }
+}
+
 // FrameSubscriber is a proxy to the ThreadSafeCaptureOracle that's compatible
 // with RenderWidgetHostViewFrameSubscriber. We create one per event type.
 class FrameSubscriber : public RenderWidgetHostViewFrameSubscriber {
@@ -211,7 +227,7 @@ class WebContentsCaptureMachine
   // VideoCaptureMachine overrides.
   virtual bool Start(
       const scoped_refptr<ThreadSafeCaptureOracle>& oracle_proxy) OVERRIDE;
-  virtual void Stop() OVERRIDE;
+  virtual void Stop(const base::Closure& callback) OVERRIDE;
 
   // Starts a copy from the backing store or the composited surface. Must be run
   // on the UI BrowserThread. |deliver_frame_cb| will be run when the operation
@@ -285,7 +301,7 @@ class WebContentsCaptureMachine
 
   // A dedicated worker thread on which SkBitmap->VideoFrame conversion will
   // occur. Only used when this activity cannot be done on the GPU.
-  base::Thread render_thread_;
+  scoped_ptr<base::Thread> render_thread_;

[Alpha Left Google, 2014/01/09 01:37:07]

There's no need to change this.

[imcheng, 2014/01/09 23:31:09]

See comment below

 
   // Makes all the decisions about which frames to copy, and how.
   scoped_refptr<ThreadSafeCaptureOracle> oracle_proxy_;
@@ -551,10 +567,13 @@ WebContentsCaptureMachine::WebContentsCaptureMachine(int render_process_id,
                                                      int render_view_id)
     : initial_render_process_id_(render_process_id),
       initial_render_view_id_(render_view_id),
-      render_thread_("WebContentsVideo_RenderThread"),
+      render_thread_(new base::Thread("WebContentsVideo_RenderThread")),
       fullscreen_widget_id_(MSG_ROUTING_NONE) {}
 
-WebContentsCaptureMachine::~WebContentsCaptureMachine() {}
+WebContentsCaptureMachine::~WebContentsCaptureMachine() {
+  BrowserThread::GetBlockingPool()->PostTask(FROM_HERE,

[Alpha Left Google, 2014/01/09 01:37:07]

This shouldn't be necessary. The thread has already been stopped. If not that's
a problem we would like to know.

[imcheng, 2014/01/09 23:31:09]

See comment below

+      base::Bind(&DeleteOnWorkerThread, base::Passed(&render_thread_)));
+}
 
 bool WebContentsCaptureMachine::Start(
     const scoped_refptr<ThreadSafeCaptureOracle>& oracle_proxy) {
@@ -564,7 +583,7 @@ bool WebContentsCaptureMachine::Start(
   DCHECK(oracle_proxy.get());
   oracle_proxy_ = oracle_proxy;
 
-  if (!render_thread_.Start()) {
+  if (!render_thread_->Start()) {
     DVLOG(1) << "Failed to spawn render thread.";
     return false;
   }
@@ -576,14 +595,23 @@ bool WebContentsCaptureMachine::Start(
   return true;
 }
 
-void WebContentsCaptureMachine::Stop() {
+void WebContentsCaptureMachine::Stop(const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   subscription_.reset();
   if (web_contents()) {
     web_contents()->DecrementCapturerCount();
     Observe(NULL);
   }
-  render_thread_.Stop();
+
+  // TODO(imcheng): why is it needed?
+  // This shuts down the message loop but thread is not complete dead yet.
+  render_thread_->StopSoon();

[imcheng, 2014/01/09 01:15:17]

Not sure if needed? Seems the general advice is to avoid using StopSoon() as it
was created as a very specific workaround for a Windows issue.

[Alpha Left Google, 2014/01/09 01:37:07]

That's fine if we don't call this.

[imcheng, 2014/01/09 23:31:09]

ACK, removed.

+
+  // The render thread cannot be stopped on the UI thread, so post a message
+  // to the thread pool used for blocking operations.
+  BrowserThread::GetBlockingPool()->PostTask(FROM_HERE, 
+      base::Bind(&StopOnWorkerThread, render_thread_.get(), callback));

[Alpha Left Google, 2014/01/09 01:37:07]

Just use &render_thread_ instead of render_thread_.get().

[imcheng, 2014/01/09 23:31:09]

I am a bit concerned about render_thread_ being invalid by the time the task is
being executed if:
- we don't call post task to call DeleteOnWorkerThread during
~WebContentsCaptureMachine()
- render_thread_ is simply a base::Thread as opposed to a scoped_ptr that can be
passed

If the sequence of events go like the following:
WebContentsCaptureMachine::Stop()
 - this posts a task to the blocking pool to call StopOnWorkerThread
~WebContentsCaptureMachine()
 - if task hasn't executed, this will try to stop render_thread_ on UI thread
which is invalid. Even if it worked, the task would be holding on to an invalid
pointer. So it seems like a race condition here.

+
   started_ = false;
 }
 
@@ -710,7 +738,7 @@ void WebContentsCaptureMachine::DidCopyFromBackingStore(
     UMA_HISTOGRAM_TIMES("TabCapture.CopyTimeBitmap", now - start_time);
     TRACE_EVENT_ASYNC_STEP_INTO0("mirroring", "Capture", target.get(),
                                  "Render");
-    render_thread_.message_loop_proxy()->PostTask(FROM_HERE, base::Bind(
+    render_thread_->message_loop_proxy()->PostTask(FROM_HERE, base::Bind(
         &RenderVideoFrame, bitmap, target,
         base::Bind(deliver_frame_cb, start_time)));
   } else {