1244951 - "Assertion failure: false (This should not happen, setting the value should always work)" in mozilla::dom::AudioParam::SetValue

Status: RESOLVED WORKSFORME

(Core :: Web Audio, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: false (This should not happen, setting the value should always work), at dist/include/mozilla/dom/AudioParam.h:67

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[nadri sophia]

Hi ,
I am interested in working on this bug.  I am a computer science student and this will be the first time I have contributed to Firefox, so any help would be greatly appreciated.  Could I be assigned to the bug?

Thanks

Comment 3

[Dan Minor [:dminor]]

Feel free to ask questions here or in #media on IRC if you run into any problems. Thanks for having a look at this :)

Comment 4

[nadri sophia]

Hi, thanks for the quick reply :)
I started working on the bug but i have a little problem with the test attached.
I added the test in mochitest.ini but when launching the test i get an error that i don't really understand.
I tried asking question in #developers in IRC but they don't seem to understand it either.
ANY help will be appreciated.
Here is the error message :

######
### Now running mochitest-plain with subsuite media.
######

Checking for orphan ssltunnel processes...
Checking for orphan xpcshell processes...
SUITE-START | Running 1 tests
dir: dom/media/tests/mochitest
mozprofile.addons WARNING | Could not install /Users/bob/mozilla-central/obj-x86_64-apple-darwin15.4.0/_tests/testing/mochitest/extensions/mozscreenshots: [Errno 2] No such file or directory: '/Users/bob/mozilla-central/obj-x86_64-apple-darwin15.4.0/_tests/testing/mochitest/extensions/mozscreenshots/install.rdf'
pk12util: PKCS12 IMPORT SUCCESSFUL
MochitestServer : launching [u'/Users/bob/mozilla-central/obj-x86_64-apple-darwin15.4.0/dist/bin/xpcshell', '-g', u'/Users/bob/mozilla-central/obj-x86_64-apple-darwin15.4.0/dist/Nightly.app/Contents/Resources', '-v', '170', '-f', u'/Users/bob/mozilla-central/obj-x86_64-apple-darwin15.4.0/dist/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/var/folders/c3/33v_4k2x1gncdjh3xc2zx40w0000gn/T/tmpZEUt9W.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = true;", '-f', '/Users/bob/mozilla-central/obj-x86_64-apple-darwin15.4.0/_tests/testing/mochitest/server.js']
runtests.py | Server pid: 90423
runtests.py | Websocket server pid: 90424
runtests.py | websocket/process bridge pid: 90425
Traceback (most recent call last):
  File "websocketprocessbridge/websocketprocessbridge.py", line 6, in <module>
    from twisted.internet import protocol, reactor
ImportError: No module named twisted.internet
0 ERROR runtests.py | websocket/process bridge failed to launch. Are all the dependencies installed?
runtests.py | SSL tunnel pid: 90426
failed to bind socket on port 4443: error -5982
Shutting down...
runtests.py | Running with e10s: True
runtests.py | Running tests: start.

runtests.py | Application pid: 90428
TEST-INFO | started process Main app process
1463733090638	Marionette	INFO	Listening on port 2828
1 INFO SimpleTest START
2 INFO TEST-START | dom/media/tests/mochitest/test_setValueAudioParam_AssertionFailed.html

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Right now, due to the problem of installing dependencies on developer machines, 3 tests in the media sub-suite will typically fail when run locally (twisted, etc failures).  You can ignore these, and any errors in setting up the environment they need.

How are you running the test?  ./mach mochitest dom/media/tests/mochitest/test_setValueAudioParam_AssertionFailed.html I assume.  Does the test itself pass?  (I see TEST-START but nothing after that.)

Note that since this is a webaudio test it should go in dom/media/webaudio/test; dom/media/tests/mochitest is where getUserMedia and WebRTC tests live.

Comment 6

[ts.sajot]

Attachment: bug-1244951-fix.patch

Hi, I am currently working with Sophia on this project. however we have two ways of solving the problem. In this patch, I changed the error handling of the SetValue. I also added a test (test_audioParamSetValue.html) but I am not sure it does check for the right thing.

Comment 7

[nadri sophia]

Attachment: bug-1244951-fix-bis.patch

Hi,
You will find attached my patch for this bug.
I tried to add a validation test for SetValue to respect the spec when it sais,that If value is set during a time when there are any automation events scheduled then it will be ignored and no exception will be thrown.

Waiting for your reviews.
Thanks

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8756830 [details] [diff] [review]
bug-1244951-fix.patch

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

::: dom/media/webaudio/AudioParam.h
@@ +70,4 @@
>        return;
>      }
>  
>      AudioParamTimeline::SetValue(aValue);

