Skip to content

media: Clean up shadow root content when removing controls#43983

Merged
jdm merged 1 commit into
servo:mainfrom
Messi002:issue-43828
Apr 9, 2026
Merged

media: Clean up shadow root content when removing controls#43983
jdm merged 1 commit into
servo:mainfrom
Messi002:issue-43828

Conversation

@Messi002

@Messi002 Messi002 commented Apr 6, 2026 •
edited by jdm
Loading

Copy link
Copy Markdown
Contributor

When media controls are removed (either by removing the controls attribute or by removing the element from the DOM), the shadow root's children are now cleared. This breaks the reference cycle between the js mediacontrols instance and the media element, the event listeners and this.media reference in the controls script would otherwise keep the element alive and prevent garbage collection.

I tested the controls and it renders correctly and deleting a video with controls doesn't crash.

Fixes: #43828

@Messi002 Messi002 requested a review from gterzian as a code owner April 6, 2026 22:11
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 6, 2026
@Messi002 Messi002 force-pushed the issue-43828 branch 3 times, most recently from 27478d6 to ad96e6a Compare April 7, 2026 00:06
Gae24
Gae24 previously requested changes Apr 7, 2026
View reviewed changes
Comment thread components/script/dom/html/htmlmediaelement.rs
@servo-highfive servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 7, 2026
@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 7, 2026
@rayguo17

rayguo17 commented Apr 8, 2026

Copy link
Copy Markdown
Member

The thing is, cleaning the text content of the <script> tag does not clean up the object that already created by executing the <script> tag.

We should probably do:

