Fix stale observation crash in LazyLoadMediaObserver on media element removal [chromium/src : main]
0 views
Skip to first unread message
Iris Zhou (Gerrit)
May 6, 2026, 12:23:31 PM (6 days ago) May 6
to Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Piet Schouten
Iris Zhou added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Iris Zhou . resolved
Hey PH, mind taking a look at this when you have the chance, before I add upstream reviewers?
Related details
Attention is currently required from:
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Piet Schouten (Gerrit)
May 7, 2026, 8:19:48 PM (5 days ago) May 7
to Iris Zhou, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Iris Zhou
Piet Schouten added 3 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_media_element.cc
Line 877, Patchset 1 (Latest):
if (RuntimeEnabledFeatures::LazyLoadVideoAndAudioEnabled() &&Piet Schouten . unresolved
nit: The `InsertedInto` method above checks `insertion_point.isConnected()` before the call to `StartMonitoring`. Even though `StartMonitoring` and `StopMonitoring` check for document validity with `GetRootDocumentOrNull`, should the conditional here mirror what `InsertedInto` does and also call `insertion_point.isConnected()`?
File third_party/blink/renderer/core/html/media/lazy_load_media_observer_test.cc
Line 643, Patchset 1 (Latest):
EXPECT_GT(observer.GetObservationCountForTesting(), 0u);Piet Schouten . unresolved
nit: since there should only be 1 observer (please correct me if I'm wrong), could this check be tightened to the following to catch any other related bugs?
```suggestion
EXPECT_EQ(observer.GetObservationCountForTesting(), 1u);
```
Line 654, Patchset 1 (Latest):
}Piet Schouten . unresolved
nit: You mentioned in the comment above that "If the element is re-inserted, InsertedInto() will restart monitoring.". Would it be beneficial to re-insert the element to make sure this statement is correct? Perhaps something like:
```suggestion
GetDocument().body()->AppendChild(video);
EXPECT_EQ(observer.GetObservationCountForTesting(), 1u);
}
```
Related details
Attention is currently required from:
- Iris Zhou
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Iris Zhou (Gerrit)
May 8, 2026, 1:22:14 PM (4 days ago) May 8
to Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Piet Schouten
Iris Zhou added 3 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_media_element.cc
Line 877, Patchset 1:
if (RuntimeEnabledFeatures::LazyLoadVideoAndAudioEnabled() &&Piet Schouten . resolved
nit: The `InsertedInto` method above checks `insertion_point.isConnected()` before the call to `StartMonitoring`. Even though `StartMonitoring` and `StopMonitoring` check for document validity with `GetRootDocumentOrNull`, should the conditional here mirror what `InsertedInto` does and also call `insertion_point.isConnected()`?
Iris Zhou
I took a look, seems like `isConnected()` could be either `true` (direct removal) or `false` (subtree removal, e.g. removing a `<div>` that contains the video). We want to stop monitoring here either way, so I don't think we want the check here.
File third_party/blink/renderer/core/html/media/lazy_load_media_observer_test.cc
Line 643, Patchset 1:
EXPECT_GT(observer.GetObservationCountForTesting(), 0u);Piet Schouten . resolved
nit: since there should only be 1 observer (please correct me if I'm wrong), could this check be tightened to the following to catch any other related bugs?
```suggestion
EXPECT_EQ(observer.GetObservationCountForTesting(), 1u);
```
Iris Zhou
Done
nit: You mentioned in the comment above that "If the element is re-inserted, InsertedInto() will restart monitoring.". Would it be beneficial to re-insert the element to make sure this statement is correct? Perhaps something like:
```suggestion
GetDocument().body()->AppendChild(video);
EXPECT_EQ(observer.GetObservationCountForTesting(), 1u);
}
```
Attention is currently required from:
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Iris Zhou (Gerrit)
May 11, 2026, 12:12:31 PM (yesterday) May 11
to Dale Curtis, Helmut Januschka, Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Dale Curtis, Helmut Januschka and Piet Schouten
Iris Zhou added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Iris Zhou . resolved
Hi Dale and Helmut, adding the two of you because you were on the original crbug for HTML video element lazy-loading. Please take a look when you have a chance, thanks!
Related details
Attention is currently required from:
- Dale Curtis
- Helmut Januschka
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
May 11, 2026, 12:45:40 PM (yesterday) May 11
to Iris Zhou, Helmut Januschka, Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Helmut Januschka, Iris Zhou and Piet Schouten
Dale Curtis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
Patchset-level comments
Iris Zhou . unresolved
Hi Dale and Helmut, adding the two of you because you were on the original crbug for HTML video element lazy-loading. Please take a look when you have a chance, thanks!
Dale Curtis
Thanks! This looks right to me, but please let Helmut weigh in.
Related details
Attention is currently required from:
- Helmut Januschka
- Iris Zhou
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
May 11, 2026, 2:19:13 PM (yesterday) May 11
to Iris Zhou, Helmut Januschka, Dale Curtis, Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Iris Zhou and Piet Schouten
Helmut Januschka voted and added 2 comments![]( "Open in Gerrit")
Votes added by Helmut Januschka
| Code-Review | +1 |
2 comments
Patchset-level comments
Helmut Januschka . resolved
awesome! looking good thanks for looping me in, and thanks for fixing the bug!
File third_party/blink/renderer/core/html/media/html_media_element.cc
Line 876, Patchset 2 (Latest):
// InsertedInto() will restart monitoring.Helmut Januschka . unresolved
optional nit: i'd remove this comment
Related details
Attention is currently required from:
- Iris Zhou
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Iris Zhou (Gerrit)
May 11, 2026, 2:53:00 PM (yesterday) May 11
to Helmut Januschka, Dale Curtis, Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Dale Curtis, Helmut Januschka and Piet Schouten
Iris Zhou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Iris Zhou
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
File third_party/blink/renderer/core/html/media/html_media_element.cc
optional nit: i'd remove this comment
Attention is currently required from:
- Dale Curtis
- Helmut Januschka
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Iris Zhou (Gerrit)
May 11, 2026, 2:54:35 PM (yesterday) May 11
to Helmut Januschka, Dale Curtis, Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Dale Curtis, Helmut Januschka and Piet Schouten
Iris Zhou added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Dale CurtisHi Dale and Helmut, adding the two of you because you were on the original crbug for HTML video element lazy-loading. Please take a look when you have a chance, thanks!
Thanks! This looks right to me, but please let Helmut weigh in.
Iris Zhou
Done
Related details
Attention is currently required from:
- Dale Curtis
- Helmut Januschka
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
May 11, 2026, 3:37:08 PM (yesterday) May 11
to Iris Zhou, Helmut Januschka, Piet Schouten, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Helmut Januschka, Iris Zhou and Piet Schouten
Dale Curtis voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Helmut Januschka
- Iris Zhou
- Piet Schouten
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Piet Schouten (Gerrit)
May 11, 2026, 4:49:52 PM (yesterday) May 11
to Iris Zhou, Dale Curtis, Helmut Januschka, Chromium LUCI CQ, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Attention needed from Helmut Januschka and Iris Zhou
Piet Schouten voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Helmut Januschka
- Iris Zhou
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
May 11, 2026, 5:59:12 PM (yesterday) May 11
to Iris Zhou, Piet Schouten, Dale Curtis, Helmut Januschka, chromium...@chromium.org, srirama chandra sekhar, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Fix stale observation crash in LazyLoadMediaObserver on media element removal
The LazyLoadVideoAndAudio feature (crbug.com/469111735) added media
elements to LazyLoadMediaObserver but did not add cleanup in
HTMLMediaElement::RemovedFrom. When a <video loading="lazy"> element
is removed from the DOM, its IntersectionObservation remains in the
observer. If garbage collection later collects the element while the
observation is held alive by active_observations_, a subsequent call
to LoadAllImagesAndBlockLoadEvent (e.g. via print preview) can
dereference the null target, causing an ACCESS_VIOLATION crash.
This crash is non-deterministic, depending on GC timing relative to
intersection computation and notification delivery.
This CL:
- Calls LazyMediaHelper::StopMonitoring in
HTMLMediaElement::RemovedFrom when the element is in the deferred
lazy-load state, matching the cleanup that ImageLoader already does
for images.
- Adds a regression test verifying that the observation is cleaned up
after removal.
Bug: 510024516
Co-authored-by: Copilot <22355621...@users.noreply.github.com>
Change-Id: Idfe623d59e90351d21fc6515b869cf4c91445862
Auto-Submit: Iris Zhou <irz...@microsoft.com>
Reviewed-by: Piet Schouten <Piet.S...@microsoft.com>
Commit-Queue: Piet Schouten <Piet.S...@microsoft.com>
Commit-Queue: Iris Zhou <irz...@microsoft.com>
Reviewed-by: Dale Curtis <dalec...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1628839}
Files:
- M third_party/blink/renderer/core/html/media/html_media_element.cc
- M third_party/blink/renderer/core/html/media/lazy_load_media_observer.cc
- M third_party/blink/renderer/core/html/media/lazy_load_media_observer.h
- M third_party/blink/renderer/core/html/media/lazy_load_media_observer_test.cc
Change size: M
Delta: 4 files changed, 64 insertions(+), 0 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Dale Curtis, +1 by Piet Schouten
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages