Bug Fix: WebMediaPlayerMS Crashes

Bug Fix: WebMediaPlayerMS Crashes

WebMediaPlayerMS owns WebMediaPlayerMSCompositor, but they live on two
different threads, and then there is a chance that when WebMediaPlayerMS
is destructed on the main thread, there are still tasks regarding
WebMediaPlayerMSCompositor on the compositor thread, and finally crash
will be observed.

Pass weak pointer to the PostTask call, then when the object is
destroyed, the task will be voided.

BUG=572398
Committed: https://crrev.com/09aac57cf764382bc667203bf4afc46b821fe2fc
Cr-Commit-Position: refs/heads/master@{#368472}

[qiangchen, 2016-01-06 17:56:07]

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

[qiangchen, 2016-01-06 17:56:08]

Should be a quick fix.

[DaleCurtis, 2016-01-06 23:13:13]

We typically don't want to add more ref counted classes.  Is it sufficient just
to give the compositor a WeakPtrFactory for its methods? That way destruction
will invalidate those without the need for any ref counts.

[qiangchen, 2016-01-07 20:18:10]

Done.

[DaleCurtis, 2016-01-07 21:05:46]

https://codereview.chromium.org/1565893002/diff/40001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/1565893002/diff/40001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.h:53:
base::SupportsWeakPtr<WebMediaPlayerMSCompositor>) {
Instead of using SupportsWeakPtr, can you just add a WeakFactory member? That's
the preferred pattern for small use cases like this. It prevents external
classes from taking a weakptr unnecessarily.

[qiangchen, 2016-01-07 22:04:03]

Description was changed from

==========
Bug Fix: WebMediaPlayerMS Crashes

WebMediaPlayerMS owns WebMediaPlayerMSCompositor, but they live on two
different threads, and then there is a chance that when WebMediaPlayerMS
is destructed on the main thread, there are still tasks regarding
WebMediaPlayerMSCompositor on the compositor thread, and finally crash
will be observed.

Change WMPMSCompositor to be RefCounted, and thus its life time will be
extended if pending tasks exist.

BUG=572398
==========

to

==========
Bug Fix: WebMediaPlayerMS Crashes

WebMediaPlayerMS owns WebMediaPlayerMSCompositor, but they live on two
different threads, and then there is a chance that when WebMediaPlayerMS
is destructed on the main thread, there are still tasks regarding
WebMediaPlayerMSCompositor on the compositor thread, and finally crash
will be observed.

Pass weak pointer to the PostTask call, then when the object is
destroyed, the task will be voided.

BUG=572398
==========

[qiangchen, 2016-01-08 17:10:12]

Done.

https://codereview.chromium.org/1565893002/diff/40001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/1565893002/diff/40001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.h:53:
base::SupportsWeakPtr<WebMediaPlayerMSCompositor>) {
On 2016/01/07 21:05:46, DaleCurtis wrote:
> Instead of using SupportsWeakPtr, can you just add a WeakFactory member?
That's
> the preferred pattern for small use cases like this. It prevents external
> classes from taking a weakptr unnecessarily.

Done.

[DaleCurtis, 2016-01-08 18:56:17]

lgtm

https://codereview.chromium.org/1565893002/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/1565893002/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.h:153: };
Can you add a DISALLOW_COPY_AND_ASSIGN(...) while you're here? Adding a comment
to the weak factory saying it must always be the last member variable is a good
idea too.

[qiangchen, 2016-01-08 22:00:11]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-01-08 22:00:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1565893002/#ps80001
(title: "Disallow Copy")

[commit-bot: I haz the power, 2016-01-08 22:03:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1565893002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1565893002/80001

[qiangchen, 2016-01-08 22:09:39]

https://codereview.chromium.org/1565893002/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/1565893002/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_compositor.h:153: };
On 2016/01/08 18:56:17, DaleCurtis wrote:
> Can you add a DISALLOW_COPY_AND_ASSIGN(...) while you're here? Adding a
comment
> to the weak factory saying it must always be the last member variable is a
good
> idea too.

Done.

[commit-bot: I haz the power, 2016-01-09 00:37:04]

Description was changed from

==========
Bug Fix: WebMediaPlayerMS Crashes

WebMediaPlayerMS owns WebMediaPlayerMSCompositor, but they live on two
different threads, and then there is a chance that when WebMediaPlayerMS
is destructed on the main thread, there are still tasks regarding
WebMediaPlayerMSCompositor on the compositor thread, and finally crash
will be observed.

Pass weak pointer to the PostTask call, then when the object is
destroyed, the task will be voided.

BUG=572398
==========

to

==========
Bug Fix: WebMediaPlayerMS Crashes

WebMediaPlayerMS owns WebMediaPlayerMSCompositor, but they live on two
different threads, and then there is a chance that when WebMediaPlayerMS
is destructed on the main thread, there are still tasks regarding
WebMediaPlayerMSCompositor on the compositor thread, and finally crash
will be observed.

Pass weak pointer to the PostTask call, then when the object is
destroyed, the task will be voided.

BUG=572398
==========

[commit-bot: I haz the power, 2016-01-09 00:37:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-09 00:39:16]

Description was changed from

==========
Bug Fix: WebMediaPlayerMS Crashes

WebMediaPlayerMS owns WebMediaPlayerMSCompositor, but they live on two
different threads, and then there is a chance that when WebMediaPlayerMS
is destructed on the main thread, there are still tasks regarding
WebMediaPlayerMSCompositor on the compositor thread, and finally crash
will be observed.

Pass weak pointer to the PostTask call, then when the object is
destroyed, the task will be voided.

BUG=572398
==========

to

==========
Bug Fix: WebMediaPlayerMS Crashes

WebMediaPlayerMS owns WebMediaPlayerMSCompositor, but they live on two
different threads, and then there is a chance that when WebMediaPlayerMS
is destructed on the main thread, there are still tasks regarding
WebMediaPlayerMSCompositor on the compositor thread, and finally crash
will be observed.

Pass weak pointer to the PostTask call, then when the object is
destroyed, the task will be voided.

BUG=572398

Committed: https://crrev.com/09aac57cf764382bc667203bf4afc46b821fe2fc
Cr-Commit-Position: refs/heads/master@{#368472}
==========

[commit-bot: I haz the power, 2016-01-09 00:39:17]

Patchset 5 (id:??) landed as
https://crrev.com/09aac57cf764382bc667203bf4afc46b821fe2fc
Cr-Commit-Position: refs/heads/master@{#368472}