Closed Bug 1145101 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

All
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: ted, Assigned: jya)

References

Details

(Keywords: crash)

Crash Data

Attachments

(6 files, 2 obsolete files)

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.
Summary: crash in mozilla::AVCCMediaDataDecoder::Shutdown() → crash in [@ mozilla::AVCCMediaDataDecoder::Shutdown() ]
Depends on: 1145779
No longer depends on: 1145779
Assignee: nobody → jyavenard
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.
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.
Fix logging
Attachment #8581221 - Flags: review?(cpearce)
When unlinking a framework, clear all function pointers. Make it much easier to identify when a framework is used after being unlinked
Attachment #8581222 - Flags: review?(cpearce)
Ensure we don't delete the PDM that created the decoder used in SharedDecoderModule, before that decoder is shutdown
Attachment #8581223 - Flags: review?(cpearce)
Fix coding style so its consistent with the rest of fmp4
Attachment #8581224 - Flags: review?(cpearce)
Reuse the original PDM when re-creating a decoder. This is necessary to ensure PDM are all and correctly shutdown
Attachment #8581227 - Flags: review?(matt.woodrow)
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?
Flags: needinfo?(cpearce)
Remove PlatformDecoderModule::Shutdown() method. It isn't required and only increase complexity as to when / when not shutdown.
Attachment #8581252 - Flags: review?(cpearce)
Duplicate of this bug: 1145779
Reuse the original PDM when re-creating a decoder. This is necessary to ensure PDM are all and correctly shutdown.
Attachment #8581258 - Flags: review?(matt.woodrow)
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
Attachment #8581227 - Attachment is obsolete: true
Attachment #8581227 - Flags: review?(matt.woodrow)
rebase
Attachment #8581261 - Flags: review?(cpearce)
Attachment #8581252 - Attachment is obsolete: true
Attachment #8581252 - Flags: review?(cpearce)
Crash Signature: [@ mozilla::AVCCMediaDataDecoder::Shutdown()] → [@ mozilla::AVCCMediaDataDecoder::Shutdown()] [@ CFRelease | __CFBasicHashDrain ]
Will need uplifting for 38
Attachment #8581221 - Flags: review?(cpearce) → review+
Attachment #8581222 - Flags: review?(cpearce) → review+
Attachment #8581223 - Flags: review?(cpearce) → review+
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...
Attachment #8581224 - Flags: review?(cpearce) → review+
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...
Attachment #8581261 - Flags: review?(cpearce) → review+
Attachment #8581258 - Flags: review?(matt.woodrow) → review+
Duplicate of this bug: 1146337
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
Attachment #8581221 - Flags: approval-mozilla-aurora?
Attachment #8581221 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8581222 - Flags: approval-mozilla-aurora+
Attachment #8581223 - Flags: approval-mozilla-aurora+
Attachment #8581224 - Flags: approval-mozilla-aurora+
Attachment #8581258 - Flags: approval-mozilla-aurora+
Attachment #8581261 - Flags: approval-mozilla-aurora+
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?
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?
Flags: needinfo?(anthony.s.hughes)
(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.
Flags: needinfo?(anthony.s.hughes)
You need to log in before you can comment on or make changes to this bug.