This is not what the spec says this should do:

> If value is set during a time when there are any automation events scheduled
> then it will be ignored and no exception will be thrown.
> 
> The effect of setting this attribute is equivalent to calling setValueAtTime()
> with the current AudioContext's currentTime and the requested value. Subsequent
> accesses to this attribute's getter will return the same value. 

This means that we need to check whether the timeline is empty or not. If it is, then those two lines should be equivalent:

> gain.gain.value = 0.0;
> gain.gain.setValueAtTime(0.0, audiocontext.currentTime);

i.e. we should call SetValueAtTime from SetValue.

If it is not, we should simply return without throwing an exception, with nothing changed.

::: dom/media/webaudio/test/test_audioParamSetValue.html
@@ +29,5 @@
> +        gain.gain.value = 2;
> +        source.connect(gain);
> +
> +        source.start(0);
> +        return gain;

This should test the two cases:

- The timeline is empty, the call is equivalent to calling `setValueAtTime(someValue, context.currentTime);`. Getting the value should return `someValue`.
- The timeline is not empty (because another automation method have been called before), the call should be ignored.

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8756851 [details] [diff] [review]
bug-1244951-fix-bis.patch

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

You can use Jesse's testcase as the basis for a mochitest, it's often what we do.

::: dom/media/webaudio/AudioEventTimeline.h
@@ +192,5 @@
> +          return false;
> +          }
> +
> +      }
> +    }

The prose in the spec means that if any automation methods have been called, setting the value should be ignored, so this check is incorrect.

::: dom/media/webaudio/AudioParam.h
@@ +71,4 @@
>        return;
>      }
>  
>      AudioParamTimeline::SetValue(aValue);

This should be converted to a SetValueAtTime with aValue and CurrentTime() as parameters.

Comment 10

[nadri sophia]

Attachment: bug-1244951-fix.patch

