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

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Bobby Holley (:bholley)]

(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?

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: logcrash.txt.zip

Sorry forgot to attach the log

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add a templatized move constructor for nsRefPtr. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Fix the crash. v1

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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix one other remaining case of invoking Resolve/Reject directly on a Promise. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Separate the creator and consumer APIs for MediaPromise. v1

This causes the buggy code that caused the crash to fail to compile.

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=642976c04c24

Comment 9

[Bobby Holley (:bholley)]

Builds everywhere but ICS. Let's see if we can fix that:

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

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8561822 [details] [diff] [review]
Part 2 - Fix the crash. v1

In reality this is more of an f?

Comment 11

[Bobby Holley (:bholley)]

(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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d30eb154f8fa
https://hg.mozilla.org/mozilla-central/rev/e64148dcc6d7
https://hg.mozilla.org/mozilla-central/rev/a812298c5d98
https://hg.mozilla.org/mozilla-central/rev/2d8dde5faef9

Comment 13

[Ralph Giles (:rillian)]

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

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8561821 [details] [diff] [review]
Part 1 - Add a templatized move constructor for nsRefPtr. v1

MSE prereq. Aurora+