Fixing a crash in MediaCustomControlsFullscreenDetector when the page is destroyed

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}
Committed: https://chromium.googlesource.com/chromium/src/+/fe180eb4effd53064142647a839b89d3910dfff0

[Zhiqiang Zhang (Slow), 2017-03-03 20:07:16]

Description was changed from

==========
Fixing a crash in MediaCustomControlsFullscreenDetector when the page is
destroyed

BUG=698034
==========

to

==========
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.

BUG=698034
==========

[Zhiqiang Zhang (Slow), 2017-03-03 20:08:31]

zqzhang@chromium.org changed reviewers:
+ mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-03 20:08:31]

Initial patch. PTAL.

There are still a couple of MockWebMediaPlayers that haven't adopted to using
EmptyWebMediaPlayer. I'll do the rest if you are OK with it.

[Zhiqiang Zhang (Slow), 2017-03-03 20:08:39]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 20:08:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 21:33:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 21:33:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mlamouri (slow - plz ping), 2017-03-07 17:50:16]

lgtm -- thanks! :)

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp
(right):

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp:25:
class MockWebMediaPlayer : public EmptyWebMediaPlayer {
final?

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp:76:
using ::testing::Invoke;
Maybe this should be moved above the anonymous namespace? Not sure if it's part
of the style guide but it sounds surprising to see this here.

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h (right):

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h:12: class
EmptyWebMediaPlayer : public WebMediaPlayer {
Can you add class-level comments?

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h:14: void
load(LoadType, const WebMediaPlayerSource&, CORSMode) override {}
Add dtor?

[Zhiqiang Zhang (Slow), 2017-03-07 18:24:08]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 18:25:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhiqiang Zhang (Slow), 2017-03-07 18:26:19]

Description was changed from

==========
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.

BUG=698034
==========

to

==========
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
==========

[Zhiqiang Zhang (Slow), 2017-03-07 18:29:01]

zqzhang@chromium.org changed reviewers:
+ foolip@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-07 18:29:02]

+foolip: PTAL

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp
(right):

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp:25:
class MockWebMediaPlayer : public EmptyWebMediaPlayer {
On 2017/03/07 at 17:50:16, mlamouri wrote:
> final?

Done.

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp:76:
using ::testing::Invoke;
On 2017/03/07 at 17:50:16, mlamouri wrote:
> Maybe this should be moved above the anonymous namespace? Not sure if it's
part of the style guide but it sounds surprising to see this here.

Done.

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h (right):

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h:12: class
EmptyWebMediaPlayer : public WebMediaPlayer {
On 2017/03/07 at 17:50:16, mlamouri wrote:
> Can you add class-level comments?

Done.

https://codereview.chromium.org/2729613007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/EmptyWebMediaPlayer.h:14: void
load(LoadType, const WebMediaPlayerSource&, CORSMode) override {}
On 2017/03/07 at 17:50:16, mlamouri wrote:
> Add dtor?

Done.

[commit-bot: I haz the power, 2017-03-07 21:08:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 21:08:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhiqiang Zhang (Slow), 2017-03-09 10:43:55]

zqzhang@chromium.org changed reviewers:
+ jochen@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-09 10:43:55]

+jochen for EMEA third_party/WebKit review

[jochen (gone - plz use gerrit), 2017-03-09 11:29:34]

lgtm

[Zhiqiang Zhang (Slow), 2017-03-09 12:16:39]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-09 12:16:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2729613007/#ps60001
(title: "addressed mlamouri's comments")

[commit-bot: I haz the power, 2017-03-09 12:16:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 12:20:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 12:20:02]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Zhiqiang Zhang (Slow), 2017-03-09 12:39:48]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-03-09 12:39:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2729613007/#ps100001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-09 12:40:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 14:12:19]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489063188375740,
"parent_rev": "621af2a70da9fdeb86951854796385eda8924cf1", "commit_rev":
"fe180eb4effd53064142647a839b89d3910dfff0"}

[commit-bot: I haz the power, 2017-03-09 14:12:58]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/fe180eb4effd53064142647a839b...
==========

[commit-bot: I haz the power, 2017-03-09 14:13:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/fe180eb4effd53064142647a839b...

[qyearsley, 2017-03-09 19:01:29]

qyearsley@chromium.org changed reviewers:
+ qyearsley@chromium.org

[qyearsley, 2017-03-09 19:01:30]

https://codereview.chromium.org/2729613007/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp
(right):

https://codereview.chromium.org/2729613007/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp:171:
EXPECT_EQ(1u, observedResults.size());
It looks like this test is flaky:

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType...

Example output:

../../third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp:171:
Failure
Value of: observedResults.size()
  Actual: 0
Expected: 1u
Which is: 1

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit...

So observedResults might sometimes be empty and sometimes not, so it looks like
the lambda expression on line 160 is sometimes not invoked in time? Do you know
how this could be made un-flaky?