New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 698034 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Android MediaRouter only (left Chro...
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug


Participants' hotlists:
Media-Controls


Sign in to add a comment

FATAL:MediaCustomControlsFullscreenDetector.cpp(131)] Check failed: isVideoOrParentFullscreen().

Project Member Reported by dalecur...@chromium.org, Mar 2 2017

Issue description

[1:1:0302/134142.178401:1131500925042:FATAL:MediaCustomControlsFullscreenDetector.cpp(131)] Check failed: isVideoOrParentFullscreen(). 
STDERR: #0 0x0000004dec61 __interceptor_backtrace
STDERR: #1 0x7f6687872dbc base::debug::StackTrace::StackTrace()
STDERR: #2 0x7f66878f3fc7 logging::LogMessage::~LogMessage()
STDERR: #3 0x7f6675459981 blink::MediaCustomControlsFullscreenDetector::onCheckViewportIntersectionTimerFired()
STDERR: #4 0x7f667b792129 blink::TimerBase::runInternal()
STDERR: #5 0x7f667b792e1f _ZN4base8internal13FunctorTraitsIMN5blink9TimerBaseEFvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_
STDERR: #6 0x7f66878755e9 base::debug::TaskAnnotator::RunTask()
STDERR: #7 0x7f667bec1932 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
STDERR: #8 0x7f667beba449 blink::scheduler::TaskQueueManager::DoWork()
STDERR: #9 0x7f667beca018 _ZN4base8internal13FunctorTraitsIMN5blink9scheduler16TaskQueueManagerEFvbEvE6InvokeIRKNS_7WeakPtrIS4_EEJRKbEEEvS6_OT_DpOT0_
STDERR: #10 0x7f667be99a9a base::CancelableCallback<>::Forward()
STDERR: #11 0x7f667be9a0af _ZN4base8internal13FunctorTraitsIMNS_18CancelableCallbackIFvvEEEKFvvEvE6InvokeIRKNS_7WeakPtrIS4_EEJEEEvS6_OT_DpOT0_
STDERR: #12 0x7f66878755e9 base::debug::TaskAnnotator::RunTask()
STDERR: #13 0x7f66879233fb base::MessageLoop::RunTask()
STDERR: #14 0x7f66879243b6 base::MessageLoop::DeferOrRunPendingTask()
STDERR: #15 0x7f6687926118 base::MessageLoop::DoDelayedWork()
STDERR: #16 0x7f668792def3 base::MessagePumpDefault::Run()
STDERR: #17 0x7f6687922a01 base::MessageLoop::RunHandler()
STDERR: #18 0x7f66879d6b91 base::RunLoop::Run()
STDERR: #19 0x7f668b972317 content::RendererMain()
STDERR: #20 0x7f668bf0755f content::RunZygote()
STDERR: #21 0x7f668bf08a90 content::RunNamedProcessTypeMain()
STDERR: #22 0x7f668bf0a7db content::ContentMainRunnerImpl::Run()
STDERR: #23 0x7f668bf05a1b content::ContentMain()
STDERR: #24 0x000000568288 main
STDERR: #25 0x7f6666605f45 __libc_start_main
STDERR: #26 0x00000048fa09 <unknown>

Seen on Linux while running layout tests under ASAN w/ DCHECK enabled:

media/controls-right-click-on-timebar.html
media/no-autoplay-with-user-gesture-requirement.html 
media/media-continues-playing-after-replace-source.html
 
Cc: mlamouri@chromium.org
Looking into it.
Hmm, this should not happen. These tests have nothing to do with fullscreen, the timer should never fire.

Dale, is this reproducible if you only run these tests?
Could the timer be started by a test that was previously running? Can you
try to run all the media/ tests and see if you can reproduce?
A potential sequence hitting the DCHECK is:

1. The video or its parent goes fullscreen, MediaCustomControlsFullscreenDetector starts the timer
2. The document exits fullscreen, queueing a fullscreenchange event
3. The MediaCustomControlsFullscreenDetector fires before the fullscreenchange event fires.

We should do an early return instead of DCHECK() I think.

However, I'm still wondering how these three tests can hit the DCHECK()
It actually fails randomly on different tests. So I guess the order of test execution should matter.
Let's write a test for the "potential sequence" you have in comment #4. If that actually triggers the DCHECK, we can fix it and see if it also fixes the random DCHECK. I wouldn't feel comfortable just removing the DCHECK without understanding why it's happening.
Turns out to be the the timer for the previous test page fired after navigating to the next test page, and the exiting fullscreenchange event is never fired for the previous test page as it is gone anyway.

I'm trying to use ContextLifecycleObserver, and stop the timer when the context is gone.
Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe180eb4effd53064142647a839b89d3910dfff0

commit fe180eb4effd53064142647a839b89d3910dfff0
Author: zqzhang <zqzhang@chromium.org>
Date: Thu Mar 09 14:12:15 2017

Fixing a crash in MediaCustomControlsFullscreenDetector when the page is destroyed

