[Modern Media Controls] HTMLMediaElement is never destroyed when show…

…ing media controls

https://bugs.webkit.org/show_bug.cgi?id=270571

Reviewed by Xabier Rodriguez-Calvar.

At least in GStreamer-based ports (WPE and WebKitGTK, I haven't checked
on Mac ports because I don't have the proper environment easily
available), the media element is leaked after explicit deinitialization
and detaching from the HTML document, even after manually triggering
garbage collection (GC) using the web inspector.

See: #1285

After some debugging, we've detected that 2 extra references to
HTMLMediaElement appear when using the controls (3 refs in total), while
in a scenario where the controls are hidden on purpose only 1 reference
remains, which is released as soon as the GC kicks in.

Those references are held (or transitively held) by the MediaController,
the shadowRoot referenced by the MediaController and by the IconService,
and the <div> element that MediaController adds to the shadowRoot as a
container for the controls.

This commit adds code to use WeakRefs to the ShadowRoot and to the
MediaJSWrapper (media) on MediaController, and to the ShadowRoot on
the iconService, instead of the original objects. This allows the
garbage collector to kick in when needed and have those objects freed
automatically. The WeakRefs are transparently exposed as regular objects
by using properties (get/set), to avoid the need to change a lot of code
that expects regular objects.

There's still the <div> element added to the ShadowRoot, which
transitively holds a reference that prevents GC. There's no good place
to remove that element, so I removed it in the "less bad place", which
is HTMLMediaElement::pauseAfterDetachedTask(). A new deinitialize() JS
function takes care of that. Unfortunately, the element can still be
used after deinitialization, so there's also a method to reinitialize()
it if needed and an extra ControlsState to mark the element as
PartiallyDeinitialized in order to know when to recover from that state.

* Source/WebCore/Modules/modern-media-controls/controls/icon-service.js: Store shadowRoot as a WeakRef.
(const.iconService.new.IconService.prototype.get shadowRoot): Expose the shadowRootWeakRef as a regular shadowRoot object by binding it to the shadowRoot property.
(const.iconService.new.IconService.prototype.set shadowRoot): Ditto, but for the setter.
* Source/WebCore/Modules/modern-media-controls/media/media-controller.js:
(MediaController): Store shadowRoot and media as WeakRefs.
(MediaController.prototype.get media): Expose the mediaWeakRef as a regular media object by binding it to the media property.
(MediaController.prototype.get shadowRoot): Expose the shadowRootWeakRef as a regular shadowRoot object by binding it to the shadowRoot property.
(MediaController.prototype.get isFullscreen): Take into account the case where media can be null.
(MediaController.prototype.deinitialize): Function called from HTMLMediaElement to deinitialize the MediaController. Just removes the shadowRoot child.
(MediaController.prototype.reinitialize): Function called from HTMLMediaElement to reinitialize the MediaController. Readds the shadowRoot child and sets again the WeakRefs that might have become by lack of use of the main objects.
* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::convertEnumerationToString): Utility function to get a printable version of ControlsState. Useful for debugging.
(WebCore::HTMLMediaElement::pauseAfterDetachedTask): Deinitialize the MediaController by calling its deinitialize() JS method.
(WebCore::HTMLMediaElement::ensureMediaControls): Support the case of reinitialization. Call the reinitialize() JS method in MediaController in that case.
* Source/WebCore/html/HTMLMediaElement.h: Added new PartiallyDeinitialized state to ControlsState. Give friend access to convertEnumerationToString() so it can do its job.

Canonical link: https://commits.webkit.org/277196@main

Source/WebCore/Modules/modern-media-controls/controls/icon-service.js

@@ -74,6 +74,15 @@ const iconService = new class IconService {
     }
 
     // Public
