Skip to content

Commit a4387a2

Browse files
eocanhacalvaris
eocanha
authored and
calvaris
committed
[Modern Media Controls] HTMLMediaElement is never destroyed when showing 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
1 parent 96b46d0 commit a4387a2

4 files changed

Lines changed: 201 additions & 68 deletions

File tree

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,15 @@ const iconService = new class IconService {
7474
}
7575

7676
// Public
77+
get shadowRoot()
78+
{
79+
return this.shadowRootWeakRef ? this.shadowRootWeakRef.deref() : null;
80+
}
81+
82+
set shadowRoot(shadowRoot)
83+
{
84+
this.shadowRootWeakRef = new WeakRef(shadowRoot);
85+
}
7786

7887
imageForIconAndLayoutTraits(icon, layoutTraits)
7988
{

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

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,10 @@
2525

2626
class MediaController
2727
{
28-
2928
constructor(shadowRoot, media, host)
3029
{
31-
this.shadowRoot = shadowRoot;
32-
this.media = media;
30+
this.shadowRootWeakRef = new WeakRef(shadowRoot);
31+
this.mediaWeakRef = new WeakRef(media);
3332
this.host = host;
3433

3534
this.fullscreenChangeEventType = media.webkitSupportsPresentationMode ? "webkitpresentationmodechanged" : "webkitfullscreenchange";
@@ -65,6 +64,16 @@ class MediaController
6564
}
6665

6766
// Public
67+
get media()
68+
{
69+
return this.mediaWeakRef ? this.mediaWeakRef.deref() : null;
70+
}
71+
72+
get shadowRoot()
73+
{
74+
75+
return this.shadowRootWeakRef ? this.shadowRootWeakRef.deref() : null;
76+
}
6877

6978
get isAudio()
7079
{
@@ -91,6 +100,9 @@ class MediaController
91100

92101
get isFullscreen()
93102
{
103+
if (!this.media)
104+
return false;
105+
94106
return this.media.webkitSupportsPresentationMode ? this.media.webkitPresentationMode === "fullscreen" : this.media.webkitDisplayingFullscreen;
95107
}
96108

@@ -265,6 +277,22 @@ class MediaController
265277
return true;
266278
}
267279

280+
deinitialize()
281+
{
282+
this.shadowRoot.removeChild(this.container);
283+
return true;
284+
}
285+
286+
reinitialize(shadowRoot, media, host)
287+
{
288+
iconService.shadowRoot = shadowRoot;
289+
this.shadowRootWeakRef = new WeakRef(shadowRoot);
290+
this.mediaWeakRef = new WeakRef(media);
291+
this.host = host;
292+
shadowRoot.appendChild(this.container);
293+
return true;
294+
}
295+
268296
// Private
269297

270298
_supportingObjectClasses()

‎Source/WebCore/html/HTMLMediaElement.cpp‎

Lines changed: 159 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,25 @@ String convertEnumerationToString(HTMLMediaElement::SpeechSynthesisState enumera
311311
return values[static_cast<size_t>(enumerationValue)];
312312
}
313313

314+
String convertEnumerationToString(HTMLMediaElement::ControlsState enumerationValue)
315+
{
316+
// None, Initializing, Ready, PartiallyDeinitialized
317+
static const NeverDestroyed<String> values[] = {
318+
MAKE_STATIC_STRING_IMPL("None"),
319+
MAKE_STATIC_STRING_IMPL("Initializing"),
320+
MAKE_STATIC_STRING_IMPL("Ready"),
321+
MAKE_STATIC_STRING_IMPL("PartiallyDeinitialized"),
322+
};
323+
static_assert(!static_cast<size_t>(HTMLMediaElement::ControlsState::None), "HTMLMediaElement::None is not 0 as expected");
324+
static_assert(static_cast<size_t>(HTMLMediaElement::ControlsState::Initializing) == 1, "HTMLMediaElement::Initializing is not 1 as expected");
325+
static_assert(static_cast<size_t>(HTMLMediaElement::ControlsState::Ready) == 2, "HTMLMediaElement::Ready is not 2 as expected");
326+
static_assert(static_cast<size_t>(HTMLMediaElement::ControlsState::PartiallyDeinitialized) == 3, "HTMLMediaElement::PartiallyDeinitialized is not 3 as expected");
327+
ASSERT(static_cast<size_t>(enumerationValue) < std::size(values));
328+
return values[static_cast<size_t>(enumerationValue)];
329+
}
330+
331+
static JSC::JSValue controllerJSValue(JSC::JSGlobalObject& lexicalGlobalObject, JSDOMGlobalObject&, HTMLMediaElement&);
332+
314333
class TrackDisplayUpdateScope {
315334
public:
316335
TrackDisplayUpdateScope(HTMLMediaElement& element)
@@ -920,6 +939,38 @@ void HTMLMediaElement::pauseAfterDetachedTask()
920939
if (m_videoFullscreenMode == VideoFullscreenModeStandard)
921940
exitFullscreen();
922941

942+
#if ENABLE(MODERN_MEDIA_CONTROLS)
943+
if (m_controlsState == ControlsState::Initializing || m_controlsState == ControlsState::Ready) {
944+
// Call MediaController.deinitialize() to get rid of circular references.
945+
bool isDeinitialized = setupAndCallJS([this](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController&, DOMWrapperWorld&) {
946+
auto& vm = globalObject.vm();
947+
auto scope = DECLARE_THROW_SCOPE(vm);
948+
949+
auto controllerValue = controllerJSValue(lexicalGlobalObject, globalObject, *this);
950+
RETURN_IF_EXCEPTION(scope, false);
951+
auto* controllerObject = controllerValue.toObject(&lexicalGlobalObject);
952+
RETURN_IF_EXCEPTION(scope, false);
953+
954+
auto functionValue = controllerObject->get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "deinitialize"_s));
955+
if (UNLIKELY(scope.exception()) || functionValue.isUndefinedOrNull())
956+
return false;
957+
958+
auto* function = functionValue.toObject(&lexicalGlobalObject);
959+
RETURN_IF_EXCEPTION(scope, false);
960+
961+
auto callData = JSC::getCallData(function);
962+
if (callData.type == JSC::CallData::Type::None)
963+
return false;
964+
965+
auto resultValue = JSC::call(&lexicalGlobalObject, function, callData, controllerObject, JSC::MarkedArgumentBuffer());
966+
RETURN_IF_EXCEPTION(scope, false);
967+
968+
return resultValue.toBoolean(&lexicalGlobalObject);
969+
});
970+
m_controlsState = isDeinitialized ? ControlsState::PartiallyDeinitialized : m_controlsState;
971+
}
972+
#endif // ENABLE(MODERN_MEDIA_CONTROLS)
973+
923974
if (!m_player)
924975
return;
925976

@@ -8153,94 +8204,138 @@ bool HTMLMediaElement::ensureMediaControls()
81538204

81548205
INFO_LOG(LOGIDENTIFIER);
81558206

8207+
ControlsState oldControlsState = m_controlsState;
81568208
m_controlsState = ControlsState::Initializing;
81578209

8158-
auto controlsReady = setupAndCallJS([this, mediaControlsScripts = WTFMove(mediaControlsScripts)](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController& scriptController, DOMWrapperWorld& world) {
8159-
auto& vm = globalObject.vm();
8160-
auto scope = DECLARE_CATCH_SCOPE(vm);
8210+
auto controlsReady = false;
8211+
if (oldControlsState == ControlsState::None) {
8212+
controlsReady = setupAndCallJS([this, mediaControlsScripts = WTFMove(mediaControlsScripts)](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController& scriptController, DOMWrapperWorld& world) {
8213+
auto& vm = globalObject.vm();
8214+
auto scope = DECLARE_CATCH_SCOPE(vm);
8215+
8216+
auto reportExceptionAndReturnFalse = [&] {
8217+
auto* exception = scope.exception();
8218+
scope.clearException();
8219+
reportException(&globalObject, exception);
8220+
return false;
8221+
};
8222+
8223+
for (auto& mediaControlsScript : mediaControlsScripts) {
8224+
if (mediaControlsScript.isEmpty())
8225+
continue;
8226+
scriptController.evaluateInWorldIgnoringException(ScriptSourceCode(mediaControlsScript), world);
8227+
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8228+
}
81618229

8162-
auto reportExceptionAndReturnFalse = [&] {
8163-
auto* exception = scope.exception();
8164-
scope.clearException();
8165-
reportException(&globalObject, exception);
8166-
return false;
8167-
};
8230+
// The media controls script must provide a method with the following details.
8231+
// Name: createControls
8232+
// Parameters:
8233+
// 1. The ShadowRoot element that will hold the controls.
8234+
// 2. This object (and HTMLMediaElement).
8235+
// 3. The MediaControlsHost object.
8236+
// Return value:
8237+
// A reference to the created media controller instance.
81688238

8169-
for (auto& mediaControlsScript : mediaControlsScripts) {
8170-
if (mediaControlsScript.isEmpty())
8171-
continue;
8172-
scriptController.evaluateInWorldIgnoringException(ScriptSourceCode(mediaControlsScript), world);
8239+
auto functionValue = globalObject.get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "createControls"_s));
8240+
if (functionValue.isUndefinedOrNull())
8241+
return false;
8242+
8243+
if (!m_mediaControlsHost)
8244+
m_mediaControlsHost = MediaControlsHost::create(*this);
8245+
8246+
auto mediaJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *this);
8247+
auto mediaControlsHostJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *m_mediaControlsHost);
8248+
8249+
JSC::MarkedArgumentBuffer argList;
8250+
argList.append(toJS(&lexicalGlobalObject, &globalObject, ensureUserAgentShadowRoot()));
8251+
argList.append(mediaJSWrapper);
8252+
argList.append(mediaControlsHostJSWrapper);
8253+
ASSERT(!argList.hasOverflowed());
8254+
8255+
auto* function = functionValue.toObject(&lexicalGlobalObject);
81738256
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8174-
}
8257+
auto callData = JSC::getCallData(function);
8258+
if (callData.type == JSC::CallData::Type::None)
8259+
return false;
81758260

8176-
// The media controls script must provide a method with the following details.
8177-
// Name: createControls
8178-
// Parameters:
8179-
// 1. The ShadowRoot element that will hold the controls.
8180-
// 2. This object (and HTMLMediaElement).
8181-
// 3. The MediaControlsHost object.
8182-
// Return value:
8183-
// A reference to the created media controller instance.
8261+
auto controllerValue = JSC::call(&lexicalGlobalObject, function, callData, &globalObject, argList);
8262+
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
81848263

8185-
auto functionValue = globalObject.get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "createControls"_s));
8186-
if (functionValue.isUndefinedOrNull())
8187-
return false;
8264+
auto* controllerObject = JSC::jsDynamicCast<JSC::JSObject*>(controllerValue);
8265+
if (!controllerObject)
8266+
return false;
81888267

