script: Decouple ownership of the media element from IPC router callback#41131
script: Decouple ownership of the media element from IPC router callback#41131jdm merged 1 commit intoservo:mainfrom
Conversation
9e85d5e to
4fa829f
Compare
|
🔨 Triggering try run (#20025577970) for Linux (WPT) |
|
Test results for linux-wpt from try job (#20025577970): Flaky unexpected result (28)
Stable unexpected results that are known to be intermittent (25)
Stable unexpected results (3)
|
|
|
4fa829f to
8c4e035
Compare
|
🔨 Triggering try run (#20029615563) for Linux (WPT) |
|
@rayguo17 < If you is observing some media resource leakage in OHOS media backend it should a little bit helpful ( gstreamer media backend is still buggy) |
|
Test results for linux-wpt from try job (#20029615563): Flaky unexpected result (33)
Stable unexpected results that are known to be intermittent (25)
Stable unexpected results (1)
|
|
|
8c4e035 to
efd92da
Compare
|
Added missing |
|
🔨 Triggering try run (#20030944290) for Linux (WPT) |
|
Test results for linux-wpt from try job (#20030944290): Flaky unexpected result (35)
Stable unexpected results that are known to be intermittent (22)
|
|
✨ Try run (#20030944290) succeeded. |
|
I am not yet sufficiently familiar with memory management to feel comfortable reviewing this PR. I will read along the reviews of others to build up more knowledge in this area. |
efd92da to
228af82
Compare
|
🔨 Triggering try run (#20058800225) for Linux (WPT) |
|
Test results for linux-wpt from try job (#20058800225): Flaky unexpected result (34)
Stable unexpected results that are known to be intermittent (24)
|
|
✨ Try run (#20058800225) succeeded. |
mrobinson
left a comment
There was a problem hiding this comment.
Seems okay, but I am not certain of the safety of sending the WeakRefs across thread boundaries or if there is a better alternative.
| #[expect(unsafe_code)] | ||
| unsafe impl Send for HTMLMediaElementEventHandler {} |
There was a problem hiding this comment.
I'm actually not sure if it is safe to send a DOM WeakRef across thread boundaries. I think this is a question for @jdm.
There was a problem hiding this comment.
Sending a DOM WeakRef across thread boundaries is not safe by itself, but with the fine-granted usage of a Arc<T> ownership + Weak<T> upgrade (inside the IPC callback's queued task on the script thread) give to us this safety.
If in future someone decide to change it, the assert inside HTMLMediaElementEventHandler::drop will notify about broken safety.
There was a problem hiding this comment.
Yeah, this is a pattern that it may make sense to generalize in the future if other DOM types need weak references from other threads.
| audio_renderer: DomRefCell<Option<Arc<Mutex<dyn AudioRenderer>>>>, | ||
| #[conditional_malloc_size_of] | ||
| #[no_trace] | ||
| event_handler: RefCell<Option<Arc<Mutex<HTMLMediaElementEventHandler>>>>, |
There was a problem hiding this comment.
Maybe a OnceCell makes more sense here?
There was a problem hiding this comment.
More suitable here Cell<T> but T is not copyable type and MallocSizeOf trait bound is not satisfied, so RefCell will be used.
And yes lazy OnceCell could be used here as well but the point is to drop WeakRef as soon as possible (on new media source loading with possible source selection error) and do not preserve it till the end when the media element will be GC-ed.
Added new invokation struct `HTMLMediaElementEventHandler` that allows to decouple ownership of the `HTMLMediaElement` from the IPC router callback (break cyclic referrence dependency) by replacing a `Trusted` wrapper with more suitable `WeakRef`. Note that `WeakRef` is not thread-safe and MUST be rooted and/or deleted on the script thread. Testing: Regressions in the following tests caused by overly aggressive GC on document activity change to `Inactive` and will be follow up in servo#41138 - html/semantics/embedded-content/media-elements/autoplay-allowed-by-feature-policy.https.sub.html - html/semantics/embedded-content/media-elements/autoplay-default-feature-policy.https.sub.html - html/semantics/embedded-content/media-elements/autoplay-disabled-by-feature-policy.https.sub.html Fixes: servo#40243 Signed-off-by: Andrei Volykhin <[email protected]>
228af82 to
08499cc
Compare
The better alternative (per my opinion) is to add another invocation level to remove AS_IS: TO_BE: Note that a |
|
@jdm < Please review and add to merge queue if PR is OK. |
Added new invocation struct
HTMLMediaElementEventHandlerthat allows to decouple ownership of theHTMLMediaElementfrom the IPC router callback (break cyclic reference dependency) by replacing aTrustedwrapper with more suitableWeakRef.Note that
WeakRefis not thread-safe and MUST be rooted and/or deleted on the script thread.Testing: Regressions in the following tests caused by overly aggressive GC on document activity change to
Inactiveand will be follow up in #41138Fixes: #40243