+    get shadowRoot()
+    {
+        return this.shadowRootWeakRef ? this.shadowRootWeakRef.deref() : null;
+    }
+
+    set shadowRoot(shadowRoot)
+    {
+        this.shadowRootWeakRef = new WeakRef(shadowRoot);
+    }
 
     imageForIconAndLayoutTraits(icon, layoutTraits)
     {

Source/WebCore/Modules/modern-media-controls/media/media-controller.js

@@ -25,11 +25,10 @@
 
 class MediaController
 {
-
     constructor(shadowRoot, media, host)
     {
-        this.shadowRoot = shadowRoot;
-        this.media = media;
+        this.shadowRootWeakRef = new WeakRef(shadowRoot);
+        this.mediaWeakRef = new WeakRef(media);
         this.host = host;
 
         this.fullscreenChangeEventType = media.webkitSupportsPresentationMode ? "webkitpresentationmodechanged" : "webkitfullscreenchange";
@@ -65,6 +64,16 @@ class MediaController
     }
 
     // Public
+    get media()
+    {
+        return this.mediaWeakRef ? this.mediaWeakRef.deref() : null;
+    }
+
+    get shadowRoot()
+    {
+
+        return this.shadowRootWeakRef ? this.shadowRootWeakRef.deref() : null;
+    }
 
     get isAudio()
     {
@@ -91,6 +100,9 @@ class MediaController
 
     get isFullscreen()
     {
+        if (!this.media)
+            return false;
+
         return this.media.webkitSupportsPresentationMode ? this.media.webkitPresentationMode === "fullscreen" : this.media.webkitDisplayingFullscreen;
     }
 
@@ -265,6 +277,22 @@ class MediaController
         return true;
     }
 
+    deinitialize()
+    {
+        this.shadowRoot.removeChild(this.container);
+        return true;
+    }
+
+    reinitialize(shadowRoot, media, host)
+    {
+        iconService.shadowRoot = shadowRoot;
+        this.shadowRootWeakRef = new WeakRef(shadowRoot);
+        this.mediaWeakRef = new WeakRef(media);
+        this.host = host;
+        shadowRoot.appendChild(this.container);
+        return true;
+    }
+
     // Private
 
     _supportingObjectClasses()

Source/WebCore/html/HTMLMediaElement.cpp

@@ -311,6 +311,25 @@ String convertEnumerationToString(HTMLMediaElement::SpeechSynthesisState enumera
     return values[static_cast<size_t>(enumerationValue)];
 }
 
+String convertEnumerationToString(HTMLMediaElement::ControlsState enumerationValue)
+{
+    // None, Initializing, Ready, PartiallyDeinitialized
+    static const NeverDestroyed<String> values[] = {
+        MAKE_STATIC_STRING_IMPL("None"),
+        MAKE_STATIC_STRING_IMPL("Initializing"),
+        MAKE_STATIC_STRING_IMPL("Ready"),
+        MAKE_STATIC_STRING_IMPL("PartiallyDeinitialized"),
+    };
+    static_assert(!static_cast<size_t>(HTMLMediaElement::ControlsState::None), "HTMLMediaElement::None is not 0 as expected");
+    static_assert(static_cast<size_t>(HTMLMediaElement::ControlsState::Initializing) == 1, "HTMLMediaElement::Initializing is not 1 as expected");
+    static_assert(static_cast<size_t>(HTMLMediaElement::ControlsState::Ready) == 2, "HTMLMediaElement::Ready is not 2 as expected");
+    static_assert(static_cast<size_t>(HTMLMediaElement::ControlsState::PartiallyDeinitialized) == 3, "HTMLMediaElement::PartiallyDeinitialized is not 3 as expected");
+    ASSERT(static_cast<size_t>(enumerationValue) < std::size(values));
+    return values[static_cast<size_t>(enumerationValue)];
+}
+
+static JSC::JSValue controllerJSValue(JSC::JSGlobalObject& lexicalGlobalObject, JSDOMGlobalObject&, HTMLMediaElement&);
+
 class TrackDisplayUpdateScope {
 public:
     TrackDisplayUpdateScope(HTMLMediaElement& element)
@@ -920,6 +939,38 @@ void HTMLMediaElement::pauseAfterDetachedTask()
     if (m_videoFullscreenMode == VideoFullscreenModeStandard)
         exitFullscreen();
 
+#if ENABLE(MODERN_MEDIA_CONTROLS)
+    if (m_controlsState == ControlsState::Initializing || m_controlsState == ControlsState::Ready) {
+        // Call MediaController.deinitialize() to get rid of circular references.
+        bool isDeinitialized = setupAndCallJS([this](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController&, DOMWrapperWorld&) {
+            auto& vm = globalObject.vm();
+            auto scope = DECLARE_THROW_SCOPE(vm);
+
+            auto controllerValue = controllerJSValue(lexicalGlobalObject, globalObject, *this);
+            RETURN_IF_EXCEPTION(scope, false);
+            auto* controllerObject = controllerValue.toObject(&lexicalGlobalObject);
+            RETURN_IF_EXCEPTION(scope, false);
+
+            auto functionValue = controllerObject->get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "deinitialize"_s));
+            if (UNLIKELY(scope.exception()) || functionValue.isUndefinedOrNull())
+                return false;
+
+            auto* function = functionValue.toObject(&lexicalGlobalObject);
+            RETURN_IF_EXCEPTION(scope, false);
+
+            auto callData = JSC::getCallData(function);
+            if (callData.type == JSC::CallData::Type::None)
+                return false;
+
+            auto resultValue = JSC::call(&lexicalGlobalObject, function, callData, controllerObject, JSC::MarkedArgumentBuffer());
+            RETURN_IF_EXCEPTION(scope, false);
+
+            return resultValue.toBoolean(&lexicalGlobalObject);
+        });
+        m_controlsState = isDeinitialized ? ControlsState::PartiallyDeinitialized : m_controlsState;
+    }
+#endif // ENABLE(MODERN_MEDIA_CONTROLS)
+
     if (!m_player)
         return;
 
@@ -8153,94 +8204,138 @@ bool HTMLMediaElement::ensureMediaControls()
 
     INFO_LOG(LOGIDENTIFIER);
 
+    ControlsState oldControlsState = m_controlsState;
     m_controlsState = ControlsState::Initializing;
 
-    auto controlsReady = setupAndCallJS([this, mediaControlsScripts = WTFMove(mediaControlsScripts)](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController& scriptController, DOMWrapperWorld& world) {
-        auto& vm = globalObject.vm();
-        auto scope = DECLARE_CATCH_SCOPE(vm);
+    auto controlsReady = false;
+    if (oldControlsState == ControlsState::None) {
+        controlsReady = setupAndCallJS([this, mediaControlsScripts = WTFMove(mediaControlsScripts)](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController& scriptController, DOMWrapperWorld& world) {
+            auto& vm = globalObject.vm();
+            auto scope = DECLARE_CATCH_SCOPE(vm);
+
+            auto reportExceptionAndReturnFalse = [&] {
+                auto* exception = scope.exception();
+                scope.clearException();
+                reportException(&globalObject, exception);
+                return false;
+            };
+
+            for (auto& mediaControlsScript : mediaControlsScripts) {
+                if (mediaControlsScript.isEmpty())
+                    continue;
+                scriptController.evaluateInWorldIgnoringException(ScriptSourceCode(mediaControlsScript), world);
+                RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
+            }
 
-        auto reportExceptionAndReturnFalse = [&] {
-            auto* exception = scope.exception();
-            scope.clearException();
-            reportException(&globalObject, exception);
-            return false;
-        };
+            // The media controls script must provide a method with the following details.
+            // Name: createControls
+            // Parameters:
+            //     1. The ShadowRoot element that will hold the controls.
+            //     2. This object (and HTMLMediaElement).
+            //     3. The MediaControlsHost object.
+            // Return value:
+            //     A reference to the created media controller instance.
 
-        for (auto& mediaControlsScript : mediaControlsScripts) {
-            if (mediaControlsScript.isEmpty())
-                continue;
-            scriptController.evaluateInWorldIgnoringException(ScriptSourceCode(mediaControlsScript), world);
+            auto functionValue = globalObject.get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "createControls"_s));
+            if (functionValue.isUndefinedOrNull())
+                return false;
+
+            if (!m_mediaControlsHost)
+                m_mediaControlsHost = MediaControlsHost::create(*this);
+
+            auto mediaJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *this);
+            auto mediaControlsHostJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *m_mediaControlsHost);
+
+            JSC::MarkedArgumentBuffer argList;
+            argList.append(toJS(&lexicalGlobalObject, &globalObject, ensureUserAgentShadowRoot()));
+            argList.append(mediaJSWrapper);
+            argList.append(mediaControlsHostJSWrapper);
+            ASSERT(!argList.hasOverflowed());
+
+            auto* function = functionValue.toObject(&lexicalGlobalObject);
             RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
-        }
+            auto callData = JSC::getCallData(function);
+            if (callData.type == JSC::CallData::Type::None)
+                return false;
 
-        // The media controls script must provide a method with the following details.
-        // Name: createControls
-        // Parameters:
-        //     1. The ShadowRoot element that will hold the controls.
-        //     2. This object (and HTMLMediaElement).
-        //     3. The MediaControlsHost object.
-        // Return value:
-        //     A reference to the created media controller instance.
+            auto controllerValue = JSC::call(&lexicalGlobalObject, function, callData, &globalObject, argList);
+            RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
 
-        auto functionValue = globalObject.get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "createControls"_s));
-        if (functionValue.isUndefinedOrNull())
-            return false;
+            auto* controllerObject = JSC::jsDynamicCast<JSC::JSObject*>(controllerValue);
+            if (!controllerObject)
+                return false;
 
