Fixed and re-enabled unit tests for WebContentsVideoCaptureDevice.

1. Added mechanism to synchronize unit test tear-down with objects being destroyed on other threads.
2. Added locking around StubRenderWidgetHost::color_ in test code.
3. Re-enabled tests and removed relevant suppressions.

BUG=158317
TEST=Ran TSAN and Heapcheck with no errors.  Ran debug Chromium build with --vmodule=web_contents_video_capture_device=2 and confirmed no breakage in functionality.


Review URL: https://chromiumcodereview.appspot.com/11336027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@165327 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 43 insertions, 38 deletions

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 376f8be..cfb1e8e 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
@@ -606,8 +606,11 @@ class CaptureMachine
  public:
   CaptureMachine(int render_process_id, int render_view_id);
 
-  // Set capture source to the given |override| for unit testing.
-  void SetRenderWidgetHostForTesting(RenderWidgetHost* override);
+  // Sets the capture source to the given |override| for unit testing.
+  // Also, |destroy_cb| will be invoked after CaptureMachine is fully destroyed
+  // (to synchronize tear-down).
+  void InitializeForTesting(RenderWidgetHost* override,
+                            const base::Closure& destroy_cb);
 
   // Synchronously sets/unsets the consumer.  Pass |consumer| as NULL to remove
   // the reference to the consumer; then, once this method returns,
@@ -678,6 +681,8 @@ class CaptureMachine
   VideoFrameRenderer renderer_;
   VideoFrameDeliverer deliverer_;
 
+  base::Closure destroy_cb_;  // Invoked once CaptureMachine is destroyed.
+
   DISALLOW_COPY_AND_ASSIGN(CaptureMachine);
 };
 
@@ -691,9 +696,10 @@ CaptureMachine::CaptureMachine(int render_process_id, int render_view_id)
   manager_thread_.Start();
 }
 
