Skip to content

Commit

Permalink
[Modern Media Controls] HTMLMediaElement is never destoyed when showi…
Browse files Browse the repository at this point in the history
…ng media controls

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

Reviewed by NOBODY (OOPS!).

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: WebPlatformForEmbedded/WPEWebKit#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 deinitialize the MediaController when
HTMLMediaElement is no longer in use. The best place mathing that
instant is pauseAfterDetachedTask(). This deinitialization takes care of
properly nulling the mentioned objects that hold (or transitively hold)
the references to HTMLMediaElement. A new MediaController.deinitialize()
method has been added to the JavaScript code for this purpose.

* Source/WebCore/Modules/modern-media-controls/media/media-controller.js:
(MediaController.prototype.deinitialize): Set references to the media element and the shadow root to null and also detach the UI container from the shadow root, so no references to HTMLMediaElement are held anymore, either directly or transitively.
* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::pauseAfterDetachedTask): Add code to call to the MediaController.deinitialize() JavaScript method.
  • Loading branch information
eocanha committed Mar 13, 2024
1 parent 09b4cd8 commit 329c272
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,15 @@ class MediaController
return true;
}

deinitialize()
{
this.media = null;
this.shadowRoot.removeChild(this.container);
this.shadowRoot = null;
iconService.shadowRoot = null;
return true;
}

// Private

_supportingObjectClasses()
Expand Down
34 changes: 34 additions & 0 deletions Source/WebCore/html/HTMLMediaElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,8 @@ String convertEnumerationToString(HTMLMediaElement::SpeechSynthesisState enumera
return values[static_cast<size_t>(enumerationValue)];
}

static JSC::JSValue controllerJSValue(JSC::JSGlobalObject& lexicalGlobalObject, JSDOMGlobalObject&, HTMLMediaElement&);

class TrackDisplayUpdateScope {
public:
TrackDisplayUpdateScope(HTMLMediaElement& element)
Expand Down Expand Up @@ -934,6 +936,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::None : m_controlsState;
}
#endif // ENABLE(MODERN_MEDIA_CONTROLS)

if (!m_player)
return;

Expand Down

0 comments on commit 329c272

Please sign in to comment.