Closed Bug 1129877 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 --- fixed

People

(Reporter: jya, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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)
Attached file logcrash.txt.zip
Sorry forgot to attach the log
Flags: needinfo?(jyavenard)
See Also: → 1130253
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+
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?
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+
Attachment #8561824 - Flags: feedback?(cpearce)
You need to log in before you can comment on or make changes to this bug.