Crash can occur if request for decode not completed while shutting down

RESOLVED FIXED in Firefox 37

Status

()

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: bobbyholley)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 unaffected, firefox37+ fixed, firefox38 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

4 years ago
Was playing with the YouTube test.

At some stage it crashed with Assertion failure: !mPromise, at ../../dist/include/MediaPromise.h:419

It seems that if a shutdown appear when a decoding MediaPromise hasn't completed, it will cause an assert.
(In reply to Jean-Yves Avenard [:jya] from comment #0)
> Was playing with the YouTube test.
> 
> At some stage it crashed with Assertion failure: !mPromise, at
> ../../dist/include/MediaPromise.h:419
> 
> It seems that if a shutdown appear when a decoding MediaPromise hasn't
> completed, it will cause an assert.

Which MediaPromiseHolder was asserting? Are you implying that it was MediaSourceReader::m{Audio,Video}Promise? Can you post a stack?
Flags: needinfo?(jyavenard)
(Reporter)

Comment 2

4 years ago
Posted file logcrash.txt.zip
Sorry forgot to attach the log
Flags: needinfo?(jyavenard)
(Reporter)

Updated

4 years ago
See Also: → bug 1130253

Updated

4 years ago
Priority: -- → P1
Looks like Chris introduced this bug in bug 1112822 - That patch does .Ensure() on a MediaPromiseHolder, but then Rejects the promise without informing the holder.

I think this is a bad footgun - I'm going to see if I can prevent this from compiling somehow.
Assignee: nobody → bobbyholley
Blocks: 1112822
We could also fix this by invoking .Reject() on the holder instead of on the raw
promise. But there's actually no reason to involve the holder at all here.
Attachment #8561822 - Flags: review?(matt.woodrow)
Attachment #8561822 - Flags: review?(cpearce)
This causes the buggy code that caused the crash to fail to compile.
Attachment #8561824 - Flags: review?(matt.woodrow)
Attachment #8561824 - Flags: feedback?(cpearce)
Attachment #8561822 - Flags: review?(matt.woodrow) → review+
Attachment #8561823 - Flags: review?(matt.woodrow) → review+
Attachment #8561824 - Flags: review?(matt.woodrow) → review+
Attachment #8561821 - Flags: review?(nfroyd) → review+
Builds everywhere but ICS. Let's see if we can fix that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd6ab713764
Comment on attachment 8561822 [details] [diff] [review]
Part 2 - Fix the crash. v1

In reality this is more of an f?
Attachment #8561822 - Flags: review?(cpearce) → feedback?(cpearce)
Attachment #8561823 - Flags: review?(cpearce)
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #9)
> Builds everywhere but ICS. Let's see if we can fix that:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd6ab713764

ICS build bustage fixed. Pushing to inbound.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d30eb154f8fa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e64148dcc6d7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a812298c5d98
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8dde5faef9
Attachment #8561822 - Flags: feedback?(cpearce) → feedback+
status-firefox36: --- → unaffected
status-firefox37: --- → affected
Comment on attachment 8561821 [details] [diff] [review]
Part 1 - Add a templatized move constructor for nsRefPtr. v1

Requesting 37 uplift of all patches on this bug.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes playing youtube video. Less consistent testing and harder to port later changes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This touches non-MSE code, but has been fine on nightly. Risk should be low at this point.
[String/UUID change made/needed]: None
Attachment #8561821 - Flags: approval-mozilla-aurora?
tracking-firefox37: --- → +
Comment on attachment 8561821 [details] [diff] [review]
Part 1 - Add a templatized move constructor for nsRefPtr. v1

MSE prereq. Aurora+
Attachment #8561821 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8561822 - Flags: approval-mozilla-aurora+
Attachment #8561823 - Flags: approval-mozilla-aurora+
Attachment #8561824 - Flags: approval-mozilla-aurora+
status-firefox37: affected → fixed
Attachment #8561824 - Flags: feedback?(cpearce)
You need to log in before you can comment on or make changes to this bug.