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

 // 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.
 //
 // Implementation notes: This needs to work on a variety of hardware
 // configurations where the speed of the CPU and GPU greatly affect overall
 // performance. Spanning several threads, the process of capturing has been
 // split up into four conceptual stages:
 //
 //   1. Reserve Buffer: Before a frame can be captured, a slot in the client's
@@ skipping 90 matching lines @@
 
 // Wrapper function to invoke ThreadSafeCaptureOracle::CaptureFrameCallback, is
 // compatible with RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback.
 void InvokeCaptureFrameCallback(
     const ThreadSafeCaptureOracle::CaptureFrameCallback& capture_frame_cb,
     base::TimeTicks timestamp,
     bool frame_captured) {
   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 {
  public:
   FrameSubscriber(VideoCaptureOracle::Event event_type,
                   const scoped_refptr<ThreadSafeCaptureOracle>& oracle)
       : event_type_(event_type),
         oracle_proxy_(oracle) {}
 
   virtual bool ShouldCaptureFrame(
@@ skipping 83 matching lines @@
     : public VideoCaptureMachine,
       public WebContentsObserver,
       public base::SupportsWeakPtr<WebContentsCaptureMachine> {
  public:
   WebContentsCaptureMachine(int render_process_id, int render_view_id);
   virtual ~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
   // completes. The copy will occur to |target|.
   //
   // This may be used as a ContentCaptureSubscription::CaptureCallback.
   void Capture(const base::TimeTicks& start_time,
                const scoped_refptr<media::VideoFrame>& target,
                const RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback&
                    deliver_frame_cb);
@@ skipping 53 matching lines @@
   // after any change to the WebContents that affects the RenderWidgetHost or
   // attached views.
   void RenewFrameSubscription();
 
   // Parameters saved in constructor.
   const int initial_render_process_id_;
   const int initial_render_view_id_;
 
   // 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_;
 
   // Routing ID of any active fullscreen render widget or MSG_ROUTING_NONE
   // otherwise.
   int fullscreen_widget_id_;
 
   // Last known RenderView size.
   gfx::Size last_view_size_;
@@ skipping 245 matching lines @@
       count_frames_rendered_ = 0;
       last_frame_number_ = frame_number;
     }
   }
 }
 
 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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!started_);
 
   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;
   }
 
   if (!StartObservingWebContents())
     return false;
 
   started_ = true;
   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;
 }
 
 void WebContentsCaptureMachine::Capture(
     const base::TimeTicks& start_time,
     const scoped_refptr<media::VideoFrame>& target,
     const RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback&
         deliver_frame_cb) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ skipping 106 matching lines @@
         deliver_frame_cb,
     bool success,
     const SkBitmap& bitmap) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   base::TimeTicks now = base::TimeTicks::Now();
   if (success) {
     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 {
     // Capture can fail due to transient issues, so just skip this frame.
     DVLOG(1) << "CopyFromBackingStore failed; skipping frame.";
     deliver_frame_cb.Run(start_time, false);
   }
 }
 
 void WebContentsCaptureMachine::DidCopyFromCompositingSurfaceToVideoFrame(
@@ skipping 57 matching lines @@
     scoped_ptr<Client> client) {
   DVLOG(1) << "Allocating " << params.requested_format.frame_size.ToString();
   impl_->AllocateAndStart(params, client.Pass());
 }
 
 void WebContentsVideoCaptureDevice::StopAndDeAllocate() {
   impl_->StopAndDeAllocate();
 }
 
 }  // namespace content