Closed Bug 1319251 Opened 3 years ago Closed 3 years ago

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"

Categories

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

52 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- disabled
firefox53 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: mattwoodrow)

References

Details

(Keywords: assertion, intermittent-failure, regression)

Attachments

(3 files)

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.
JW, do you have cycles to look at this?
Flags: needinfo?(jwwang)
Blocks: 1315141
Keywords: regression
Flags: needinfo?(jwwang)
Related to bug 1319190 maybe?
Flags: needinfo?(matt.woodrow)
If we call Shutdown() before Init completes, then we will Reject the promise from shutdown, and it'll be dead when Init completes.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Attachment #8813006 - Flags: review?(jyavenard)
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.
Attachment #8813007 - Flags: review?(jyavenard)
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?
Attachment #8813007 - Flags: review?(jyavenard) → review-
Attachment #8813006 - Flags: review?(jyavenard) → review+
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.
Attachment #8813019 - Flags: review?(jyavenard)
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?
Attachment #8813019 - Flags: review?(jyavenard) → review+
Attachment #8813007 - Flags: review- → review+
Pushed by mwoodrow@mozilla.com:
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
Version: unspecified → 52 Branch
You need to log in before you can comment on or make changes to this bug.