Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671993002:
[Blink>Media] Fix a crash in autoplay caused by delayed visibility observation (Closed)
|
Created:
3 years, 10 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 10 months ago Reviewers:
mlamouri (slow - plz ping) CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blink>Media] Fix a crash in autoplay caused by delayed visibility observation
There is a crash caused by delayed visibility observation after an
autoplay muted video gets unmuted, so that HTMLMediaElement tries to
stop the observer twice. However the pointer to the observer is set to
null on stop, thus a crash happens.
This CL adds null-check in
HTMLMediaElement::onVisibilityChangedForAutoplay() to avoid the crash.
BUG=686458
Review-Url: https://codereview.chromium.org/2671993002
Cr-Commit-Position: refs/heads/master@{#448641}
Committed: https://chromium.googlesource.com/chromium/src/+/0268543786283db4107d2362f338901d909545b1
Patch Set 1 #
Total comments: 3
Messages
Total messages: 16 (9 generated)
Description was changed from ========== [Blink>Media] Fix a crash in autoplay caused by delayed visibility observation There is a crash caused by delayed visibility observation after an autoplay muted video gets unmuted, so that HTMLMediaElement tries to stop the observer twice. However the pointer to the observer is set to null on stop, thus a crash happens. This CL adds null-check in HTMLMediaElement::onVisibilityChangedForAutoplay() to avoid the crash. BUG=686458 temporary ========== to ========== [Blink>Media] Fix a crash in autoplay caused by delayed visibility observation There is a crash caused by delayed visibility observation after an autoplay muted video gets unmuted, so that HTMLMediaElement tries to stop the observer twice. However the pointer to the observer is set to null on stop, thus a crash happens. This CL adds null-check in HTMLMediaElement::onVisibilityChangedForAutoplay() to avoid the crash. BUG=686458 ==========
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
PTAL The failing test is generated by the fuzz bots, which a modified version of autoplay-unmute.html. I prefer not to add the fuzz test into LayoutTest/media as it is not really a test human will write. Also the crash is a bit trivial to trigger as we need to hard code on ElementVisibilityObserver/IntersectionObserver.
https://codereview.chromium.org/2671993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2671993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4011: return; Why should we early return here? Shouldn't we instead do the check before calling stop?
https://codereview.chromium.org/2671993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2671993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4011: return; On 2017/02/06 12:55:43, mlamouri wrote: > Why should we early return here? Shouldn't we instead do the check before > calling stop? I think we should assume visibility change will never be fired after we stop the observer. Here's a corner case if we do the null-check before stop(): 1. A video with autoplay attribute gets loaded and becomes visible (however the signal is delayed) 2. The page calls video.muted = true 3. The page calls video.muted = false 4. The visibility change signal is delivered In this situation, the video will end up to be playing, however it is expected to be paused.
lgtm https://codereview.chromium.org/2671993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2671993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4011: return; On 2017/02/06 at 14:17:14, Zhiqiang Zhang wrote: > On 2017/02/06 12:55:43, mlamouri wrote: > > Why should we early return here? Shouldn't we instead do the check before > > calling stop? > > I think we should assume visibility change will never be fired after we stop the observer. > > Here's a corner case if we do the null-check before stop(): > 1. A video with autoplay attribute gets loaded and becomes visible (however the signal is delayed) > 2. The page calls video.muted = true > 3. The page calls video.muted = false > 4. The visibility change signal is delivered > > In this situation, the video will end up to be playing, however it is expected to be paused. I see. That makes sense :)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zqzhang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1486484481314420, "parent_rev":
"22930502f505e612f0a9eca2011aa3ddca3476ad", "commit_rev":
"0268543786283db4107d2362f338901d909545b1"}
Message was sent while issue was closed.
Description was changed from ========== [Blink>Media] Fix a crash in autoplay caused by delayed visibility observation There is a crash caused by delayed visibility observation after an autoplay muted video gets unmuted, so that HTMLMediaElement tries to stop the observer twice. However the pointer to the observer is set to null on stop, thus a crash happens. This CL adds null-check in HTMLMediaElement::onVisibilityChangedForAutoplay() to avoid the crash. BUG=686458 ========== to ========== [Blink>Media] Fix a crash in autoplay caused by delayed visibility observation There is a crash caused by delayed visibility observation after an autoplay muted video gets unmuted, so that HTMLMediaElement tries to stop the observer twice. However the pointer to the observer is set to null on stop, thus a crash happens. This CL adds null-check in HTMLMediaElement::onVisibilityChangedForAutoplay() to avoid the crash. BUG=686458 Review-Url: https://codereview.chromium.org/2671993002 Cr-Commit-Position: refs/heads/master@{#448641} Committed: https://chromium.googlesource.com/chromium/src/+/0268543786283db4107d2362f338... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/0268543786283db4107d2362f338...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2683783002/ by zqzhang@chromium.org. The reason for reverting is: Decided to fix the issue in IntersectionObserver instead of HTMLMediaElement. This CL will be reverted and be superseded by the new fix.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
