1419897 - Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-56cc84a6-a8f0-4f6f-9155-807730171119.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2zcCXIS. Fairly low volume and started using 20171117100127.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f41930a869a84af81df1a88d8e82323ff3a6509a&tochange=daa0dcd1616cbdf5541f548e44662d20d6d67d99.

Bug 1369548 landed in that timeframe. ni on :kikuo

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::HTMLMediaElement::RemoveMediaKeys dom/html/HTMLMediaElement.cpp:7058
1 xul.dll <lambda_ef52c37b58668ddc2fe0b7e52a91603e>::operator dom/html/HTMLMediaElement.cpp:7081
2 xul.dll mozilla::MozPromise<bool, mozilla::MediaResult, 1>::ThenValue<<lambda_ef52c37b58668ddc2fe0b7e52a91603e>, <lambda_7035f5609cf451ad4fe135b769b9531c> >::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:765
3 xul.dll mozilla::MozPromise<RefPtr<mozilla::VideoData>, mozilla::MediaResult, 1>::ThenValueBase::DoResolveOrReject xpcom/threads/MozPromise.h:497
4 xul.dll mozilla::MozPromise<mozilla::media::TimeUnit, mozilla::MediaResult, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:402
5 xul.dll mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:214
6 xul.dll mozilla::EventTargetWrapper::Runner::Run xpcom/threads/AbstractThread.cpp:150
7 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:396
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1037
9 xul.dll nsThread::Shutdown xpcom/threads/nsThread.cpp:800

=============================================================

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

An access violation happens when the code runs to "mMediaKeys->Unbind();" [1] in an runnable.
But the most weird thing is, |TryRemoveMediaKeysAssociation()| will only be called when mMediaKeys is not null. [2]
In the meantime, all following calls to HTMLME::SetMediaKeys() will be rejected [3] if the prior operations are not finished.

Hi JW,
Any idea what may cause this crash ? 
I not sure whether the ignorance of argument here [4] for the SetCDMPromise may affect as I referred to the code here [5].

[1] https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/HTMLMediaElement.cpp#l7058
[2] https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/HTMLMediaElement.cpp#l7120
[3] https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/HTMLMediaElement.cpp#l7252
[4] https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/HTMLMediaElement.cpp#l7078
[5] https://searchfox.org/mozilla-central/source/dom/media/MediaDecoder.h#86-88

Comment 2

[James Cheng[:JamesCheng]]

(In reply to Kilik Kuo [:kikuo] from comment #1)
> An access violation happens when the code runs to "mMediaKeys->Unbind();"
> [1] in an runnable.
> But the most weird thing is, |TryRemoveMediaKeysAssociation()| will only be
> called when mMediaKeys is not null. [2]
> In the meantime, all following calls to HTMLME::SetMediaKeys() will be
> rejected [3] if the prior operations are not finished.
> 
> Hi JW,
> Any idea what may cause this crash ? 
> I not sure whether the ignorance of argument here [4] for the SetCDMPromise
> may affect as I referred to the code here [5].
> 
> [1]
> https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/
> HTMLMediaElement.cpp#l7058
> [2]
> https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/
> HTMLMediaElement.cpp#l7120
> [3]
> https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/
> HTMLMediaElement.cpp#l7252
> [4]
> https://hg.mozilla.org/mozilla-central/annotate/a77c628829b3/dom/html/
> HTMLMediaElement.cpp#l7078
> [5]
> https://searchfox.org/mozilla-central/source/dom/media/MediaDecoder.h#86-88

Here is a point [1] that may set mMediaKeys to nullptr (maybe the function is running in non-main-thread, so you meet a race condition).

You should make 

HTMLMediaElement::RemoveMediaKeys() thread safe with Mutex guarded.

And change the [1] using HTMLMediaElement::RemoveMediaKeys() instead.

[1]
https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/dom/html/HTMLMediaElement.cpp#6620

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

(In reply to James Cheng[:JamesCheng] from comment #2)
> Here is a point [1] that may set mMediaKeys to nullptr (maybe the function
> is running in non-main-thread, so you meet a race condition).
> 
> You should make 
> 
> HTMLMediaElement::RemoveMediaKeys() thread safe with Mutex guarded.
> 
> And change the [1] using HTMLMediaElement::RemoveMediaKeys() instead.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 7a8c667bdd2a4a32746c9862356e199627c0896d/dom/html/HTMLMediaElement.cpp#6620
 
Ah, thanks for pointing it out. I just missed that !

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

All HTMLMediaElement functions should run on the main thread. That's why you don't see any assertions about the main thread.

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1419897 - Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys.

The process of |TryRemoveMediaKeysAssociation()| is a 2-step async procedue in mainthread.
mMediaKeys might be set to null inside |NotifyOwnerDocumentActivityChanged()| in between
|TryRemoveMediaKeysAssociation| and |RemoveMediaKeys|.

Review commit: https://reviewboard.mozilla.org/r/203538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203538/

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8932492 [details]
Bug 1419897 - Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys.

https://reviewboard.mozilla.org/r/203538/#review209238

::: commit-message-f5f03:3
(Diff revision 1)
> +Bug 1419897 - Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys.
> +The process of |TryRemoveMediaKeysAssociation()| is a 2-step async procedue in mainthread.
> +mMediaKeys might be set to null inside |NotifyOwnerDocumentActivityChanged()| in between

It appears to me that we should just disconnect mSetCDMRequest if NotifyOwnerDocumentActivityChanged() happens in between. Thoughts?

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8932492 [details]
Bug 1419897 - Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys.

https://reviewboard.mozilla.org/r/203538/#review209254

::: commit-message-f5f03:3
(Diff revision 1)
> +Bug 1419897 - Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys.
> +The process of |TryRemoveMediaKeysAssociation()| is a 2-step async procedue in mainthread.
> +mMediaKeys might be set to null inside |NotifyOwnerDocumentActivityChanged()| in between

The process of HTMLMediaElement::SetMediaKeys() should be independent to the OwnerDocumentActivity.
If we disconnect the mSetCDMRequest when NotifyOwnerDocumentActivityChanged() happens, we still need to keep the HTMLME::SetMediaKeys process going so that those variables' status could reflect correctly.

See also the commit message [1] in Bug 1369548.

[1] https://reviewboard.mozilla.org/r/162962/diff/2#index_header

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8932492 [details]
Bug 1419897 - Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys.

https://reviewboard.mozilla.org/r/203538/#review209268

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a4a5661cd94
Crash in mozilla::dom::HTMLMediaElement::RemoveMediaKeys. r=jwwang

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9a4a5661cd94