Hi ,
We modified the patch according to the comments concerning the webAPI (https://github.com/WebAudio/web-audio-api/issues/828#issuecomment-222182923).
We tried to add a validation condition that tests that the time line is empty.
we're able to set a value after a setTargetAtTime though .
I hope this will do it this time.
Thanks

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8757811 [details] [diff] [review]
bug-1244951-fix.patch

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

In light of https://github.com/WebAudio/web-audio-api/issues/828#issuecomment-222457578, we should do the following:

::: dom/media/webaudio/AudioEventTimeline.h
@@ +185,5 @@
> +    // when there are any automation events
> +    // scheduled then it will be ignored and no exception will be thrown.
> +    if (aEvent.mType == AudioTimelineEvent::SetValue && !mEvents.IsEmpty()) {
> +      for (unsigned i = 0; i < mEvents.Length(); i++) {
> +        if(mEvents[i].mType != AudioTimelineEvent::SetTarget){

rtoy was referencing a bug in Chrome when he was talking about SetTargetAtTime, we should not implement it.

I think the right check is simply to replace `AudioTimelineEvent::SetTarget` by `AudioTimelineEvent::SetValue`: it's okay to set the value again if all the events are `SetValue` events, which means only the setter was ever used, otherwise, the call should be ignored.

::: dom/media/webaudio/AudioParam.h
@@ -63,5 @@
>    void SetValue(float aValue, ErrorResult& aRv)
>    {
>      AudioTimelineEvent event(AudioTimelineEvent::SetValue, 0.0f, aValue);
>      if(!ValidateEvent(event, aRv)){
> -       aRv. SuppressException();

Is this based off the previous patch ? I'd rather see the method `ValidateEvent` modified.

@@ +67,3 @@
>        return;
>      }
>      AudioParamTimeline::SetValue(aValue);

We should modify SetValue so that it sends an event that contains the `currentTime`, that is exactly the same as `SetValueAtTime`, but has a different type so we can check for it in `ValidateEvent`.

::: dom/media/webaudio/test/test_bug1244951.html
@@ +40,5 @@
> +            return expectedBuffer;
> +        },
> +    };
> +
> +    runTest();

It would be good to have a test that tests that we can set the value directly multiple time, and that it can be the start value for a ramp, and also that it should be ignored when automation events are present:

gain.gain.value = 2.0;
gain.gain.value = 3.0; //check that the gain is effectively 3.0 by processing some audio with it and checking the values

gain.gain.value = 2.0;
gain.gain.linearRampToValueAtTime(3.0, ac.currentTime + 3.0); // check that the ramp is from 2.0 to 3.0

gain.gain.setValueAtTime(2.0, ac.currentTime);
gain.gain.value = 3.0; // check that the gain effectively applied is still 2.0.

Comment 12

[nadri sophia]

Attachment: bug-1244951-fix.patch

Comment 13

[nadri sophia]

Attachment: bug-1244951-fix.patch

Hi,
I tried to test all the situations you talked about in your last review and i think it works just fine.
however i'm not sure about something , in rtoy's comment about the API (https://github.com/WebAudio/web-audio-api/issues/828#issuecomment-222182923)
he said :"Well, except for the goofy behavior that if you call the setter when setTargetAtTime is running, the value does take affect causing setTargetAtTime to start ramping with a new starting value from the setter.

This particular behavior will change once the value setter becomes equivalent to setValueAtTime."
Does it mean that ow it shouldn't start ramping with the value from the setter?(if yes than i think that my test should work)

Comment 14

[Paul Adenot (:padenot)]

(In reply to nadri sophia from comment #13)
> Created attachment 8759563 [details] [diff] [review]
> bug-1244951-fix.patch
> 
> Hi,
> I tried to test all the situations you talked about in your last review and
> i think it works just fine.
> however i'm not sure about something , in rtoy's comment about the API
> (https://github.com/WebAudio/web-audio-api/issues/828#issuecomment-222182923)
> he said :"Well, except for the goofy behavior that if you call the setter
> when setTargetAtTime is running, the value does take affect causing
> setTargetAtTime to start ramping with a new starting value from the setter.
> 
> This particular behavior will change once the value setter becomes
> equivalent to setValueAtTime."
> Does it mean that ow it shouldn't start ramping with the value from the
> setter?(if yes than i think that my test should work)

Setting the value when ramping should now be ignored, yes.

Comment 15

[Paul Adenot (:padenot)]

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

Comment 16

[Paul Adenot (:padenot)]

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

Comment 17

[nadri sophia]

Attachment: bug-1244951.Patch

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8760819 [details] [diff] [review]
bug-1244951.Patch

Canceling the review right now, I'll pick this up sometime, hopefully soon.

Comment 19

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Trunk is still affected. Paul, are you still working on this?

Comment 21

[Paul Adenot (:padenot)]

I don't see myself having time to work on this soon, sorry.

Comment 22

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 23

[Tyson Smith [:tsmith]]

The fuzzers are still seeing this issue and the attached test case is still reliable. I see there is patch so a Pernosco session would likely not be useful however if you would like one please let me know.

Comment 24

[Tyson Smith [:tsmith]]

This issue is no longer reproducible using the attached test case and was last found by fuzzers targeting m-c 20210504-3aa883d4b622.