MediaControls: Disconnect observers when controls are hidden

Disconnect ResizeObserver from media controls when the controls are
hidden. Otherwise, the obervsers are have pending activities (are
observed) which makes them roots for the object graph, ultimately
keeping elements (including e.g. HTMLVideoElement) and nodes alive.

Bug: 969049
Change-Id: I07a4470170f187631db307188f6cbe0052aa7d8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677053
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672629}

third_party/blink/renderer/modules/media_controls/elements/media_control_slider_element.cc

@@ -91,7 +91,7 @@ MediaControlSliderElement::MediaControlSliderElement(
               this))) {
   setType(input_type_names::kRange);
   setAttribute(html_names::kStepAttr, "any");
-  resize_observer_->observe(this);
+  OnControlsShown();
 }
 
 Element& MediaControlSliderElement::GetTrackElement() {
@@ -170,4 +170,12 @@ void MediaControlSliderElement::Trace(blink::Visitor* visitor) {
   MediaControlInputElement::Trace(visitor);
 }
 
+void MediaControlSliderElement::OnControlsShown() {
+  resize_observer_->observe(this);
+}
+
+void MediaControlSliderElement::OnControlsHidden() {
+  resize_observer_->disconnect();
+}
+
 }  // namespace blink

third_party/blink/renderer/modules/media_controls/elements/media_control_slider_element.h

@@ -33,6 +33,9 @@ class MODULES_EXPORT MediaControlSliderElement
   // simplicity; deliberately ignores pinch zoom's pageScaleFactor).
   int TrackWidth();
 
+  void OnControlsShown();
+  void OnControlsHidden();
+
  protected:
   friend class MediaControlsImplTest;
 

third_party/blink/renderer/modules/media_controls/elements/media_control_timeline_element.cc

@@ -271,10 +271,12 @@ void MediaControlTimelineElement::OnControlsHidden() {
 
   // End scrubbing state.
   is_touching_ = false;
+  MediaControlSliderElement::OnControlsHidden();
 }
 
 void MediaControlTimelineElement::OnControlsShown() {
   controls_hidden_ = false;
+  MediaControlSliderElement::OnControlsShown();
 }
 
 bool MediaControlTimelineElement::EndScrubbingEvent(Event& event) {

third_party/blink/renderer/modules/media_controls/media_controls_impl.cc

@@ -938,6 +938,7 @@ void MediaControlsImpl::MaybeShow() {
     loading_panel_->OnControlsShown();
 
   timeline_->OnControlsShown();
+  volume_slider_->OnControlsShown();
   UpdateCSSClassFromState();
   UpdateActingAsAudioControls();
 }
@@ -961,6 +962,7 @@ void MediaControlsImpl::Hide() {
     EndScrubbing();
   }
   timeline_->OnControlsHidden();
+  volume_slider_->OnControlsHidden();
 
   UpdateCSSClassFromState();
 

third_party/blink/renderer/modules/media_controls/media_controls_impl_test.cc

@@ -13,10 +13,12 @@
 #include "third_party/blink/public/platform/web_mouse_event.h"
 #include "third_party/blink/public/platform/web_screen_info.h"
 #include "third_party/blink/public/platform/web_size.h"
+#include "third_party/blink/renderer/bindings/core/v8/v8_gc_controller.h"
 #include "third_party/blink/renderer/core/css/css_property_value_set.h"
 #include "third_party/blink/renderer/core/css/document_style_environment_variables.h"
 #include "third_party/blink/renderer/core/css/style_engine.h"
 #include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/core/dom/document_parser.h"
 #include "third_party/blink/renderer/core/dom/dom_token_list.h"
 #include "third_party/blink/renderer/core/dom/element_traversal.h"
 #include "third_party/blink/renderer/core/dom/events/event.h"
@@ -1135,6 +1137,33 @@ TEST_F(MediaControlsImplTest,
   EXPECT_EQ(nullptr, weak_persistent_video);
 }
 
+TEST_F(MediaControlsImplTest,
+       RemovingFromDocumentWhenResettingSrcAllowsReclamation) {
+  // Regression test: https://crbug.com/918064
+  //
+  // Test ensures that unified heap garbage collections are able to collect
+  // detached HTMLVideoElements. The tricky part is that ResizeObserver's are
+  // treated as roots as long as they have observations which prevent the video
+  // element from being collected.
+
+  auto page_holder = std::make_unique<DummyPageHolder>();
+  page_holder->GetDocument().write("<video controls>");
+  page_holder->GetDocument().Parser()->Finish();
+
+  HTMLVideoElement& video =
+      ToHTMLVideoElement(*page_holder->GetDocument().QuerySelector("video"));
+  WeakPersistent<HTMLMediaElement> weak_persistent_video = &video;
+  video.remove();
+
+  test::RunPendingTasks();
+
+  // Needs to call into V8's GC here to trigger a unified garbage collection.
+  V8GCController::CollectAllGarbageForTesting(
+      ThreadState::Current()->GetIsolate(),
+      v8::EmbedderHeapTracer::EmbedderStackState::kEmpty);
+  EXPECT_EQ(nullptr, weak_persistent_video);
+}
+
 TEST_F(MediaControlsImplTest,
        ReInsertingInDocumentRestoresListenersAndCallbacks) {
   auto page_holder = std::make_unique<DummyPageHolder>();