8189-
if (!m_mediaControlsHost)
8190-
m_mediaControlsHost = MediaControlsHost::create(*this);
8268+
// Connect the Media, MediaControllerHost, and Controller so the GC knows about their relationship
8269+
auto* mediaJSWrapperObject = mediaJSWrapper.toObject(&lexicalGlobalObject);
8270+
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8271+
auto controlsHost = JSC::Identifier::fromString(vm, "controlsHost"_s);
81918272

8192-
auto mediaJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *this);
8193-
auto mediaControlsHostJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *m_mediaControlsHost);
8273+
ASSERT(!mediaJSWrapperObject->hasProperty(&lexicalGlobalObject, controlsHost));
81948274

8195-
JSC::MarkedArgumentBuffer argList;
8196-
argList.append(toJS(&lexicalGlobalObject, &globalObject, ensureUserAgentShadowRoot()));
8197-
argList.append(mediaJSWrapper);
8198-
argList.append(mediaControlsHostJSWrapper);
8199-
ASSERT(!argList.hasOverflowed());
8275+
mediaJSWrapperObject->putDirect(vm, controlsHost, mediaControlsHostJSWrapper, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
82008276

8201-
auto* function = functionValue.toObject(&lexicalGlobalObject);
8202-
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8203-
auto callData = JSC::getCallData(function);
8204-
if (callData.type == JSC::CallData::Type::None)
8205-
return false;
8277+
auto* mediaControlsHostJSWrapperObject = JSC::jsDynamicCast<JSC::JSObject*>(mediaControlsHostJSWrapper);
8278+
if (!mediaControlsHostJSWrapperObject)
8279+
return false;
82068280

8207-
auto controllerValue = JSC::call(&lexicalGlobalObject, function, callData, &globalObject, argList);
8208-
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8281+
auto controller = builtinNames(vm).controllerPublicName();
82098282

8210-
auto* controllerObject = JSC::jsDynamicCast<JSC::JSObject*>(controllerValue);
8211-
if (!controllerObject)
8212-
return false;
8283+
ASSERT(!controllerObject->hasProperty(&lexicalGlobalObject, controller));
82138284

8214-
// Connect the Media, MediaControllerHost, and Controller so the GC knows about their relationship
8215-
auto* mediaJSWrapperObject = mediaJSWrapper.toObject(&lexicalGlobalObject);
8216-
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8217-
auto controlsHost = JSC::Identifier::fromString(vm, "controlsHost"_s);
8285+
mediaControlsHostJSWrapperObject->putDirect(vm, controller, controllerValue, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
82188286

8219-
ASSERT(!mediaJSWrapperObject->hasProperty(&lexicalGlobalObject, controlsHost));
8287+
if (m_mediaControlsDependOnPageScaleFactor)
8288+
updatePageScaleFactorJSProperty();
82208289

8221-
mediaJSWrapperObject->putDirect(vm, controlsHost, mediaControlsHostJSWrapper, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
8290+
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
82228291

8223-
auto* mediaControlsHostJSWrapperObject = JSC::jsDynamicCast<JSC::JSObject*>(mediaControlsHostJSWrapper);
8224-
if (!mediaControlsHostJSWrapperObject)
8225-
return false;
8292+
updateUsesLTRUserInterfaceLayoutDirectionJSProperty();
8293+
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
82268294

8227-
auto controller = builtinNames(vm).controllerPublicName();
8295+
return true;
8296+
});
8297+
} else if (oldControlsState == ControlsState::PartiallyDeinitialized) {
8298+
controlsReady = setupAndCallJS([this](JSDOMGlobalObject& globalObject, JSC::JSGlobalObject& lexicalGlobalObject, ScriptController&, DOMWrapperWorld&) {
8299+
auto& vm = globalObject.vm();
8300+
auto scope = DECLARE_THROW_SCOPE(vm);
82288301

8229-
ASSERT(!controllerObject->hasProperty(&lexicalGlobalObject, controller));
8302+
auto controllerValue = controllerJSValue(lexicalGlobalObject, globalObject, *this);
8303+
RETURN_IF_EXCEPTION(scope, false);
8304+
auto* controllerObject = controllerValue.toObject(&lexicalGlobalObject);
8305+
RETURN_IF_EXCEPTION(scope, false);
82308306

8231-
mediaControlsHostJSWrapperObject->putDirect(vm, controller, controllerValue, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
8307+
auto functionValue = controllerObject->get(&lexicalGlobalObject, JSC::Identifier::fromString(vm, "reinitialize"_s));
8308+
if (UNLIKELY(scope.exception()) || functionValue.isUndefinedOrNull())
8309+
return false;
82328310

8233-
if (m_mediaControlsDependOnPageScaleFactor)
8234-
updatePageScaleFactorJSProperty();
8311+
if (!m_mediaControlsHost)
8312+
m_mediaControlsHost = MediaControlsHost::create(*this);
82358313

8236-
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8314+
auto mediaJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *this);
8315+
auto mediaControlsHostJSWrapper = toJS(&lexicalGlobalObject, &globalObject, *m_mediaControlsHost.copyRef());
82378316

8238-
updateUsesLTRUserInterfaceLayoutDirectionJSProperty();
8239-
RETURN_IF_EXCEPTION(scope, reportExceptionAndReturnFalse());
8317+
JSC::MarkedArgumentBuffer argList;
8318+
argList.append(toJS(&lexicalGlobalObject, &globalObject, Ref { ensureUserAgentShadowRoot() }));
8319+
argList.append(mediaJSWrapper);
8320+
argList.append(mediaControlsHostJSWrapper);
8321+
ASSERT(!argList.hasOverflowed());
82408322

8241-
return true;
8242-
});
8243-
m_controlsState = controlsReady ? ControlsState::Ready : ControlsState::None;
8323+
auto* function = functionValue.toObject(&lexicalGlobalObject);
8324+
RETURN_IF_EXCEPTION(scope, false);
8325+
8326+
auto callData = JSC::getCallData(function);
8327+
if (callData.type == JSC::CallData::Type::None)
8328+
return false;
8329+
8330+
auto resultValue = JSC::call(&lexicalGlobalObject, function, callData, controllerObject, argList);
8331+
RETURN_IF_EXCEPTION(scope, false);
8332+
8333+
return resultValue.toBoolean(&lexicalGlobalObject);
8334+
});
8335+
} else
8336+
ASSERT_NOT_REACHED();
8337+
8338+
m_controlsState = controlsReady ? ControlsState::Ready : oldControlsState;
82448339
return controlsReady;
82458340
}
82468341

‎Source/WebCore/html/HTMLMediaElement.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1215,7 +1215,8 @@ class HTMLMediaElement
12151215
bool m_shouldVideoPlaybackRequireUserGesture : 1;
12161216
bool m_volumeLocked : 1;
12171217

1218-
enum class ControlsState : uint8_t { None, Initializing, Ready };
1218+
enum class ControlsState : uint8_t { None, Initializing, Ready, PartiallyDeinitialized };
1219+
friend String convertEnumerationToString(HTMLMediaElement::ControlsState enumerationValue);
12191220
ControlsState m_controlsState { ControlsState::None };
12201221

12211222
AutoplayEventPlaybackState m_autoplayEventPlaybackState { AutoplayEventPlaybackState::None };

0 commit comments

Comments
 (0)