Fixing a crash in MediaCustomControlsFullscreenDetector when the page is destroyed

There's a situation that hits a DCHECK() in
MediaCustomControlsFullscreenDetector when the video become
fullscreen but the page navigates away before the timer fires.
This CL solves the issue by stopping the timer when the
ExecutionContext is destroyed.

This CL also moves the common parts of MockWebMediaPlayer into
EmptyWebMediaPlayer. There are still some other
MockWebMediaPlayers which will be adopted in a follow-up.

BUG=698034

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

third_party/WebKit/Source/core/BUILD.gn

@@ -1234,6 +1234,7 @@ source_set("unit_tests") {
     "html/HTMLInputElementTest.cpp",
     "html/HTMLLinkElementSizesAttributeTest.cpp",
     "html/HTMLLinkElementTest.cpp",
+    "html/HTMLMediaElementEventListenersTest.cpp",
     "html/HTMLMediaElementTest.cpp",
     "html/HTMLOutputElementTest.cpp",
     "html/HTMLSelectElementTest.cpp",
@@ -1265,7 +1266,6 @@ source_set("unit_tests") {
     "html/parser/HTMLTreeBuilderSimulatorTest.cpp",
     "html/parser/HTMLViewSourceParserTest.cpp",
     "html/parser/TextResourceDecoderTest.cpp",
-    "html/shadow/MediaControlsLeakTest.cpp",
     "html/shadow/MediaControlsOrientationLockDelegateTest.cpp",
     "html/shadow/MediaControlsTest.cpp",
     "html/track/TextTrackListTest.cpp",

third_party/WebKit/Source/core/html/HTMLMediaElement.h

@@ -735,6 +735,7 @@ class CORE_EXPORT HTMLMediaElement
   friend class TrackDisplayUpdateScope;
   friend class MediaControlsTest;
   friend class HTMLMediaElementTest;
+  friend class HTMLMediaElementEventListenersTest;
   friend class HTMLVideoElementTest;
   friend class MediaControlsOrientationLockDelegateTest;
 

third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp

+// Copyright 2017 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.
+
+#include "core/html/HTMLMediaElement.h"
+
+#include "core/dom/DocumentUserGestureToken.h"
+#include "core/dom/Fullscreen.h"
+#include "core/html/HTMLVideoElement.h"
+#include "core/html/MediaCustomControlsFullscreenDetector.h"
+#include "core/html/shadow/MediaControls.h"
+#include "core/loader/EmptyClients.h"
+#include "core/testing/DummyPageHolder.h"
+#include "platform/UserGestureIndicator.h"
+#include "platform/testing/EmptyWebMediaPlayer.h"
+#include "platform/testing/UnitTestHelpers.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace blink {
+
+namespace {
+
+class MockWebMediaPlayer final : public EmptyWebMediaPlayer {
+ public:
+  MOCK_METHOD1(setIsEffectivelyFullscreen, void(bool));
+};
+
+class StubLocalFrameClient : public EmptyLocalFrameClient {
+ public:
+  static StubLocalFrameClient* create() { return new StubLocalFrameClient; }
+
+  std::unique_ptr<WebMediaPlayer> createWebMediaPlayer(
+      HTMLMediaElement&,
+      const WebMediaPlayerSource&,
+      WebMediaPlayerClient*) override {
+    return WTF::wrapUnique(new MockWebMediaPlayer());
+  }
+};
+
+using ::testing::_;
+using ::testing::Invoke;
+
+}  // anonymous namespace
+
+class HTMLMediaElementEventListenersTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    m_pageHolder = DummyPageHolder::create(IntSize(800, 600), nullptr,
+                                           StubLocalFrameClient::create());
+  }
+
+  Document& document() { return m_pageHolder->document(); }
+  void destroyDocument() { m_pageHolder.reset(); }
+  HTMLVideoElement* video() {
+    return toHTMLVideoElement(document().querySelector("video"));
+  }
+  MockWebMediaPlayer* webMediaPlayer() {
+    return static_cast<MockWebMediaPlayer*>(video()->webMediaPlayer());
+  }
+  MediaControls* controls() { return video()->mediaControls(); }
+  void simulateReadyState(HTMLMediaElement::ReadyState state) {
+    video()->setReadyState(state);
+  }
+  MediaCustomControlsFullscreenDetector* fullscreenDetector() {
+    return video()->m_customControlsFullscreenDetector;
+  }
+  bool isCheckViewportIntersectionTimerActive(
+      MediaCustomControlsFullscreenDetector* detector) {
+    return detector->m_checkViewportIntersectionTimer.isActive();
+  }
+
+ private:
+  std::unique_ptr<DummyPageHolder> m_pageHolder;
+};
+
+TEST_F(HTMLMediaElementEventListenersTest, RemovingFromDocumentCollectsAll) {
+  EXPECT_EQ(video(), nullptr);
+  document().body()->setInnerHTML("<body><video controls></video></body>");
+  EXPECT_NE(video(), nullptr);
+  EXPECT_TRUE(video()->hasEventListeners());
+  EXPECT_NE(controls(), nullptr);
+  EXPECT_TRUE(document().hasEventListeners());
+
+  WeakPersistent<HTMLVideoElement> weakPersistentVideo = video();
+  WeakPersistent<MediaControls> weakPersistentControls = controls();
+  {
+    Persistent<HTMLVideoElement> persistentVideo = video();
+    document().body()->setInnerHTML("");
+
+    // When removed from the document, the event listeners should have been
+    // dropped.
+    EXPECT_FALSE(document().hasEventListeners());
+    // The video element should still have some event listeners.
+    EXPECT_TRUE(persistentVideo->hasEventListeners());
+  }
+
+  ThreadState::current()->collectAllGarbage();
+
+  // They have been GC'd.
+  EXPECT_EQ(weakPersistentVideo, nullptr);
+  EXPECT_EQ(weakPersistentControls, nullptr);
+}
+
+TEST_F(HTMLMediaElementEventListenersTest,
+       ReInsertingInDocumentCollectsControls) {
+  EXPECT_EQ(video(), nullptr);
+  document().body()->setInnerHTML("<body><video controls></video></body>");
+  EXPECT_NE(video(), nullptr);
+  EXPECT_TRUE(video()->hasEventListeners());
+  EXPECT_NE(controls(), nullptr);
+  EXPECT_TRUE(document().hasEventListeners());
+
+  // This should be a no-op. We keep a reference on the VideoElement to avoid an
+  // unexpected GC.
+  {
+    Persistent<HTMLVideoElement> videoHolder = video();
+    document().body()->removeChild(video());
+    document().body()->appendChild(videoHolder.get());
+  }
+
+  EXPECT_TRUE(document().hasEventListeners());
+  EXPECT_TRUE(video()->hasEventListeners());
+
+  ThreadState::current()->collectAllGarbage();
+
+  EXPECT_NE(video(), nullptr);
+  EXPECT_NE(controls(), nullptr);
+  EXPECT_EQ(controls(), video()->mediaControls());
+}
+
+TEST_F(HTMLMediaElementEventListenersTest,
+       FullscreenDetectorTimerCancelledOnContextDestroy) {
+  EXPECT_EQ(video(), nullptr);
+  document().body()->setInnerHTML("<body><video></video</body>");
+  video()->setSrc("http://example.com");
+
+  testing::runPendingTasks();
+
+  EXPECT_NE(webMediaPlayer(), nullptr);
+
+  // Set ReadyState as HaveMetadata and go fullscreen, so the timer is fired.
+  EXPECT_NE(video(), nullptr);
+  simulateReadyState(HTMLMediaElement::kHaveMetadata);
+  UserGestureIndicator gestureIndicator(
+      DocumentUserGestureToken::create(&document()));
+  Fullscreen::requestFullscreen(*video());
+  Fullscreen::from(document()).didEnterFullscreen();
+
+  testing::runPendingTasks();
+
+  Persistent<Document> persistentDocument = &document();
+  Persistent<MediaCustomControlsFullscreenDetector> detector =
+      fullscreenDetector();
+
+  std::vector<bool> observedResults;
+
+  ON_CALL(*webMediaPlayer(), setIsEffectivelyFullscreen(_))
+      .WillByDefault(Invoke(
+          [&](bool isFullscreen) { observedResults.push_back(isFullscreen); }));
+
+  destroyDocument();
+
+  testing::runPendingTasks();
+
+  // Document should not have listeners as the ExecutionContext is destroyed.
+  EXPECT_FALSE(persistentDocument->hasEventListeners());
+  // The timer should be cancelled when the ExecutionContext is destroyed.
+  EXPECT_FALSE(isCheckViewportIntersectionTimerActive(detector));
+  // Should only notify the false value when ExecutionContext is destroyed.
+  EXPECT_EQ(1u, observedResults.size());
+  EXPECT_FALSE(observedResults[0]);
+}
+
+}  // namespace blink

