Remove WebMediaPlayerDelegate null checks

Remove WebMediaPlayerDelegate null checks

As per the bug description and xhwang@, we are currently storing the
|delegate_| in WMPI as a weak pointer and null-checking it everywhere.

From the bug (and verified with dcheng@ as still being true):
HTMLMediaElement is a Blink::SuspendableObject, which will have its
contextDestroyed() called when Blink::Document::shutdown() is called,
which then destroys WMPI. Document::shutdown() is called before a frame
is detached, and the RenderFrameImpl will outlive HTMLMediaElement.
Since |delegate_| is owned by the RFI, it is safe to pass |delegate_|
as a raw pointer an not null check it.

This CL removes the null-checks and stores |delegate_| as a raw pointer
instead.

BUG=593932
TEST= Ran media UTs on Android and desktop. Ran content UTs on desktop.

Review-Url: https://codereview.chromium.org/2640573002
Cr-Commit-Position: refs/heads/master@{#445487}
Committed: https://chromium.googlesource.com/chromium/src/+/1bb1c78bef7565222274dd05ba49cfbbed1978ec

[tguilbert, 2017-01-17 22:12:22]

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

[commit-bot: I haz the power, 2017-01-17 22:13:12]

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

[tguilbert, 2017-01-17 22:16:42]

tguilbert@chromium.org changed reviewers:
+ clamy@chromium.org, sandersd@chromium.org

[tguilbert, 2017-01-17 22:16:42]

clamy@, please OWNER review the one line change in content/*
sandersd@, please review changes in media/*

Thank you! :)

[sandersd (OOO until July 31), 2017-01-17 22:57:30]

lgtm

[DaleCurtis, 2017-01-18 00:02:38]

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

[DaleCurtis, 2017-01-18 00:02:39]

Remove from WMPA, WMPMS too?

[DaleCurtis, 2017-01-18 00:03:13]

Also I feel like I tried this and it didn't work for some reason, but all your
bots are green and I don't remember :|

[commit-bot: I haz the power, 2017-01-18 00:21:40]

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

[commit-bot: I haz the power, 2017-01-18 00:21:41]

Dry run: This issue passed the CQ dry run.

[watk, 2017-01-18 01:12:11]

I just searched for ActiveDOMObject but it's not a thing? Maybe it got renamed.

[DaleCurtis, 2017-01-18 01:14:35]

https://codereview.chromium.org/2567913002/

[tguilbert, 2017-01-18 02:13:09]

Description was changed from

==========
Remove WebMediaPlayerDelegate null checks

As per the bug description and xhwang@, we are currently storing the
|delegate_| in WMPI as a weak pointer and null-checking it everywhere.

The WebMediaPlayerDelegate is owned by RenderFrameImpl, and
HMTLMediaElement is an ActiveDOMObject. ActiveDOMObject::stop() is
guaranteed to be called before a frame is detached, so it's impossible
for WMPI to outlive the frame and dereference a destructed delegate.

This CL removes the null-checks and stores |delegate_| as a raw pointer
instead.

BUG=593932
TEST= Ran media UTs on Android and desktop.
==========

to

==========
Remove WebMediaPlayerDelegate null checks

As per the bug description and xhwang@, we are currently storing the
|delegate_| in WMPI as a weak pointer and null-checking it everywhere.

From the bug:
"The WebMediaPlayerDelegate is owned by RenderFrameImpl, and
HMTLMediaElement is an ActiveDOMObject. ActiveDOMObject::Stop() should
be called before a frame is detached, so WMPI should not outlive the
frame and dereference a destructed delegate."
However, since the bug was originally opened, ActiveDOMObject was
renamed to SuspendableObject and Stop() was removed in favor of calling
contextDestroyed() directly. The lifetime guarantees should still hold,
and WMPI should not outlive its delegate.

This CL removes the null-checks and stores |delegate_| as a raw pointer
instead.

BUG=593932
TEST= Ran media UTs on Android and desktop. Ran content UTs on desktop.
==========

[tguilbert, 2017-01-18 02:14:36]

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

[commit-bot: I haz the power, 2017-01-18 02:14:56]

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-01-18 04:16:13]

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

[commit-bot: I haz the power, 2017-01-18 04:16:14]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-01-18 13:56:16]

Thanks! Could you explain a bit more the relationship between the various
objects involved. I'm not familiar with this code, so it's hard for me to assess
whether this is safe or not.

https://codereview.chromium.org/2640573002/diff/20001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/2640573002/diff/20001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:280:
media::WebMediaPlayerDelegate* delegate_;
Could you add a comment describing why this is safe to store as a naked pointer?
This is far from obvious and it would help readers. Thanks!

Same in other header files.

[tguilbert, 2017-01-19 02:20:35]

Description was changed from

==========
Remove WebMediaPlayerDelegate null checks

As per the bug description and xhwang@, we are currently storing the
|delegate_| in WMPI as a weak pointer and null-checking it everywhere.

From the bug:
"The WebMediaPlayerDelegate is owned by RenderFrameImpl, and
HMTLMediaElement is an ActiveDOMObject. ActiveDOMObject::Stop() should
be called before a frame is detached, so WMPI should not outlive the
frame and dereference a destructed delegate."
However, since the bug was originally opened, ActiveDOMObject was
renamed to SuspendableObject and Stop() was removed in favor of calling
contextDestroyed() directly. The lifetime guarantees should still hold,
and WMPI should not outlive its delegate.

This CL removes the null-checks and stores |delegate_| as a raw pointer
instead.

BUG=593932
TEST= Ran media UTs on Android and desktop. Ran content UTs on desktop.
==========

to

==========
Remove WebMediaPlayerDelegate null checks

As per the bug description and xhwang@, we are currently storing the
|delegate_| in WMPI as a weak pointer and null-checking it everywhere.

From the bug (and verified with dcheng@ as still being true):
HTMLMediaElement is a Blink::SuspendableObject, which will have its
contextDestroyed() called when Blink::Document::shutdown() is called,
which then destroys WMPI. Document::shutdown() is called before a frame
is detached, and the RenderFrameImpl will outlive HTMLMediaElement.
Since |delegate_| is owned by the RFI, it is safe to pass |delegate_|
as a raw pointer an not null check it.

This CL removes the null-checks and stores |delegate_| as a raw pointer
instead.

BUG=593932
TEST= Ran media UTs on Android and desktop. Ran content UTs on desktop.
==========

[tguilbert, 2017-01-19 02:31:05]

Patchset #3 (id:40001) has been deleted

[tguilbert, 2017-01-19 02:37:08]

Patchset #3 (id:60001) has been deleted

[tguilbert, 2017-01-19 02:39:12]

I added comments about the lifetime guarantees, thanks to dcheng@'s
explanations.

https://codereview.chromium.org/2640573002/diff/20001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/2640573002/diff/20001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:280:
media::WebMediaPlayerDelegate* delegate_;
On 2017/01/18 13:56:16, clamy wrote:
> Could you add a comment describing why this is safe to store as a naked
pointer?
> This is far from obvious and it would help readers. Thanks!
> 
> Same in other header files.

Done.

[tguilbert, 2017-01-19 22:02:43]

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

[commit-bot: I haz the power, 2017-01-19 22:03:18]

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

[DaleCurtis, 2017-01-19 22:36:10]

lgtm \o/ fingers crossed. I feel like blink bots exploded locally when I tried
this.

[commit-bot: I haz the power, 2017-01-19 23:26:57]

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

[commit-bot: I haz the power, 2017-01-19 23:26:58]

Dry run: This issue passed the CQ dry run.

[tguilbert, 2017-01-20 23:01:42]

Friendly ping :)

[clamy, 2017-01-23 13:52:41]

Thanks! Lgtm.

[tguilbert, 2017-01-23 19:37:47]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2017-01-23 19:37:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2640573002/#ps100001
(title: "Undo accidental delete of WMPI::delegate_")

[commit-bot: I haz the power, 2017-01-23 19:38:14]

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-01-23 21:15:16]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485200267888880,
"parent_rev": "1470b1f47385ebd166e6ca60a34a770c0f4c40f6", "commit_rev":
"1bb1c78bef7565222274dd05ba49cfbbed1978ec"}

[commit-bot: I haz the power, 2017-01-23 21:15:50]

Description was changed from

==========
Remove WebMediaPlayerDelegate null checks

As per the bug description and xhwang@, we are currently storing the
|delegate_| in WMPI as a weak pointer and null-checking it everywhere.

From the bug (and verified with dcheng@ as still being true):
HTMLMediaElement is a Blink::SuspendableObject, which will have its
contextDestroyed() called when Blink::Document::shutdown() is called,
which then destroys WMPI. Document::shutdown() is called before a frame
is detached, and the RenderFrameImpl will outlive HTMLMediaElement.
Since |delegate_| is owned by the RFI, it is safe to pass |delegate_|
as a raw pointer an not null check it.

This CL removes the null-checks and stores |delegate_| as a raw pointer
instead.

BUG=593932
TEST= Ran media UTs on Android and desktop. Ran content UTs on desktop.
==========

to

==========
Remove WebMediaPlayerDelegate null checks

As per the bug description and xhwang@, we are currently storing the
|delegate_| in WMPI as a weak pointer and null-checking it everywhere.

From the bug (and verified with dcheng@ as still being true):
HTMLMediaElement is a Blink::SuspendableObject, which will have its
contextDestroyed() called when Blink::Document::shutdown() is called,
which then destroys WMPI. Document::shutdown() is called before a frame
is detached, and the RenderFrameImpl will outlive HTMLMediaElement.
Since |delegate_| is owned by the RFI, it is safe to pass |delegate_|
as a raw pointer an not null check it.

This CL removes the null-checks and stores |delegate_| as a raw pointer
instead.

BUG=593932
TEST= Ran media UTs on Android and desktop. Ran content UTs on desktop.

Review-Url: https://codereview.chromium.org/2640573002
Cr-Commit-Position: refs/heads/master@{#445487}
Committed:
https://chromium.googlesource.com/chromium/src/+/1bb1c78bef7565222274dd05ba49...
==========

[commit-bot: I haz the power, 2017-01-23 21:15:52]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1bb1c78bef7565222274dd05ba49...