Prevent reentrant cleanup events for idle delegates.

Crash stacks show nested calls into the cleanup method, we shouldn't
invoke such cases. It may be worth investigating how much overhead
shotgunning a bunch of post tasks adds here and if that is too much
on JellyBean.

BUG=650663
TEST=new unittest

Review-Url: https://codereview.chromium.org/2367253007
Cr-Commit-Position: refs/heads/master@{#421347}

content/renderer/media/renderer_webmediaplayer_delegate.cc

@@ -220,9 +220,16 @@ void RendererWebMediaPlayerDelegate::RemoveIdleDelegate(int delegate_id) {
 
 void RendererWebMediaPlayerDelegate::CleanupIdleDelegates(
     base::TimeDelta timeout) {
+  // Drop reentrant cleanups which can occur during forced suspension when the
+  // number of idle delegates is too high for a given device.
+  if (idle_cleanup_running_)
+    return;
+
   // Iterate over the delegates and suspend the idle ones. Note: The call to
-  // OnHidden() can trigger calls into RemoveIdleDelegate(), so for iterator
-  // validity we set |idle_cleanup_running_| to true and defer deletions.
+  // OnSuspendRequested() can trigger calls into RemoveIdleDelegate(), so for
+  // iterator validity we set |idle_cleanup_running_| to true and defer
+  // deletions.
+  DCHECK(!idle_cleanup_running_);
   base::AutoReset<bool> scoper(&idle_cleanup_running_, true);
   const base::TimeTicks now = tick_clock_->NowTicks();
   for (auto& idle_delegate_entry : idle_delegate_map_) {

content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc

@@ -248,6 +248,44 @@ TEST_F(RendererWebMediaPlayerDelegateTest, MaxLowEndIdleDelegates) {
   run_loop.Run();
 }
 
+TEST_F(RendererWebMediaPlayerDelegateTest, ReentrantIdleDelegates) {
+  // Start the tick clock off at a non-null value.
+  base::SimpleTestTickClock tick_clock;
+  tick_clock.Advance(base::TimeDelta::FromSeconds(1234));
+
+  const base::TimeDelta kIdleTimeout = base::TimeDelta::FromSeconds(10);
+  delegate_manager_->SetIdleCleanupParamsForTesting(kIdleTimeout, &tick_clock,
+                                                    true);
+  EXPECT_FALSE(delegate_manager_->IsIdleCleanupTimerRunningForTesting());
+
+  // Add two observers, both of which should keep the idle timer running.
+  testing::StrictMock<MockWebMediaPlayerDelegateObserver> observer_1;
+  const int delegate_id_1 = delegate_manager_->AddObserver(&observer_1);
+  EXPECT_TRUE(delegate_manager_->IsIdleCleanupTimerRunningForTesting());
+
+  testing::StrictMock<MockWebMediaPlayerDelegateObserver> observer_2;
+  const int delegate_id_2 = delegate_manager_->AddObserver(&observer_2);
+  EXPECT_TRUE(delegate_manager_->IsIdleCleanupTimerRunningForTesting());
+
+  testing::StrictMock<MockWebMediaPlayerDelegateObserver> observer_3;
+  EXPECT_CALL(observer_1, OnSuspendRequested(false))
+      .WillOnce(RunClosure(base::Bind(&RendererWebMediaPlayerDelegate::DidPause,
+                                      base::Unretained(delegate_manager_.get()),
+                                      delegate_id_1, false)));
+  EXPECT_CALL(observer_2, OnSuspendRequested(false))
+      .WillOnce(RunClosure(base::Bind(&RendererWebMediaPlayerDelegate::DidPause,
+                                      base::Unretained(delegate_manager_.get()),
+                                      delegate_id_2, false)));
+  base::RunLoop run_loop;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                run_loop.QuitClosure());
+  tick_clock.Advance(base::TimeDelta::FromMicroseconds(1));
+  // Adding the third observer should force idle cleanup.
+  delegate_manager_->AddObserver(&observer_3);
+  EXPECT_TRUE(delegate_manager_->IsIdleCleanupTimerRunningForTesting());
+  run_loop.Run();
+}
+
 TEST_F(RendererWebMediaPlayerDelegateTest, IdleDelegatesAreSuspended) {
   // Start the tick clock off at a non-null value.
   base::SimpleTestTickClock tick_clock;