-void CaptureMachine::SetRenderWidgetHostForTesting(
-    RenderWidgetHost* override) {
+void CaptureMachine::InitializeForTesting(RenderWidgetHost* override,
+                                          const base::Closure& destroy_cb) {
   copier_.SetRenderWidgetHostForTesting(override);
+  destroy_cb_ = destroy_cb;
 }
 
 void CaptureMachine::SetConsumer(
@@ -806,8 +812,12 @@ void CaptureMachine::Destruct(const CaptureMachine* x) {
 
 // static
 void CaptureMachine::DeleteFromOutsideThread(const CaptureMachine* x) {
+  const base::Closure run_after_delete = x->destroy_cb_;
   // Note: Thread joins are about to happen here (in ~CaptureThread()).
   delete x;
+  if (!run_after_delete.is_null()) {
+    run_after_delete.Run();
+  }
 }
 
 void CaptureMachine::TransitionStateTo(State next_state) {
@@ -947,11 +957,11 @@ WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
       capturer_(new CaptureMachine(render_process_id, render_view_id)) {}
 
 WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
-    RenderWidgetHost* test_source)
+    RenderWidgetHost* test_source, const base::Closure& destroy_cb)
     : capturer_(new CaptureMachine(-1, -1)) {
   device_name_.device_name = "WebContentsForTesting";
   device_name_.unique_id = "-1:-1";
-  capturer_->SetRenderWidgetHostForTesting(test_source);
+  capturer_->InitializeForTesting(test_source, destroy_cb);
 }
 
 WebContentsVideoCaptureDevice::~WebContentsVideoCaptureDevice() {
@@ -988,8 +998,8 @@ media::VideoCaptureDevice* WebContentsVideoCaptureDevice::Create(
 
 // static
 media::VideoCaptureDevice* WebContentsVideoCaptureDevice::CreateForTesting(
-    RenderWidgetHost* test_source) {
-  return new WebContentsVideoCaptureDevice(test_source);
+    RenderWidgetHost* test_source, const base::Closure& destroy_cb) {
+  return new WebContentsVideoCaptureDevice(test_source, destroy_cb);
 }
 
 void WebContentsVideoCaptureDevice::Allocate(
diff --git a/content/browser/renderer_host/media/web_contents_video_capture_device.h b/content/browser/renderer_host/media/web_contents_video_capture_device.h
index 56eb847..45d4c59 100644
--- a/content/browser/renderer_host/media/web_contents_video_capture_device.h
+++ b/content/browser/renderer_host/media/web_contents_video_capture_device.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "content/common/content_export.h"
 #include "media/video/capture/video_capture_device.h"
@@ -35,9 +36,12 @@ class CONTENT_EXPORT WebContentsVideoCaptureDevice
   static media::VideoCaptureDevice* Create(const std::string& device_id);
 
   // Construct an instance with the following |test_source| injected for testing
-  // purposes.
+  // purposes.  |destroy_cb| is invoked once all outstanding objects are
+  // completely destroyed.
+  // TODO(miu): Passing a destroy callback suggests needing to revisit the
+  // design philosophy of an asynchronous DeAllocate().  http://crbug.com/158641
   static media::VideoCaptureDevice* CreateForTesting(
-      RenderWidgetHost* test_source);
+      RenderWidgetHost* test_source, const base::Closure& destroy_cb);
 
   virtual ~WebContentsVideoCaptureDevice();
 
@@ -60,7 +64,8 @@ class CONTENT_EXPORT WebContentsVideoCaptureDevice
   // Constructors.  The latter is used for testing.
   WebContentsVideoCaptureDevice(
       const Name& name, int render_process_id, int render_view_id);
-  explicit WebContentsVideoCaptureDevice(RenderWidgetHost* test_source);
+  WebContentsVideoCaptureDevice(RenderWidgetHost* test_source,
+                                const base::Closure& destroy_cb);
 
   Name device_name_;
   scoped_refptr<CaptureMachine> capturer_;
diff --git a/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc b/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
index d1cfbb3..8e8afe7 100644
--- a/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
+++ b/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
@@ -4,7 +4,9 @@
 
 #include "content/browser/renderer_host/media/web_contents_video_capture_device.h"
 
+#include "base/bind_helpers.h"
 #include "base/synchronization/condition_variable.h"
+#include "base/synchronization/waitable_event.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/renderer_host/render_widget_host_delegate.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
@@ -35,6 +37,7 @@ class StubRenderWidgetHost : public RenderWidgetHostImpl {
         color_(kNothingYet) {}
 
   void SetSolidColor(SkColor color) {
+    base::AutoLock guard(lock_);
     color_ = color;
   }
 
@@ -48,6 +51,7 @@ class StubRenderWidgetHost : public RenderWidgetHostImpl {
     SkBitmap bitmap = output->GetBitmap();
     {
       SkAutoLockPixels locker(bitmap);
+      base::AutoLock guard(lock_);
       bitmap.eraseColor(color_);
     }
 
@@ -65,6 +69,7 @@ class StubRenderWidgetHost : public RenderWidgetHostImpl {
   };
 
   StubRenderWidgetHostDelegate delegate_;
+  base::Lock lock_;  // Guards changes to color_.
   SkColor color_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(StubRenderWidgetHost);
@@ -160,15 +165,22 @@ class WebContentsVideoCaptureDeviceTest : public testing::Test {
     browser_context_.reset(new TestBrowserContext());
     source_.reset(new StubRenderWidgetHost(
         new MockRenderProcessHost(browser_context_.get()), MSG_ROUTING_NONE));
+    destroyed_.reset(new base::WaitableEvent(true, false));
     device_.reset(WebContentsVideoCaptureDevice::CreateForTesting(
-        source_.get()));
+        source_.get(),
+        base::Bind(&base::WaitableEvent::Signal,
+                   base::Unretained(destroyed_.get()))));
     consumer_.reset(new StubConsumer);
   }
 
   virtual void TearDown() {
     // Tear down in opposite order of set-up.
+    device_->DeAllocate();  // Guarantees no more use of consumer_.
     consumer_.reset();
-    device_.reset();
+    device_.reset();  // Release reference to internal CaptureMachine.
+    message_loop_->RunAllPending();  // Just in case.
+    destroyed_->Wait();  // Wait until CaptureMachine is fully destroyed.
+    destroyed_.reset();
     source_.reset();
     browser_context_.reset();
     ui_thread_->Stop();
@@ -187,19 +199,17 @@ class WebContentsVideoCaptureDeviceTest : public testing::Test {
   scoped_ptr<BrowserThreadImpl> ui_thread_;
   scoped_ptr<TestBrowserContext> browser_context_;
   scoped_ptr<StubRenderWidgetHost> source_;
+  scoped_ptr<base::WaitableEvent> destroyed_;
   scoped_ptr<media::VideoCaptureDevice> device_;
   scoped_ptr<StubConsumer> consumer_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsVideoCaptureDeviceTest);
 };
 
-// TODO(miu): Fix test crashing and race conditions, then re-enable tests, per
-// http://crbug.com/158317.
-
 // The "happy case" test.  No scaling is needed, so we should be able to change
 // the picture emitted from the source and expect to see each delivered to the
 // consumer.
-TEST_F(WebContentsVideoCaptureDeviceTest, DISABLED_GoesThroughAllTheMotions) {
+TEST_F(WebContentsVideoCaptureDeviceTest, GoesThroughAllTheMotions) {
   device()->Allocate(kTestWidth, kTestHeight, kTestFramesPerSecond,
                      consumer());
 
@@ -216,8 +226,7 @@ TEST_F(WebContentsVideoCaptureDeviceTest, DISABLED_GoesThroughAllTheMotions) {
   device()->DeAllocate();
 }
 
-TEST_F(WebContentsVideoCaptureDeviceTest,
-       DISABLED_RejectsInvalidAllocateParams) {
+TEST_F(WebContentsVideoCaptureDeviceTest, RejectsInvalidAllocateParams) {
   device()->Allocate(1280, 720, -2, consumer());
   EXPECT_FALSE(consumer()->WaitForNextColorOrError(kNotInterested));
 }
diff --git a/tools/valgrind/gtest_exclude/content_unittests.gtest.txt b/tools/valgrind/gtest_exclude/content_unittests.gtest.txt
deleted file mode 100644
index 10791f2..0000000
--- a/tools/valgrind/gtest_exclude/content_unittests.gtest.txt
+++ /dev/null
@@ -1,3 +0,0 @@
-# These crash under TSan due to data races, might affect other tools as well.
-# See http://crbug.com/158317
-WebContentsVideoCaptureDeviceTest.*
diff --git a/tools/valgrind/tsan/suppressions.txt b/tools/valgrind/tsan/suppressions.txt
index c9553bf..f2f8073 100644
--- a/tools/valgrind/tsan/suppressions.txt
+++ b/tools/valgrind/tsan/suppressions.txt
@@ -1000,19 +1000,3 @@
    fun:safe_browsing::ClientSideDetectionHost::ShouldClassifyUrlRequest::CheckCsdWhitelist
    fun:base::internal::RunnableAdapter::Run
 }
-{
-   bug_158317_a
-   ThreadSanitizer:Race
-   ...
-   fun:content::::BackingStoreCopier::StartCopy
-   fun:base::internal::RunnableAdapter::Run
-}
-{
-   bug_158317_b
-   ThreadSanitizer:Race
-   ...
-   fun:content::RenderWidgetHostImpl::~RenderWidgetHostImpl
-   fun:StubRenderWidgetHost::~StubRenderWidgetHost
-   fun:scoped_ptr::reset
-   fun:WebContentsVideoCaptureDeviceTest::TearDown
-}