1319251 - Intermittent dom/media/test/test_media_sniffer.html | application timed out after 330 seconds with no output after "Assertion failure: mPromise, at MozPromise.h:814"

Status: RESOLVED FIXED

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

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=6968297&repo=autoland

https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-win64-debug/1479756060/autoland_win8_64-debug_test-mochitest-media-e10s-bm112-tests1-windows-build2.txt.gz

New Win8 e10s failure that was probably lurking in the background since they were only enabled on trunk today.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

JW, do you have cycles to look at this?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Related to bug 1319190 maybe?

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: check-init-promise

If we call Shutdown() before Init completes, then we will Reject the promise from shutdown, and it'll be dead when Init completes.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: check-decoder-init

Same as above, but when the race affects the parent side. We clear mDecoder when we get Shutdown called, and then the Init promise returns and derefs it.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8813007 [details] [diff] [review]
check-decoder-init

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

seeing that we're attempting to fix a race, then the member itself should be made thread-safe.

will have to use another flag, atomic

::: dom/media/ipc/VideoDecoderParent.cpp
@@ +101,5 @@
>  {
>    RefPtr<VideoDecoderParent> self = this;
>    mDecoder->Init()->Then(mManagerTaskQueue, __func__,
>      [self] (TrackInfo::TrackType aTrack) {
> +      if (self->mDecoder) {

this seems racy to me...

mDecoder is accessed over multiple thread with no monitor in place

@@ +108,5 @@
>          Unused << self->SendInitComplete(hardwareAccelerated, hardwareReason);
>        }
>      },
>      [self] (MediaResult aReason) {
>        if (!self->mDestroyed) {

what about the case where initialisation failed?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8813007 [details] [diff] [review]
check-decoder-init

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

::: dom/media/ipc/VideoDecoderParent.cpp
@@ +101,5 @@
>  {
>    RefPtr<VideoDecoderParent> self = this;
>    mDecoder->Init()->Then(mManagerTaskQueue, __func__,
>      [self] (TrackInfo::TrackType aTrack) {
> +      if (self->mDecoder) {

It shouldn't be? It should only ever be accessed from the manager thread (mManagerTaskQueue).

It's racy in terms of async operations happening in an order that we didn't expect (Init, Shutdown, InitComplete), not in terms of multiple threads racing on a data access.

@@ +108,5 @@
>          Unused << self->SendInitComplete(hardwareAccelerated, hardwareReason);
>        }
>      },
>      [self] (MediaResult aReason) {
>        if (!self->mDestroyed) {

This case doesn't touch mDecoder, but I can make them consistent.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: assert-parent-thread

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8813019 [details] [diff] [review]
assert-parent-thread

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

thank you for this...

The names however are utter confusing...
To assert we're on the mManagerTaskQueue you assert OnReaderTaskQueue.

Gotta admit at a glance, that it's impossible to tell they are the same!

Can you rename either the taskqueue or the method checking that we're on it?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/718387d11e93
Check Init promise in VideoDecoderChild before trying to resolve it. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/431fe7720cb5
Ensure that our decoder pointer hasn't be revoked when we get the Init callback. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/88f8e3bee8c1
Add more current thread assertions in VideoDecoderParent so it's more obvious about which code runs where. r=jya

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/718387d11e93
https://hg.mozilla.org/mozilla-central/rev/431fe7720cb5
https://hg.mozilla.org/mozilla-central/rev/88f8e3bee8c1

Comment 11

[Intermittent Failures Robot]

14 failures in 623 pushes (0.022 failures/push) were associated with this bug in the last 7 days.  

Repository breakdown:
* autoland: 13
* mozilla-inbound: 1

Platform breakdown:
* windows8-64: 14

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1319251&startday=2016-11-21&endday=2016-11-27&tree=all

Comment 12

[Intermittent Failures Robot]

1 failures in 824 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* autoland: 1

Platform breakdown:
* windows7-32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1319251&startday=2017-10-02&endday=2017-10-08&tree=all