third_party/WebKit/Source/core/html/HTMLVideoElement.h

@@ -123,6 +123,7 @@ class CORE_EXPORT HTMLVideoElement final : public HTMLMediaElement,
 
  private:
   friend class MediaCustomControlsFullscreenDetectorTest;
+  friend class HTMLMediaElementEventListenersTest;
 
   HTMLVideoElement(Document&);
 

third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp

@@ -11,8 +11,8 @@
 #include "core/testing/DummyPageHolder.h"
 #include "platform/UserGestureIndicator.h"
 #include "platform/network/NetworkStateNotifier.h"
+#include "platform/testing/EmptyWebMediaPlayer.h"
 #include "platform/testing/UnitTestHelpers.h"
-#include "public/platform/WebMediaPlayer.h"
 #include "public/platform/WebSize.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -22,43 +22,6 @@ namespace blink {
 
 namespace {
 
-class EmptyWebMediaPlayer : public WebMediaPlayer {
- public:
-  void load(LoadType, const WebMediaPlayerSource&, CORSMode) override{};
-  void play() override{};
-  void pause() override{};
-  bool supportsSave() const override { return false; };
-  void seek(double seconds) override{};
-  void setRate(double) override{};
-  void setVolume(double) override{};
-  WebTimeRanges buffered() const override { return WebTimeRanges(); };
-  WebTimeRanges seekable() const override { return WebTimeRanges(); };
-  void setSinkId(const WebString& sinkId,
-                 const WebSecurityOrigin&,
-                 WebSetSinkIdCallbacks*) override{};
-  bool hasVideo() const override { return false; };
-  bool hasAudio() const override { return false; };
-  WebSize naturalSize() const override { return WebSize(0, 0); };
-  bool paused() const override { return false; };
-  bool seeking() const override { return false; };
-  double duration() const override { return 0.0; };
-  double currentTime() const override { return 0.0; };
-  NetworkState getNetworkState() const override { return NetworkStateEmpty; };
-  ReadyState getReadyState() const override { return ReadyStateHaveNothing; };
-  WebString getErrorMessage() override { return WebString(); };
-  bool didLoadingProgress() override { return false; };
-  bool hasSingleSecurityOrigin() const override { return true; };
-  bool didPassCORSAccessCheck() const override { return true; };
-  double mediaTimeForTimeValue(double timeValue) const override {
-    return timeValue;
-  };
-  unsigned decodedFrameCount() const override { return 0; };
-  unsigned droppedFrameCount() const override { return 0; };
-  size_t audioDecodedByteCount() const override { return 0; };
-  size_t videoDecodedByteCount() const override { return 0; };
-  void paint(WebCanvas*, const WebRect&, PaintFlags&) override{};
-};
-
 class MockWebMediaPlayer : public EmptyWebMediaPlayer {
  public:
   MOCK_METHOD1(setBufferingStrategy, void(BufferingStrategy));

third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp

@@ -23,6 +23,7 @@ constexpr float kMostlyFillViewportThresholdOfVisibleProportion = 0.75f;
 MediaCustomControlsFullscreenDetector::MediaCustomControlsFullscreenDetector(
     HTMLVideoElement& video)
     : EventListener(CPPEventListenerType),
+      ContextLifecycleObserver(nullptr),
       m_videoElement(video),
       m_checkViewportIntersectionTimer(
           TaskRunnerHelper::get(TaskType::Unthrottled, &video.document()),
@@ -43,6 +44,7 @@ bool MediaCustomControlsFullscreenDetector::operator==(
 }
 
 void MediaCustomControlsFullscreenDetector::attach() {
+  setContext(&videoElement().document());
   videoElement().document().addEventListener(
       EventTypeNames::webkitfullscreenchange, this, true);
   videoElement().document().addEventListener(EventTypeNames::fullscreenchange,
@@ -50,10 +52,12 @@ void MediaCustomControlsFullscreenDetector::attach() {
 }
 
 void MediaCustomControlsFullscreenDetector::detach() {
+  setContext(nullptr);
   videoElement().document().removeEventListener(
       EventTypeNames::webkitfullscreenchange, this, true);
   videoElement().document().removeEventListener(
       EventTypeNames::fullscreenchange, this, true);
+  m_checkViewportIntersectionTimer.stop();
 }
 
 bool MediaCustomControlsFullscreenDetector::computeIsDominantVideoForTests(
@@ -126,6 +130,14 @@ void MediaCustomControlsFullscreenDetector::handleEvent(
                                                 BLINK_FROM_HERE);
 }
 
+void MediaCustomControlsFullscreenDetector::contextDestroyed(
+    ExecutionContext*) {
+  if (videoElement().webMediaPlayer())
+    videoElement().webMediaPlayer()->setIsEffectivelyFullscreen(false);
+
+  detach();
+}
+
 void MediaCustomControlsFullscreenDetector::
     onCheckViewportIntersectionTimerFired(TimerBase*) {
   DCHECK(isVideoOrParentFullscreen());
@@ -152,6 +164,7 @@ bool MediaCustomControlsFullscreenDetector::isVideoOrParentFullscreen() {
 
 DEFINE_TRACE(MediaCustomControlsFullscreenDetector) {
   EventListener::trace(visitor);
+  ContextLifecycleObserver::trace(visitor);
   visitor->trace(m_videoElement);
 }
 

third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h

@@ -6,6 +6,7 @@
 #define MediaCustomControlsFullscreenDetector_h
 
 #include "core/CoreExport.h"
+#include "core/dom/ContextLifecycleObserver.h"
 #include "core/events/EventListener.h"
 #include "platform/Timer.h"
 
@@ -16,7 +17,9 @@ class IntRect;
 class TimerBase;
 
 class CORE_EXPORT MediaCustomControlsFullscreenDetector final
-    : public EventListener {
+    : public EventListener,
+      public ContextLifecycleObserver {
+  USING_GARBAGE_COLLECTED_MIXIN(MediaCustomControlsFullscreenDetector);
   WTF_MAKE_NONCOPYABLE(MediaCustomControlsFullscreenDetector);
 
  public:
@@ -25,10 +28,14 @@ class CORE_EXPORT MediaCustomControlsFullscreenDetector final
   // EventListener implementation.
   bool operator==(const EventListener&) const override;
 
+  // ContextLifecycleObserver implemnetation.
+  void contextDestroyed(ExecutionContext*);
+
   DECLARE_VIRTUAL_TRACE();
 
  private:
   friend class MediaCustomControlsFullscreenDetectorTest;
+  friend class HTMLMediaElementEventListenersTest;
 
   // EventListener implementation.
   void handleEvent(ExecutionContext*, Event*) override;

third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp

-// Copyright 2017 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.
-
-#include "core/html/shadow/MediaControls.h"
-
-#include "core/dom/Document.h"
-#include "core/html/HTMLVideoElement.h"
-#include "core/testing/DummyPageHolder.h"
-#include "platform/geometry/IntSize.h"
-#include "platform/heap/ThreadState.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace blink {
-
-class MediaControlsLeakTest : public ::testing::Test {
- protected:
-  virtual void SetUp() {
-    m_pageHolder = DummyPageHolder::create(IntSize(800, 600));
-    Document& document = this->document();
-
-    document.write("<body><video controls></video></body>");
-    m_video = toHTMLVideoElement(document.querySelector("video"));
-    m_controls = m_video->mediaControls();
-  }
-
-  Document& document() { return m_pageHolder->document(); }
-  HTMLVideoElement* video() { return m_video; }
-  MediaControls* controls() { return m_controls; }
-
- private:
-  std::unique_ptr<DummyPageHolder> m_pageHolder;
-  WeakPersistent<HTMLVideoElement> m_video;
-  WeakPersistent<MediaControls> m_controls;
-};
-
-TEST_F(MediaControlsLeakTest, RemovingFromDocumentCollectsAll) {
-  ASSERT_NE(video(), nullptr);
-  EXPECT_TRUE(video()->hasEventListeners());
-  EXPECT_NE(controls(), nullptr);
-  EXPECT_TRUE(document().hasEventListeners());
-
-  document().body()->setInnerHTML("");
-
-  // When removed from the document, the event listeners should have been
-  // dropped.
-  EXPECT_FALSE(document().hasEventListeners());
-  // The video element should still have some event listeners.
-  EXPECT_TRUE(video()->hasEventListeners());
-
-  ThreadState::current()->collectAllGarbage();
-
-  // They have been GC'd.
-  EXPECT_EQ(video(), nullptr);
-  EXPECT_EQ(controls(), nullptr);
-}
-
-TEST_F(MediaControlsLeakTest, ReInsertingInDocumentCollectsControls) {
-  ASSERT_NE(video(), nullptr);
-  EXPECT_TRUE(video()->hasEventListeners());
-  EXPECT_NE(controls(), nullptr);
-  EXPECT_TRUE(document().hasEventListeners());
-
-  // This should be a no-op. We keep a reference on the VideoElement to avoid an
-  // unexpected GC.
-  {
-    Persistent<HTMLVideoElement> videoHolder = video();
-    document().body()->removeChild(video());
-    document().body()->appendChild(video());
-  }
-
-  EXPECT_TRUE(document().hasEventListeners());
-  EXPECT_TRUE(video()->hasEventListeners());
-
-  ThreadState::current()->collectAllGarbage();
-
-  ASSERT_NE(video(), nullptr);
-  EXPECT_NE(controls(), nullptr);
-  EXPECT_EQ(controls(), video()->mediaControls());
-}
-
-}  // namespace blink

third_party/WebKit/Source/platform/BUILD.gn

@@ -1569,6 +1569,8 @@ static_library("test_support") {
     "scroll/ScrollbarTestSuite.h",
     "testing/CompositorTest.cpp",
     "testing/CompositorTest.h",
+    "testing/EmptyWebMediaPlayer.cpp",
+    "testing/EmptyWebMediaPlayer.h",
     "testing/FakeDisplayItemClient.h",
     "testing/FakeGraphicsLayer.h",
     "testing/FakeGraphicsLayerClient.h",

third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.cpp

+// Copyright 2017 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.
+
+#include "platform/testing/EmptyWebMediaPlayer.h"
+
+#include "public/platform/WebSize.h"
+#include "public/platform/WebTimeRange.h"
+
+namespace blink {
+
+WebTimeRanges EmptyWebMediaPlayer::buffered() const {
+  return WebTimeRanges();
+}
+
+WebTimeRanges EmptyWebMediaPlayer::seekable() const {
+  return WebTimeRanges();
+}
+
+WebSize EmptyWebMediaPlayer::naturalSize() const {
+  return WebSize(0, 0);
+}
+
+}  // namespace blink

third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h

+// Copyright 2017 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.
+
+#include "public/platform/WebMediaPlayer.h"
+
+#ifndef EmptyWebMediaPlayer_h
+#define EmptyWebMediaPlayer_h
+
+namespace blink {
+
+// An empty WebMediaPlayer used only for tests. This class defines the methods
+// of WebMediaPlayer so that mock WebMediaPlayers don't need to redefine them if
+// they don't care their behavior.
+class EmptyWebMediaPlayer : public WebMediaPlayer {
+ public:
+  ~EmptyWebMediaPlayer() override {}
+
+  void load(LoadType, const WebMediaPlayerSource&, CORSMode) override {}
+  void play() override {}
+  void pause() override {}
+  bool supportsSave() const override { return false; }
+  void seek(double seconds) override {}
+  void setRate(double) override {}
+  void setVolume(double) override {}
+  WebTimeRanges buffered() const override;
+  WebTimeRanges seekable() const override;
+  void setSinkId(const WebString& sinkId,
+                 const WebSecurityOrigin&,
+                 WebSetSinkIdCallbacks*) override {}
+  bool hasVideo() const override { return false; }
+  bool hasAudio() const override { return false; }
+  WebSize naturalSize() const override;
+  bool paused() const override { return false; }
+  bool seeking() const override { return false; }
+  double duration() const override { return 0.0; }
+  double currentTime() const override { return 0.0; }
+  NetworkState getNetworkState() const override { return NetworkStateEmpty; }
+  ReadyState getReadyState() const override { return ReadyStateHaveNothing; }
+  WebString getErrorMessage() override { return WebString(); }
+  bool didLoadingProgress() override { return false; }
+  bool hasSingleSecurityOrigin() const override { return true; }
+  bool didPassCORSAccessCheck() const override { return true; }
+  double mediaTimeForTimeValue(double timeValue) const override {
+    return timeValue;
+  };
+  unsigned decodedFrameCount() const override { return 0; }
+  unsigned droppedFrameCount() const override { return 0; }
+  size_t audioDecodedByteCount() const override { return 0; }
+  size_t videoDecodedByteCount() const override { return 0; }
+  void paint(WebCanvas*, const WebRect&, cc::PaintFlags&) override {}
+};
+
+}  // namespace blink
+
+#endif  // EmptyWebMediaPlayer_h