There's a situation that hits a DCHECK() in
MediaCustomControlsFullscreenDetector when the video become
fullscreen but the page navigates away before the timer fires.
This CL solves the issue by stopping the timer when the
ExecutionContext is destroyed.

This CL also moves the common parts of MockWebMediaPlayer into
EmptyWebMediaPlayer. There are still some other
MockWebMediaPlayers which will be adopted in a follow-up.

BUG= 698034 

Review-Url: https://codereview.chromium.org/2729613007
Cr-Commit-Position: refs/heads/master@{#455740}

[modify] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/core/html/HTMLMediaElement.h
[add] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp
[modify] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/core/html/HTMLVideoElement.h
[modify] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp
[modify] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp
[modify] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h
[delete] https://crrev.com/621af2a70da9fdeb86951854796385eda8924cf1/third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
[modify] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/platform/BUILD.gn
[add] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.cpp
[add] https://crrev.com/fe180eb4effd53064142647a839b89d3910dfff0/third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h

Labels: Merge-Request-58
This added a flaky/broken test (on WebKit Linux Trusty dbg). Please do not Merge!

I'm snoozing the error for now, but please fix the test or the change will be reverted.
Labels: -Merge-Request-58
Thanks for letting me know. Looking into it. Removing the merge request.
 Issue 700207  has been merged into this issue.
Labels: Merge-Request-58
The test has been fixed, requesting to merge #9 and #15 into M58.
Project Member

Comment 17 by sheriffbot@chromium.org, Mar 11 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), bhthompson@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Please merge your change to M58 branch 3029 before 5:00 PM PT, Monday (03/13/17) so we can take it in for next week dev release. Thank you!

Project Member

Comment 19 by bugdroid1@chromium.org, Mar 12 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bb43d450f514993aa48475c759af3a072fa6038

commit 7bb43d450f514993aa48475c759af3a072fa6038
Author: Zhiqiang Zhang <zqzhang@google.com>
Date: Sun Mar 12 18:33:37 2017

Fixing a crash in MediaCustomControlsFullscreenDetector when the page is destroyed

There's a situation that hits a DCHECK() in
MediaCustomControlsFullscreenDetector when the video become
fullscreen but the page navigates away before the timer fires.
This CL solves the issue by stopping the timer when the
ExecutionContext is destroyed.

This CL also moves the common parts of MockWebMediaPlayer into
EmptyWebMediaPlayer. There are still some other
MockWebMediaPlayers which will be adopted in a follow-up.

BUG= 698034 

Review-Url: https://codereview.chromium.org/2729613007
Cr-Commit-Position: refs/heads/master@{#455740}
(cherry picked from commit fe180eb4effd53064142647a839b89d3910dfff0)

Review-Url: https://codereview.chromium.org/2747633002 .
Cr-Commit-Position: refs/branch-heads/3029@{#136}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/core/html/HTMLMediaElement.h
[add] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp
[modify] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/core/html/HTMLVideoElement.h
[modify] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp
[modify] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp
[modify] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h
[delete] https://crrev.com/4f3f496493209a314f968d9d5fdd2bc75982bd68/third_party/WebKit/Source/core/html/shadow/MediaControlsLeakTest.cpp
[modify] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/platform/BUILD.gn
[add] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.cpp
[add] https://crrev.com/7bb43d450f514993aa48475c759af3a072fa6038/third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h

Project Member

Comment 20 by bugdroid1@chromium.org, Mar 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cca045b0ea07b27182777033f531130b80066575

commit cca045b0ea07b27182777033f531130b80066575
Author: Zhiqiang Zhang <zqzhang@google.com>
Date: Sun Mar 12 18:36:17 2017

Fix an issue in fullscreen detector when context is destroyed

There's a race condition causing the WebMediaPlayer to be destroyed
before MediaCustomControlsFullscreenDetector dispatches the final
change. This CL fixes the issue.

BUG= 698034 

Review-Url: https://codereview.chromium.org/2743573003
Cr-Commit-Position: refs/heads/master@{#456160}
(cherry picked from commit ac5c34a5e762fd095bf129de24512100be09872a)

Review-Url: https://codereview.chromium.org/2741343002 .
Cr-Commit-Position: refs/branch-heads/3029@{#137}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/cca045b0ea07b27182777033f531130b80066575/third_party/WebKit/Source/core/html/HTMLMediaElement.h
[modify] https://crrev.com/cca045b0ea07b27182777033f531130b80066575/third_party/WebKit/Source/core/html/HTMLVideoElement.cpp
[modify] https://crrev.com/cca045b0ea07b27182777033f531130b80066575/third_party/WebKit/Source/core/html/HTMLVideoElement.h
[modify] https://crrev.com/cca045b0ea07b27182777033f531130b80066575/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp
[modify] https://crrev.com/cca045b0ea07b27182777033f531130b80066575/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h

Status: Fixed (was: Started)
Cc: zqzh...@chromium.org
 Issue 700393  has been merged into this issue.

Sign in to add a comment