diff --git a/components/script/resources/media-controls.js b/components/script/resources/media-controls.js
index fa457d5268a..251335232ef 100644
--- a/components/script/resources/media-controls.js
+++ b/components/script/resources/media-controls.js
@@ -241,6 +241,7 @@
 
     cleanup() {
       this.mutationObserver.disconnect();
+      this.mutationObserver = null;
       this.mediaEvents.forEach(event => {
         this.media.removeEventListener(event, this);
       });

But we still need to figure out how to run the cleanup step when media elements is deleted with controls attribute presented.

@Messi002

Messi002 commented Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

@rayguo17
thanks for your update

@Messi002

Messi002 commented Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

I will again into it

@Messi002

Messi002 commented Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

@rayguo17
what do you think about adding a way to trigger the js cleanup when the element is removed from the DOM. I'm thinking of either having the MutationObserver also watch for DOM removal, or dispatching a custom event from rust that the js cleanup code listens for.

Which approach would you prefer?

@rayguo17

rayguo17 commented Apr 8, 2026

Copy link
Copy Markdown
Member

The first approach sounds more reasonable.

@Messi002

Messi002 commented Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

@rayguo17 can you check again and tell me what you think now ?

@rayguo17

rayguo17 commented Apr 9, 2026

Copy link
Copy Markdown
Member

This is nice!

@rayguo17 rayguo17 self-requested a review April 9, 2026 02:22
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 9, 2026
jdm
jdm approved these changes Apr 9, 2026
View reviewed changes

@jdm jdm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution!

Comment thread components/script/dom/html/htmlmediaelement.rs Outdated
Comment thread components/script/resources/media-controls.js Outdated
@jdm jdm added the T-linux-wpt Do a try run of the WPT label Apr 9, 2026
@github-actions github-actions Bot removed the T-linux-wpt Do a try run of the WPT label Apr 9, 2026
@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown

🔨 Triggering try run (#24169289463) for Linux (WPT)

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown

Test results for linux-wpt from try job (#24169289463):

Flaky unexpected result (39)
  • TIMEOUT /FileAPI/url/url-in-tags-revoke.window.html (#19978)
    • PASS [expected TIMEOUT] subtest: Fetching a blob URL immediately before revoking it works in &lt;script&gt; tags.
  • OK /FileAPI/url/url-with-fetch.any.worker.html (#21517)
    • FAIL [expected PASS] subtest: Revoke blob URL after calling fetch, fetch should succeed

      promise_test: Unhandled rejection with value: object "TypeError: Network error: Blob URL store error: InvalidFileID"

  • OK /IndexedDB/idbdatabase_deleteObjectStore.any.html (#43823)
    • PASS [expected FAIL] subtest: Deleted object store's name should be removed from database's list. Attempting to use a deleted IDBObjectStore should throw an InvalidStateError
  • OK /_mozilla/css/offset_properties_inline.html (#40543)
    • FAIL [expected PASS] subtest: offsetTop

      assert_equals: offsetTop of #inline-1 should be 0. expected 0 but got -1

    • FAIL [expected PASS] subtest: offsetLeft

      assert_equals: offsetLeft of #inline-2 should be 40. expected 40 but got 25

  • OK /_mozilla/mozilla/getBoundingClientRect.html (#39668)
    • FAIL [expected PASS] subtest: getBoundingClientRect 1

      assert_equals: expected 62 but got 60.35

  • CRASH [expected OK] /_webgl/conformance/glsl/bugs/floored-division-accuracy.html
  • OK [expected TIMEOUT] /_webgl/conformance/textures/misc/tex-video-using-tex-unit-non-zero.html (#39735)
    • PASS [expected NOTRUN] subtest: Overall test
    • PASS [expected FAIL] subtest: WebGL test #0
  • CRASH [expected OK] /content-security-policy/meta/sandbox-iframe.html (#43478)
  • TIMEOUT [expected OK] /credential-management/credentialscontainer-frame-basics.https.html (#39430)
    • TIMEOUT [expected FAIL] subtest: navigator.credentials should be undefined in documents generated from data: URLs.

      Test timed out

  • OK /css/css-cascade/layer-font-face-override.html (#35935)
    • PASS [expected FAIL] subtest: @font-face override update with appended sheet 1
    • PASS [expected FAIL] subtest: @font-face override update with appended sheet 2
  • FAIL [expected PASS] /css/css-ui/compute-kind-widget-generated/grouped-kind-of-widget-fallback-border-image-source-001.html
  • FAIL [expected PASS] /css/css-ui/compute-kind-widget-generated/grouped-kind-of-widget-fallback-border-top-left-radius-001.html
  • FAIL [expected PASS] /css/css-ui/webkit-appearance-meter-001.html
  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin-fragment.html (#20768)
    • PASS [expected FAIL] subtest: Tests that a fragment navigation in the unload handler will not block the initial navigation
  • OK /html/browsers/history/the-history-interface/traverse_the_history_5.html (#21383)
    • FAIL [expected PASS] subtest: Multiple history traversals, last would be aborted

      assert_array_equals: Pages opened during history navigation expected property 1 to be 5 but got 3 (expected array [6, 5] got [6, 3])

  • OK /html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/allow-scripts-flag-changing-1.html (#39694)
    • FAIL [expected PASS] subtest: Meta refresh is blocked by the allow-scripts sandbox flag at its creation time, not when refresh comes due

      uncaught exception: Error: assert_unreached: The iframe from which the meta came from must not refresh Reached unreachable code

  • TIMEOUT [expected OK] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_other_frame_popup.sub.html (#39702)
    • TIMEOUT [expected FAIL] subtest: Sandboxed iframe can not navigate other frame's popup

      Test timed out

  • TIMEOUT [expected OK] /html/user-activation/navigation-state-reset-sameorigin.html
    • TIMEOUT [expected PASS] subtest: Post-navigation state reset.

      Test timed out

  • CRASH [expected OK] /html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-5.html
  • OK /navigation-timing/test-navigation-type-reload.html (#33334)
    • PASS [expected FAIL] subtest: Reload domInteractive &gt; Original domInteractive
  • FAIL [expected PASS] /png/apng/fcTL-blend-source-solid.html (#41560)
  • TIMEOUT [expected OK] /pointerevents/compat/pointerevent_touch-action_two-finger_interaction.html
    • NOTRUN [expected PASS] subtest: touch two-finger pan on 'touch-action: pan-x pan-y'
    • NOTRUN [expected FAIL] subtest: touch two-finger pan on 'touch-action: pinch-zoom'
  • OK /preload/link-header-preload-delay-onload.html (#39622)
    • FAIL [expected PASS] subtest: Makes sure that Link headers preload resources and block window.onload after resource discovery

      assert_true: expected true got false

  • ERROR [expected OK] /resource-timing/cors-preflight.any.html (#28694)
  • OK [expected TIMEOUT] /trusted-types/trusted-types-navigation.html?01-05 (#38975)
    • PASS [expected TIMEOUT] subtest: Navigate a window via anchor with javascript:-urls in report-only mode.
    • PASS [expected NOTRUN] subtest: Navigate a window via anchor with javascript:-urls w/ default policy in report-only mode.
    • PASS [expected NOTRUN] subtest: Navigate a frame via anchor with javascript:-urls in enforcing mode.
  • TIMEOUT /trusted-types/trusted-types-navigation.html?06-10 (#37920)
    • TIMEOUT [expected FAIL] subtest: Navigate a frame via anchor with javascript:-urls in report-only mode.

      Test timed out

    • NOTRUN [expected TIMEOUT] subtest: Navigate a frame via anchor with javascript:-urls w/ default policy in report-only mode.
  • OK [expected TIMEOUT] /trusted-types/trusted-types-navigation.html?31-35 (#38034)
    • PASS [expected TIMEOUT] subtest: Navigate a frame via form-submission with javascript:-urls w/ default policy in report-only mode.
    • FAIL [expected NOTRUN] subtest: Navigate a window via form-submission with javascript:-urls w/ a default policy throwing an exception in enforcing mode.

      promise_test: Unhandled rejection with value: "Unexpected message received: \"No securitypolicyviolation reported!\""

    • FAIL [expected NOTRUN] subtest: Navigate a window via form-submission with javascript:-urls w/ a default policy throwing an exception in report-only mode.

      promise_test: Unhandled rejection with value: "Unexpected message received: \"No securitypolicyviolation reported!\""

    • FAIL [expected NOTRUN] subtest: Navigate a window via form-submission with javascript:-urls w/ a default policy making the URL invalid in enforcing mode.

      promise_test: Unhandled rejection with value: "Unexpected message received: \"No securitypolicyviolation reported!\""

  • OK /trusted-types/trusted-types-reporting.html (#43737)
    • PASS [expected FAIL] subtest: Trusted Type violation report: sample for SVGScriptElement href assignment by setAttribute
  • OK /visual-viewport/resize-event-order.html (#41981)
    • PASS [expected FAIL] subtest: Popup: DOMWindow resize fired before VisualViewport.
  • OK /webaudio/the-audio-api/the-audiobuffersourcenode-interface/sub-sample-buffer-stitching.html (#22849)
    • FAIL [expected PASS] subtest: buffer-stitching-2

      assert_approx_equals: Stitched sine‑wave buffers at sample rate 43800 sample[36398] |1.394802131168224e+29 - 0.825837254524231| = 1.394802131168224e+29 &gt; 0.0038986 expected 0.825837254524231 +/- 0.0038986 but got 1.394802131168224e+29

  • OK /webdriver/tests/classic/accept_alert/accept.py (#43194)
    • FAIL [expected PASS] subtest: test_null_response_value

      AssertionError: no such alert (404): No user prompt is currently active.

  • OK /webdriver/tests/classic/close_window/close.py
    • ERROR [expected PASS] subtest: test_no_top_browsing_context

      setup error: webdriver.error.NoSuchElementException: no such element (404)

    • FAIL [expected PASS] subtest: test_no_browsing_context

      webdriver.error.NoSuchElementException: no such element (404)

  • OK /webdriver/tests/classic/dismiss_alert/dismiss.py (#39098)
    • FAIL [expected PASS] subtest: test_null_response_value

      AssertionError: no such alert (404): No user prompt is currently active.

  • OK /webdriver/tests/classic/execute_script/properties.py
    • FAIL [expected PASS] subtest: test_content_attribute

      AssertionError: no such window (404): No such window

  • OK [expected ERROR] /webxr/render_state_update.https.html (#27535)
  • CRASH [expected ERROR] /workers/Worker-constructor-proto.any.serviceworker.html
  • CRASH [expected ERROR] /workers/WorkerNavigator-hardware-concurrency.any.sharedworker.html
  • ERROR [expected OK] /workers/baseurl/alpha/sharedworker-in-worker.html (#21315)
  • TIMEOUT [expected OK] /x-frame-options/redirect.html
Stable unexpected results that are known to be intermittent (18)
  • OK /IndexedDB/idbdatabase_deleteObjectStore.any.worker.html (#43776)
    • PASS [expected FAIL] subtest: Deleted object store's name should be removed from database's list. Attempting to use a deleted IDBObjectStore should throw an InvalidStateError
  • FAIL [expected PASS] /_mozilla/mozilla/sslfail.html (#10760)
  • TIMEOUT [expected OK] /_mozilla/mozilla/window_resize_event.html (#36741)
    • TIMEOUT [expected PASS] subtest: Popup onresize event fires after resizeTo

      Test timed out

  • CRASH [expected PASS] /_mozilla/shadow-dom/move-element-with-ua-shadow-tree-crash.html (#39473)
  • OK /_webgl/conformance/textures/misc/texture-upload-size.html (#21770)
    • FAIL [expected PASS] subtest: WebGL test #45

      assert_true: Texture was smaller than the expected size 2x2 expected true got false

    • FAIL [expected PASS] subtest: WebGL test #47

      assert_true: getError expected: INVALID_VALUE. Was NO_ERROR : when calling texSubImage2D with the same texture upload with offset 1, 1 expected true got false

    • FAIL [expected PASS] subtest: WebGL test #49

      assert_true: Texture was smaller than the expected size 2x2 expected true got false

    • FAIL [expected PASS] subtest: WebGL test #51

      assert_true: getError expected: INVALID_VALUE. Was NO_ERROR : when calling texSubImage2D with the same texture upload with offset 1, 1 expected true got false

    • PASS [expected FAIL] subtest: WebGL test #53
    • PASS [expected FAIL] subtest: WebGL test #55
    • PASS [expected FAIL] subtest: WebGL test #57
    • PASS [expected FAIL] subtest: WebGL test #59
    • FAIL [expected PASS] subtest: WebGL test #61

      assert_true: Texture was smaller than the expected size 2x2 expected true got false

    • FAIL [expected PASS] subtest: WebGL test #63

      assert_true: getError expected: INVALID_VALUE. Was NO_ERROR : when calling texSubImage2D with the same texture upload with offset 1, 1 expected true got false

    • And 6 more unexpected results...
  • CRASH [expected ERROR] /_webgl/conformance2/misc/uninitialized-test-2.html (#41656)
  • OK /beacon/beacon-basic.https.window.html (#41723)
    • PASS [expected FAIL] subtest: Payload size restriction should be accumulated: type = arraybuffer
  • OK /css/css-fonts/generic-family-keywords-001.html (#37467)
    • PASS [expected FAIL] subtest: @font-face matching for quoted and unquoted generic(fangsong)
  • OK /css/css-fonts/generic-family-keywords-003.html (#38994)
    • PASS [expected FAIL] subtest: @font-face matching for quoted and unquoted system-ui (drawing text in a canvas)
    • PASS [expected FAIL] subtest: @font-face matching for quoted and unquoted generic(fangsong) (drawing text in a canvas)
    • PASS [expected FAIL] subtest: @font-face matching for quoted and unquoted generic(kai) (drawing text in a canvas)
    • FAIL [expected PASS] subtest: @font-face matching for quoted and unquoted generic(khmer-mul) (drawing text in a canvas)

      assert_equals: quoted generic(khmer-mul) matches  @font-face rule expected 125 but got 40

    • PASS [expected FAIL] subtest: @font-face matching for quoted and unquoted generic(nastaliq) (drawing text in a canvas)
  • ERROR [expected OK] /fetch/fetch-later/quota/same-origin-iframe/multiple-iframes.https.window.html (#35176)
  • OK [expected ERROR] /fetch/fetch-later/quota/same-origin-iframe/sandboxed-iframe.https.window.html (#41704)
  • OK /fetch/metadata/generated/css-font-face.https.sub.tentative.html (#32732)
    • FAIL [expected PASS] subtest: sec-fetch-user

      promise_test: Unhandled rejection with value: object "Error: Failed to query for recorded headers."

  • OK /html/browsers/browsing-the-web/navigating-across-documents/005.html (#27062)
    • PASS [expected FAIL] subtest: Link with onclick navigation and href navigation
  • OK /html/browsers/browsing-the-web/navigating-across-documents/replace-before-load/a-click.html (#28697)
    • FAIL [expected PASS] subtest: aElement.click() before the load event must NOT replace

      assert_equals: expected "http://web-platform.test:8000/common/blank.html?thereplacement" but got "http://web-platform.test:8000/html/browsers/browsing-the-web/navigating-across-documents/replace-before-load/resources/code-injector.html?pipe=sub(none)&amp;code=%0A%20%20%20%20const%20a%20%3D%20document.createElement(%22a%22)%3B%0A%20%20%20%20a.href%20%3D%20%22%2Fcommon%2Fblank.html%3Fthereplacement%22%3B%0A%20%20%20%20document.currentScript.before(a)%3B%0A%20%20%20%20a.click()%3B%0A%20%20"

  • TIMEOUT /html/interaction/focus/the-autofocus-attribute/supported-elements.html (#24145)
    • TIMEOUT [expected FAIL] subtest: Element with tabindex should support autofocus

      Test timed out

    • NOTRUN [expected PASS] subtest: Non-HTMLElement should not support autofocus
  • OK /html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url.any.worker-module.html (#43510)
    • FAIL [expected PASS] subtest: Revoking a blob URL immediately after calling import will not fail

      promise_test: Unhandled rejection with value: object "TypeError: Module fetching failed"

  • OK /mixed-content/tentative/autoupgrades/mixed-content-cors.https.sub.html (#41123)
    • PASS [expected FAIL] subtest: Cross-Origin video should get upgraded even if CORS is set
  • OK /resource-timing/test_resource_timing.html (#25720)
    • PASS [expected FAIL] subtest: PerformanceEntry has correct name, initiatorType, startTime, and duration (link)
Stable unexpected results (1)
  • TIMEOUT [expected OK] /preload/modulepreload-sri-importmap.html
    • TIMEOUT [expected PASS] subtest: Script should not be loaded if modulepreload's integrity is invalid

      Test timed out

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown

⚠️ Try run (#24169289463) failed!

@jdm

jdm commented Apr 9, 2026

Copy link
Copy Markdown
Member

That test timeout is unrelated.

@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 9, 2026
@Messi002
media: Clean up shadow root content when removing controls
bd06dad 
Signed-off-by: Messi002 <rostandmessi2@gmail.com>
@Messi002

Messi002 commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

ok @jdm
thank you

@jdm jdm dismissed Gae24’s stale review April 9, 2026 13:20

Changes reverted .

@jdm jdm added this pull request to the merge queue Apr 9, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 9, 2026
Merged via the queue into servo:main with commit 4029f19 Apr 9, 2026
31 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdm jdm jdm approved these changes

@Gae24 Gae24 Gae24 left review comments

@gterzian gterzian Awaiting requested review from gterzian gterzian is a code owner

+1 more reviewer

@rayguo17 rayguo17 rayguo17 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S-awaiting-review There is new code that needs to be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Media: Controls widget would block media element from being Garbage Collected.

5 participants

@Messi002 @rayguo17 @jdm @Gae24 @servo-highfive