WebMediaPlayer implementations should never null-check their WebMediaPlayerDelegate* |
||
Issue descriptionPer xhwang@: https://chromiumcodereview.appspot.com/1766783003/diff/40001/media/blink/webmediaplayer_impl.cc#newcode1068 One comment about the |delegate_|: We are storing the |delegate_| as a weak pointer and checking it everywhere we use it. This is probably not necessary. The delegate is kinda owned by RenderFrameImpl [1]. And HTMLMediaElement 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. You can see a similar discussion at [2]. If that makes sense, we can do some clean up around the |delegate_|: 1. Pass and store |delegate_| as a raw pointer. 2. Remove all the if(delegate_) check in WMP implementations. 3. (optional) Drop the RenderFrameObserver base class for RendererWebMediaPlayerDelegate and store |media_player_delegate_| as a normal scoped_ptr member in RenderFrameImpl. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_frame_impl.h&l=1192 [2] https://codereview.chromium.org/1464183002/#msg42
,
Nov 17 2016
sandersd@ is working on a lot of WebMediaPlayerDelegate changes. I will wait until his changes land before taking this on.
,
Jan 19 2017
I spoke with dcheng@ to confirm that the assumptions still hold true. Using the renamed interface/method names: HTMLMediaElement is a Blink::SuspendableObject, which will have its contextDestroyed() called when Blink::Document::shutdown() is called. shutdown() is called before a frame is detached, and the frame will out live the HTMLMediaElement. It is safe to pass |delegate_| as a raw pointer an not null check it.
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1bb1c78bef7565222274dd05ba49cfbbed1978ec commit 1bb1c78bef7565222274dd05ba49cfbbed1978ec Author: tguilbert <tguilbert@chromium.org> Date: Mon Jan 23 21:15:11 2017 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} [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/content/renderer/media/android/webmediaplayer_android.cc [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/content/renderer/media/android/webmediaplayer_android.h [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/content/renderer/media/webmediaplayer_ms.cc [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/content/renderer/media/webmediaplayer_ms.h [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/content/renderer/media/webmediaplayer_ms_unittest.cc [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/content/renderer/render_frame_impl.cc [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/1bb1c78bef7565222274dd05ba49cfbbed1978ec/media/blink/webmediaplayer_impl_unittest.cc
,
Feb 7 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by dalecur...@chromium.org
, Jul 22 2016