1157994 - Assertion failure: "Invalid AudioContextState transition" with suspend, close

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: (mAudioContextState == AudioContextState::Suspended && aNewState == AudioContextState::Running) || (mAudioContextState == AudioContextState::Running && aNewState == AudioContextState::Suspended) || (mAudioContextState == AudioContextState::Running && aNewState == AudioContextState::Closed) || (mAudioContextState == AudioContextState::Suspended && aNewState == AudioContextState::Closed) || (mAudioContextState == aNewState) (Invalid AudioContextState transition), at dom/media/webaudio/AudioContext.cpp:790

This assertion is part of code added in https://hg.mozilla.org/mozilla-central/rev/82bbdffc624b

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Paul Adenot (:padenot)]

Attachment: Ensure AudioContext operations are started and resolved in the same order. r=

This caused a transition from closed to suspended, which was caught by the
assert.

Comment 3

[Jesse Ruderman]

Comment on attachment 8597233 [details] [diff] [review]
Ensure AudioContext operations are started and resolved in the same order. r=

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

::: dom/media/GraphDriver.cpp
@@ +1083,5 @@
>    }
>  
> +  // We're going to iterate backward on the array, because we need to remove the
> +  // elements, but we also need to conserve the order of events, so we reverse
> +  // the array before iterating backward.

I hope there's a simpler way to "iterate & remove" than reverse-in-place followed by iterating backwards. Is there a consuming iterator you can use, perhaps after swapping out array?

@@ +1087,5 @@
> +  // the array before iterating backward.
> +  uint32_t length = array.Length();
> +  for (uint32_t i = 0; i < length / 2; i++) {
> +    std::swap(array[i], array[length - i - 1]);
> +  }

Do you feel like fixing bug 1061459? ;)

::: dom/media/test/crashtests/1157994.html
@@ +8,5 @@
> +    var ac = new AudioContext();
> +    setTimeout(function() {
> +        ac.suspend();
> +        ac.close();
> +    }, 100);

Do you know how the crashtest could be transformed into something that doesn't use setTimeout 100ms? Is there an event it could listen for instead? (I think I tried setTimeout 0 and it didn't assert.)

If we can't eliminate the setTimeout, the crashtest needs to do the reftest-wait dance like in https://dxr.mozilla.org/mozilla-central/source/docshell/base/crashtests/369126-1.html#1,7. (Otherwise the test can be unloaded before the timeout fires.)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8597233 [details] [diff] [review]
Ensure AudioContext operations are started and resolved in the same order. r=

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

::: dom/media/GraphDriver.cpp
@@ +1086,5 @@
> +  // elements, but we also need to conserve the order of events, so we reverse
> +  // the array before iterating backward.
> +  uint32_t length = array.Length();
> +  for (uint32_t i = 0; i < length / 2; i++) {
> +    std::swap(array[i], array[length - i - 1]);

How about just iterating forward and explicitly subtracting 1 from the index when we remove an element? That seems simplest.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Ensure AudioContext operations are started and resolved in the same order. r=

This is simpler, I'm not sure why I did it that way the first time.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9693ebe7f5

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a9693ebe7f5