-        if (!m_mediaControlsHost)
-            m_mediaControlsHost = MediaControlsHost::create(*this);
+            // Connect the Media, MediaControllerHost, and Controller so the GC knows about their relationship
+            auto* mediaJSWrapperObject = mediaJSWrapper.toObject(&lexicalGlobalObject);
+            RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
+            auto controlsHost = JSC::Identifier::fromString(vm, "controlsHost"_s);
 
-        auto mediaJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *this);
-        auto mediaControlsHostJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *m_mediaControlsHost);
+            ASSERT(!mediaJSWrapperObject->hasProperty(&lexicalGlobalObject, controlsHost));
 
-        JSC::MarkedArgumentBuffer argList;
-        argList.append(toJS(&lexicalGlobalObject, &globalObject, ensureUserAgentShadowRoot()));
-        argList.append(mediaJSWrapper);
-        argList.append(mediaControlsHostJSWrapper);
-        ASSERT(!argList.hasOverflowed());
+            mediaJSWrapperObject->putDirect(vm, controlsHost, mediaControlsHostJSWrapper, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
 
-        auto* function = functionValue.toObject(&lexicalGlobalObject);
-        RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
-        auto callData = JSC::getCallData(function);
-        if (callData.type == JSC::CallData::Type::None)
-            return false;
+            auto* mediaControlsHostJSWrapperObject = JSC::jsDynamicCast<JSC::JSObject*>(mediaControlsHostJSWrapper);
+            if (!mediaControlsHostJSWrapperObject)
+                return false;
 
-        auto controllerValue = JSC::call(&lexicalGlobalObject, function, callData, &globalObject, argList);
-        RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
+            auto controller = builtinNames(vm).controllerPublicName();
 
-        auto* controllerObject = JSC::jsDynamicCast<JSC::JSObject*>(controllerValue);
-        if (!controllerObject)
-            return false;
+            ASSERT(!controllerObject->hasProperty(&lexicalGlobalObject, controller));
 
-        // Connect the Media, MediaControllerHost, and Controller so the GC knows about their relationship
-        auto* mediaJSWrapperObject = mediaJSWrapper.toObject(&lexicalGlobalObject);
-        RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
-        auto controlsHost = JSC::Identifier::fromString(vm, "controlsHost"_s);
+            mediaControlsHostJSWrapperObject->putDirect(vm, controller, controllerValue, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
 
-        ASSERT(!mediaJSWrapperObject->hasProperty(&lexicalGlobalObject, controlsHost));
+            if (m_mediaControlsDependOnPageScaleFactor)
+                updatePageScaleFactorJSProperty();
 
-        mediaJSWrapperObject->putDirect(vm, controlsHost, mediaControlsHostJSWrapper, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
+            RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
 
-        auto* mediaControlsHostJSWrapperObject = JSC::jsDynamicCast<JSC::JSObject*>(mediaControlsHostJSWrapper);
-        if (!mediaControlsHostJSWrapperObject)
-            return false;
+            updateUsesLTRUserInterfaceLayoutDirectionJSProperty();
+            RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
 
-        auto controller = builtinNames(vm).controllerPublicName();
+            return true;
+        });
+} else if (oldControlsState == ControlsState::PartiallyDeinitialized) {
+        controlsReady = setupAndCallJS([this](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController&, DOMWrapperWorld&) {
+            auto& vm = globalObject.vm();
+            auto scope = DECLARE_THROW_SCOPE(vm);
 
-        ASSERT(!controllerObject->hasProperty(&lexicalGlobalObject, controller));
+            auto controllerValue = controllerJSValue(lexicalGlobalObject, globalObject, *this);
+            RETURN_IF_EXCEPTION(scope, false);
+            auto* controllerObject = controllerValue.toObject(&lexicalGlobalObject);
+            RETURN_IF_EXCEPTION(scope, false);
 
-        mediaControlsHostJSWrapperObject->putDirect(vm, controller, controllerValue, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
+            auto functionValue = controllerObject->get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "reinitialize"_s));
+            if (UNLIKELY(scope.exception()) || functionValue.isUndefinedOrNull())
+                return false;
 
-        if (m_mediaControlsDependOnPageScaleFactor)
-            updatePageScaleFactorJSProperty();
+            if (!m_mediaControlsHost)
+                m_mediaControlsHost = MediaControlsHost::create(*this);
 
-        RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
+            auto mediaJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *this);
+            auto mediaControlsHostJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *m_mediaControlsHost.copyRef());
 
