[webkit-changes] [WebKit/WebKit] 80dd62: [Modern Media Controls] HTMLMediaElement is never ...

[Enrique Ocaña González]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 80dd62a7e3323e4c039cac722ec4c563c92a17ff
      https://github.com/WebKit/WebKit/commit/80dd62a7e3323e4c039cac722ec4c563c92a17ff
  Author: Enrique Ocaña González <eocanha at igalia.com>
  Date:   2024-04-08 (Mon, 08 Apr 2024)

  Changed paths:
    M Source/WebCore/Modules/modern-media-controls/controls/icon-service.js
    M Source/WebCore/Modules/modern-media-controls/media/media-controller.js
    M Source/WebCore/html/HTMLMediaElement.cpp
    M Source/WebCore/html/HTMLMediaElement.h

  Log Message:
  -----------
  [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: https://github.com/WebPlatformForEmbedded/WPEWebKit/issues/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



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications