1145101 - crash in [@ mozilla::AVCCMediaDataDecoder::Shutdown() ]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(not currently active) Ted Mielczarek]

This bug was filed from the Socorro interface and is 
report bp-e838f7a2-5777-4d17-a2d4-972122150318.
=============================================================

Similar to bug 1143339 but apparently not the same. I had closed a YouTube tab ( http://www.youtube.com/watch?v=07K7LgwMrs4 ) and the content process crashed shortly afterwards.

Comment 1

[Jean-Yves Avenard [:jya]]

Step to reproduce it:

Select a stream >= 720 then another (still >= 720 so VDA is used). Repeat back and force a few time.

Close the tab.

Some VDA context corruption occurs. can't tell what it is at this stage.

Comment 2

[Jean-Yves Avenard [:jya]]

After adding extra logging:
"763654144[1347fa4a0]: Shutdown creating MediaPromise (11a326e80)
1967059712[1007421a0]: Unlinking VideoDecodeAcceleration framework.
763654144[1347fa4a0]: Shutdown resolving MediaPromise (11a326e80 created at Shutdown)
763654144[1347fa4a0]: ContinueShutdown invoking Then() [this=11a326e80, thenValue=1007a1100, aThisVal=131a49c40, isPending=0]
763654144[1347fa4a0]: Resolving Then() call made from ContinueShutdown [Runnable=1007d5610, Promise=11a326e80, ThenValue=1007a1100]
1967059712[1007421a0]: Unlinking VideoToolbox framework.
763654144[1347fa4a0]: MediaPromise::~MediaPromise [this=11a326e80]
1967059712[1007421a0]: Unlinking CoreMedia framework.
763654144[1347fa4a0]: ResolveRunnable::Run() [this=1007d5610]
763654144[1347fa4a0]: ContinueShutdown resolving MediaPromise (11a326da0 created at Shutdown)
763654144[1347fa4a0]: Resolving Then() call made from ContinueShutdown [Runnable=1007a8520, Promise=11a326da0, ThenValue=1007a11a0]
763654144[1347fa4a0]: MediaPromise::~MediaPromise [this=11a326da0]
763654144[1347fa4a0]: ResolveRunnable::Run() [this=1007a8520]
948948992[1347faa90]: AppleVDADecoder::ProcessShutdown: Thread=134b4fb60
948948992[1347faa90]: ProcessShutdown: cleaning up decoder 11a99be70
"

So we have the VideoDecodeAcceleration framework being unlinked when the last PDM is deleted with the last reader; however the SharedDecoderManager calls AppleVDADecoder::Shutdown() after that.
This cause the crash as the framework isn't linked anymore.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Correct log entry

Fix logging

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: _p2: Part2. Unset VDA/VT system functions when unlinking framework

When unlinking a framework, clear all function pointers. Make it much easier to identify when a framework is used after being unlinked

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Ensure PDM is shutdown after decoder

Ensure we don't delete the PDM that created the decoder used in SharedDecoderModule, before that decoder is shutdown

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Fix coding style

Fix coding style so its consistent with the rest of fmp4

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Re-use the same PDM when recreating a decoder

Reuse the original PDM when re-creating a decoder. This is necessary to ensure PDM are all and correctly shutdown

Comment 8

[Jean-Yves Avenard [:jya]]

In all, I'm surprised it ever worked even with windows, as wmf::MFShutdown would be called, yet WMF apis was used right after. cpearce, how could that have worked?

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Part6. Remove the PDM::Shutdown() method

Remove PlatformDecoderModule::Shutdown() method. It isn't required and only increase complexity as to when / when not shutdown.

Comment 10

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=732ab19ec5fa

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Re-use the same PDM when recreating a decoder

Reuse the original PDM when re-creating a decoder. This is necessary to ensure PDM are all and correctly shutdown.

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8581227 [details] [diff] [review]
Part5. Re-use the same PDM when recreating a decoder

need to tell the SharedDecoderManager's mPDM that HW acceleration is disabled

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: Part6. Remove the PDM::Shutdown() method

rebase

Comment 15

[Jean-Yves Avenard [:jya]]

Will need uplifting for 38

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8581224 [details] [diff] [review]
Part4. Fix coding style

Review of attachment 8581224 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/SharedDecoderManager.cpp
@@ +12,5 @@
>  class SharedDecoderCallback : public MediaDataDecoderCallback
>  {
>  public:
> +  explicit SharedDecoderCallback(SharedDecoderManager* aManager)
> +  : mManager(aManager) {}

Seeing as we're fixing coding style, this should be indented two more spaces...

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8581261 [details] [diff] [review]
Part6. Remove the PDM::Shutdown() method

Review of attachment 8581261 [details] [diff] [review]:
-----------------------------------------------------------------

I actually have a patch to do MFStartup/MFShutdown in the WMFMediaDataDecoder, was wondering if it would help the orange on Windows, didn't seem to...

Comment 18

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b00ac18b6a0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2c937ab69ae9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6fbef6e970cd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/746509b8d9a6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d91343508a39
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c20133f7f605

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b00ac18b6a0
https://hg.mozilla.org/mozilla-central/rev/2c937ab69ae9
https://hg.mozilla.org/mozilla-central/rev/6fbef6e970cd
https://hg.mozilla.org/mozilla-central/rev/746509b8d9a6
https://hg.mozilla.org/mozilla-central/rev/d91343508a39
https://hg.mozilla.org/mozilla-central/rev/c20133f7f605

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8581221 [details] [diff] [review]
Part1. Correct log entry

This is a blanket request for all changes

Approval Request Comment
[Feature/regressing bug #]: 1128381
[User impact if declined]: Crashes when closing a window/tab
[Describe test coverage new/current, TreeHerder]: I
[Risks and why]: I believe low, but it will always be less risky than crashing :)
[String/UUID change made/needed]: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2682ac013bce
https://hg.mozilla.org/releases/mozilla-aurora/rev/6d051b7b91b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/c06132cbaa8d
https://hg.mozilla.org/releases/mozilla-aurora/rev/eede1353e09f
https://hg.mozilla.org/releases/mozilla-aurora/rev/23386c5b2c67
https://hg.mozilla.org/releases/mozilla-aurora/rev/86a6c3272b09

Comment 23

[u279076]

I just hit this on Firefox 39.0a1 20150324030207 navigating back from a tab playing a Digg video. Should this have been resolved for the Nightly built on March 24?

Comment 24

[Jean-Yves Avenard [:jya]]

would you have a backtrace?

The backtrace would normally show something different if it's exactly the problem described here (as now it would be a null dereference.

Was the video you were navigating back to, paused ? or was it still playing?

Was it a DASH/MSE video?

Comment 25

[u279076]

(In reply to Jean-Yves Avenard [:jya] from comment #24)
> would you have a backtrace?

Unfortunately not and I haven't been able to reproduce the crash.

> Was the video you were navigating back to, paused ? or was it still playing?

I was not navigating back *to* a video, I was navigating back *from* a video. In other words, I had clicked a video on digg.com, watched the video to completion then went back to the digg.com main page using the Back button in my browser.

> Was it a DASH/MSE video?

Not that I know of.