-        updateUsesLTRUserInterfaceLayoutDirectionJSProperty();
-        RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
+            JSC::MarkedArgumentBuffer argList;
+            argList.append(toJS(&lexicalGlobalObject, &globalObject, Ref { ensureUserAgentShadowRoot() }));
+            argList.append(mediaJSWrapper);
+            argList.append(mediaControlsHostJSWrapper);
+            ASSERT(!argList.hasOverflowed());
 
-        return true;
-    });
-    m_controlsState = controlsReady ? ControlsState::Ready : ControlsState::None;
+            auto* function = functionValue.toObject(&lexicalGlobalObject);
+            RETURN_IF_EXCEPTION(scope, false);
+
+            auto callData = JSC::getCallData(function);
+            if (callData.type == JSC::CallData::Type::None)
+                return false;
+
+            auto resultValue = JSC::call(&lexicalGlobalObject, function, callData, controllerObject, argList);
+            RETURN_IF_EXCEPTION(scope, false);
+
+            return resultValue.toBoolean(&lexicalGlobalObject);
+        });
+    } else
+        ASSERT_NOT_REACHED();
+
+    m_controlsState = controlsReady ? ControlsState::Ready : oldControlsState;
     return controlsReady;
 }
 

Source/WebCore/html/HTMLMediaElement.h

@@ -1215,7 +1215,8 @@ class HTMLMediaElement
     bool m_shouldVideoPlaybackRequireUserGesture : 1;
     bool m_volumeLocked : 1;
 
-    enum class ControlsState : uint8_t { None, Initializing, Ready };
+    enum class ControlsState : uint8_t { None, Initializing, Ready, PartiallyDeinitialized };
+    friend String convertEnumerationToString(HTMLMediaElement::ControlsState enumerationValue);
     ControlsState m_controlsState { ControlsState::None };
 
     AutoplayEventPlaybackState m_autoplayEventPlaybackState { AutoplayEventPlaybackState::None };