Crash when blink::HTMLMediaElement::CheckViewportIntersectionTimerFired() is called |
||||||||
Issue descriptionWhen the media element's viewport intersection timer fires, sometimes this will cause a DCHECK crash (stack trace pasted below). The root cause seems to be that the layout needs updating, so blink::IntersectionGeometry::ClipToRoot() isn't able to do its thing. [1:1:1116/144528.078447:FATAL:intersection_geometry.cc(172)] Check failed: frame_view->ShouldThrottleRendering() || !layout_view || !(layout_view->NeedsPaintPropertyUpdate() || layout_view->DescendantNeedsPaintPropertyUpdate()). #0 0x7f72d7fc6ecf base::debug::StackTrace::StackTrace() #1 0x7f72d7eec13b logging::LogMessage::~LogMessage() #2 0x7f72cf70030e blink::IntersectionGeometry::ClipToRoot() #3 0x7f72cf7006b7 blink::IntersectionGeometry::ComputeGeometry() #4 0x7f72cf5511bb blink::HTMLMediaElement::CheckViewportIntersectionTimerFired() #5 0x7f72cda6b2f3 blink::TimerBase::RunInternal() #6 0x7f72cd93be54 _ZN4base8internal7InvokerINS0_9BindStateIMN5blink22CanvasResourceProvider19CanvasImageProviderEFvvEJNS_7WeakPtrIS5_EEEEEFvvEE7RunOnceEPNS0_13BindStateBaseE #7 0x7f72cd92d9f9 WTF::ThreadCheckingCallbackWrapper<>::Run() #8 0x7f72d7eccbff base::debug::TaskAnnotator::RunTask() ...
,
Dec 13
Adding ReleaseBlock-Stable, as two separate media features would be crashing on users at random times. WIP...
,
Dec 19
MediaCustomControlsFullscreenDetector::OnCheckViewportIntersectionTimerFired should instantiate a new IntersectionObserver; observe the ViewElement(); and then return. The IntersectionObserver should use a callback which contains all the rest of the logic in that method; and it should also disconnect the IntersectionObserver. It will work because IntersectionObserver *always* cues up an initial notification whenever a new target is observed. So this logic will run at the next available opportunity (i.e., the next main frame).
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f634278ab6782379bf7045d4c268cabe3ea18ea commit 1f634278ab6782379bf7045d4c268cabe3ea18ea Author: Yuri Wiitala <miu@chromium.org> Date: Wed Dec 19 23:01:13 2018 IntersectionGeometry Crash fix (1 of 2): Add 'fraction of root' logic. "Fraction of root" semantics have been added as a configurable option to IntersectionObserver. Previously, it only calculated the "fraction of target element," which was not the metric required by the "dominant-in- viewport" detection logic in HTMLMediaElement. This change also happens to implement all but the JS API for the github feature request referenced below. Bug: https://github.com/w3c/IntersectionObserver/issues/124 Bug: 906258 Change-Id: Ib1402e7047a4a22dd756df6d8a20b457f76da7a6 Reviewed-on: https://chromium-review.googlesource.com/c/1381835 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Commit-Position: refs/heads/master@{#617994} [modify] https://crrev.com/1f634278ab6782379bf7045d4c268cabe3ea18ea/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc [modify] https://crrev.com/1f634278ab6782379bf7045d4c268cabe3ea18ea/third_party/blink/renderer/core/intersection_observer/intersection_observer.cc [modify] https://crrev.com/1f634278ab6782379bf7045d4c268cabe3ea18ea/third_party/blink/renderer/core/intersection_observer/intersection_observer.h [modify] https://crrev.com/1f634278ab6782379bf7045d4c268cabe3ea18ea/third_party/blink/renderer/core/intersection_observer/intersection_observer_test.cc
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89bcb37dff04afef84e0024c99f7770b539abad6 commit 89bcb37dff04afef84e0024c99f7770b539abad6 Author: Yuri Wiitala <miu@chromium.org> Date: Thu Dec 20 01:27:26 2018 IntersectionGeometry Crash fix (2 of 2): Use IsectObserver in media element. In html_media_element.cc, the repeating timer that fired to compute the intersection geometry, to determine the HTMLMediaElement's "dominant in viewport" status has been replaced with the use of IntersectionObserver. The part 1 of this 2-part change added the required functionality. Bug: 906258 Change-Id: Id27779276a572ce024ed0138e7e2171feab6a913 Reviewed-on: https://chromium-review.googlesource.com/c/1381838 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Commit-Position: refs/heads/master@{#618062} [modify] https://crrev.com/89bcb37dff04afef84e0024c99f7770b539abad6/third_party/blink/renderer/core/html/media/html_media_element.cc [modify] https://crrev.com/89bcb37dff04afef84e0024c99f7770b539abad6/third_party/blink/renderer/core/html/media/html_media_element.h [modify] https://crrev.com/89bcb37dff04afef84e0024c99f7770b539abad6/third_party/blink/renderer/core/html/media/media_custom_controls_fullscreen_detector.cc [modify] https://crrev.com/89bcb37dff04afef84e0024c99f7770b539abad6/third_party/blink/renderer/core/html/media_element_filling_viewport_test.cc
,
Dec 20
,
Dec 20
[Auto-generated comment by a script] We noticed that this issue is targeted for M-72; it appears the fix may have landed after branch point, meaning a merge might be required. The owner of this bug should confirm if a merge is required here. If so, add Merge-Request-72 label and indicate which commits/CLs are to be merged. Otherwise, remove Merge-TBD label. Thanks.
,
Dec 20
Requesting merge to M72. Risk analysis: There is a bit of new code here, as the OWNERS code reviewers rejected the initial, less-intrusive fix. That said, this code path will only activate when using tab mirroring (via the "Cast..." dialog). Also, I would not execute the merge until our internal QA process vets the fix. ;-)
,
Dec 21
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 21
Let's fully verify this in 73 dev . We can review the merge after holidays.
,
Jan 3
Now verified by QA team (both manual and automatic end-to-end testing lab). Ok to merge?
,
Jan 4
,
Jan 4
sounds good - thanks, branch:3626
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a6eea412ee8d349fc4dc43cd01d6045ad50340c commit 4a6eea412ee8d349fc4dc43cd01d6045ad50340c Author: Yuri Wiitala <miu@chromium.org> Date: Fri Jan 04 21:36:04 2019 IntersectionGeometry Crash fix (1 of 2): Add 'fraction of root' logic. "Fraction of root" semantics have been added as a configurable option to IntersectionObserver. Previously, it only calculated the "fraction of target element," which was not the metric required by the "dominant-in- viewport" detection logic in HTMLMediaElement. This change also happens to implement all but the JS API for the github feature request referenced below. Bug: https://github.com/w3c/IntersectionObserver/issues/124 Bug: 906258 Change-Id: Ib1402e7047a4a22dd756df6d8a20b457f76da7a6 Reviewed-on: https://chromium-review.googlesource.com/c/1381835 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#617994}(cherry picked from commit 1f634278ab6782379bf7045d4c268cabe3ea18ea) Reviewed-on: https://chromium-review.googlesource.com/c/1396446 Reviewed-by: Yuri Wiitala <miu@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#570} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/4a6eea412ee8d349fc4dc43cd01d6045ad50340c/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc [modify] https://crrev.com/4a6eea412ee8d349fc4dc43cd01d6045ad50340c/third_party/blink/renderer/core/intersection_observer/intersection_observer.cc [modify] https://crrev.com/4a6eea412ee8d349fc4dc43cd01d6045ad50340c/third_party/blink/renderer/core/intersection_observer/intersection_observer.h [modify] https://crrev.com/4a6eea412ee8d349fc4dc43cd01d6045ad50340c/third_party/blink/renderer/core/intersection_observer/intersection_observer_test.cc
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cc81cf00f6d1516987165bc38c23b552a2b8ffc commit 4cc81cf00f6d1516987165bc38c23b552a2b8ffc Author: Yuri Wiitala <miu@chromium.org> Date: Fri Jan 04 21:39:38 2019 IntersectionGeometry Crash fix (2 of 2): Use IsectObserver in media element. In html_media_element.cc, the repeating timer that fired to compute the intersection geometry, to determine the HTMLMediaElement's "dominant in viewport" status has been replaced with the use of IntersectionObserver. The part 1 of this 2-part change added the required functionality. Bug: 906258 Change-Id: Id27779276a572ce024ed0138e7e2171feab6a913 Reviewed-on: https://chromium-review.googlesource.com/c/1381838 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#618062}(cherry picked from commit 89bcb37dff04afef84e0024c99f7770b539abad6) Reviewed-on: https://chromium-review.googlesource.com/c/1396613 Reviewed-by: Yuri Wiitala <miu@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#571} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/4cc81cf00f6d1516987165bc38c23b552a2b8ffc/third_party/blink/renderer/core/html/media/html_media_element.cc [modify] https://crrev.com/4cc81cf00f6d1516987165bc38c23b552a2b8ffc/third_party/blink/renderer/core/html/media/html_media_element.h [modify] https://crrev.com/4cc81cf00f6d1516987165bc38c23b552a2b8ffc/third_party/blink/renderer/core/html/media/media_custom_controls_fullscreen_detector.cc [modify] https://crrev.com/4cc81cf00f6d1516987165bc38c23b552a2b8ffc/third_party/blink/renderer/core/html/media_element_filling_viewport_test.cc
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cc81cf00f6d1516987165bc38c23b552a2b8ffc Commit: 4cc81cf00f6d1516987165bc38c23b552a2b8ffc Author: miu@chromium.org Commiter: miu@chromium.org Date: 2019-01-04 21:39:38 +0000 UTC IntersectionGeometry Crash fix (2 of 2): Use IsectObserver in media element. In html_media_element.cc, the repeating timer that fired to compute the intersection geometry, to determine the HTMLMediaElement's "dominant in viewport" status has been replaced with the use of IntersectionObserver. The part 1 of this 2-part change added the required functionality. Bug: 906258 Change-Id: Id27779276a572ce024ed0138e7e2171feab6a913 Reviewed-on: https://chromium-review.googlesource.com/c/1381838 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#618062}(cherry picked from commit 89bcb37dff04afef84e0024c99f7770b539abad6) Reviewed-on: https://chromium-review.googlesource.com/c/1396613 Reviewed-by: Yuri Wiitala <miu@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#571} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by miu@google.com
, Nov 16