Handle early destruction of CanvasCaptureHandler

Fuzz testing showed that CanvasCaptureHandler can be destructed
earlier than CanvasCaptureHandler::VideoCapturerSource. Both instances
are owned by Blink side objects, and destruction sequence might be
different(oilpan).

CanvasCaptureHandler invalidates weakptrs in dtor() on main_render_thread.
We can check if weakptr is valid in StopCapture() that also runs
on main_render_thread.

BUG=597077
TEST=Added unittest "DestructHandler" to reproduce the fuzz case.

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

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

3 files changed, 25 insertions, 12 deletions

diff --git a/content/renderer/media/canvas_capture_handler.cc b/content/renderer/media/canvas_capture_handler.cc
index fd6e55f..6056712 100644
--- a/content/renderer/media/canvas_capture_handler.cc
+++ b/content/renderer/media/canvas_capture_handler.cc
@@ -23,8 +23,11 @@
 
 namespace content {
 
-class CanvasCaptureHandler::VideoCapturerSource
-    : public media::VideoCapturerSource {
+// Implementation VideoCapturerSource that is owned by
+// MediaStreamVideoCapturerSource and delegates the Start/Stop calls to
+// CanvasCaptureHandler.
+// This class is single threaded and pinned to main render thread.
+class VideoCapturerSource : public media::VideoCapturerSource {
  public:
   explicit VideoCapturerSource(base::WeakPtr<CanvasCaptureHandler>
                                    canvas_handler,
@@ -58,15 +61,16 @@ class CanvasCaptureHandler::VideoCapturerSource
   }
   void StopCapture() override {
     DCHECK(main_render_thread_checker_.CalledOnValidThread());
-    canvas_handler_->StopVideoCapture();
+    if (canvas_handler_.get())
+      canvas_handler_->StopVideoCapture();
   }
 
  private:
-  double frame_rate_;
+  const double frame_rate_;
   // Bound to Main Render thread.
   base::ThreadChecker main_render_thread_checker_;
-  // CanvasCaptureHandler is owned by CanvasDrawListener in blink and
-  // guaranteed to be alive during the lifetime of this class.
+  // CanvasCaptureHandler is owned by CanvasDrawListener in blink and might be
+  // destroyed before StopCapture() call.
   base::WeakPtr<CanvasCaptureHandler> canvas_handler_;
 };
 
@@ -113,8 +117,7 @@ CanvasCaptureHandler::CanvasCaptureHandler(
       io_task_runner_(io_task_runner),
       weak_ptr_factory_(this) {
   scoped_ptr<media::VideoCapturerSource> video_source(
-      new CanvasCaptureHandler::VideoCapturerSource(
-          weak_ptr_factory_.GetWeakPtr(), frame_rate));
+      new VideoCapturerSource(weak_ptr_factory_.GetWeakPtr(), frame_rate));
   AddVideoCapturerSourceToVideoTrack(std::move(video_source), track);
 }
 
diff --git a/content/renderer/media/canvas_capture_handler.h b/content/renderer/media/canvas_capture_handler.h
index a4bbec4..dff93b4 100644
--- a/content/renderer/media/canvas_capture_handler.h
+++ b/content/renderer/media/canvas_capture_handler.h
@@ -72,10 +72,6 @@ class CONTENT_EXPORT CanvasCaptureHandler final
       scoped_ptr<media::VideoCapturerSource> source,
       blink::WebMediaStreamTrack* web_track);
 
-  // Implementation VideoCapturerSource that is owned by Blink and delegates
-  // the Start/Stop calls to CanvasCaptureHandler.
-  class VideoCapturerSource;
-
   // Object that does all the work of running |new_frame_callback_|.
   // Destroyed on |frame_callback_task_runner_| after the class is destroyed.
   class CanvasCaptureHandlerDelegate;
diff --git a/content/renderer/media/canvas_capture_handler_unittest.cc b/content/renderer/media/canvas_capture_handler_unittest.cc
index e17ffc6..d4665ca 100644
--- a/content/renderer/media/canvas_capture_handler_unittest.cc
+++ b/content/renderer/media/canvas_capture_handler_unittest.cc
@@ -142,6 +142,20 @@ TEST_F(CanvasCaptureHandlerTest, ConstructAndDestruct) {
   base::RunLoop().RunUntilIdle();
 }
 
+// Checks that the destruction sequence works fine.
+TEST_F(CanvasCaptureHandlerTest, DestructTrack) {
+  EXPECT_TRUE(canvas_capture_handler_->needsNewFrame());
+  track_.reset();
+  base::RunLoop().RunUntilIdle();
+}
+
+// Checks that the destruction sequence works fine.
+TEST_F(CanvasCaptureHandlerTest, DestructHandler) {
+  EXPECT_TRUE(canvas_capture_handler_->needsNewFrame());
+  canvas_capture_handler_.reset();
+  base::RunLoop().RunUntilIdle();
+}
+
 // Checks that VideoCapturerSource call sequence works fine.
 TEST_P(CanvasCaptureHandlerTest, GetFormatsStartAndStop) {
   InSequence s;