1029979 - JavascriptException: TypeError: Argument 1 of AudioContext.constructor 'null'

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Anshul]

We are often seeing an exception "JavascriptException: TypeError: Argument 1 of AudioContext.constructor 'null' is not a valid value for enumeration AudioChannel" while running voice call related marionette tests.

I don't have a definitive STRs but in general below is what a test script is doing.

1. Place an outgoing call
2. Place an incoming call
3. Answer the incoming call
4. Hangup all the calls

Comment 1

[howie [:howie]]

Assign to Star, he'll back in office in 6/30

Comment 2

[Star Cheng [:scheng]]

If the Argument of AudioContext() is null, it will be set 'normal' audio channel. It's checked in Gecko layer. So it doesn't cause system damage. So I don't think this bug should be set as blocked.

Comment 3

[Star Cheng [:scheng]]

(In reply to Anshul from comment #0)
> We are often seeing an exception "JavascriptException: TypeError: Argument 1
> of AudioContext.constructor 'null' is not a valid value for enumeration
> AudioChannel" while running voice call related marionette tests.
> 
> I don't have a definitive STRs but in general below is what a test script is
> doing.
> 
> 1. Place an outgoing call
> 2. Place an incoming call
> 3. Answer the incoming call
> 4. Hangup all the calls


Hi, 

Could you help to try to figure out the failure marionette tests. It is easy to check the Argument of AudioContext().

Comment 4

[howie [:howie]]

ni Ivan for triage Comment 2, thanks!

Comment 5

[Anshul]

(In reply to Star Cheng [:scheng] from comment #3)
> (In reply to Anshul from comment #0)
> > We are often seeing an exception "JavascriptException: TypeError: Argument 1
> > of AudioContext.constructor 'null' is not a valid value for enumeration
> > AudioChannel" while running voice call related marionette tests.
> > 
> > I don't have a definitive STRs but in general below is what a test script is
> > doing.
> > 
> > 1. Place an outgoing call
> > 2. Place an incoming call
> > 3. Answer the incoming call
> > 4. Hangup all the calls
> 
> 
> Hi, 
> 
> Could you help to try to figure out the failure marionette tests. It is easy
> to check the Argument of AudioContext().

Could you please be more specific on what you want me to do?

Comment 6

[Star Cheng [:scheng]]

(In reply to Anshul from comment #5)
> (In reply to Star Cheng [:scheng] from comment #3)
> > (In reply to Anshul from comment #0)
> > > We are often seeing an exception "JavascriptException: TypeError: Argument 1
> > > of AudioContext.constructor 'null' is not a valid value for enumeration
> > > AudioChannel" while running voice call related marionette tests.
> > > 
> > > I don't have a definitive STRs but in general below is what a test script is
> > > doing.
> > > 
> > > 1. Place an outgoing call
> > > 2. Place an incoming call
> > > 3. Answer the incoming call
> > > 4. Hangup all the calls
> > 
> > 
> > Hi, 
> > 
> > Could you help to try to figure out the failure marionette tests. It is easy
> > to check the Argument of AudioContext().
> 
> Could you please be more specific on what you want me to do?

Hi, Hsin-Yi

Do you know any marionette test about the STR?

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Star Cheng [:scheng] from comment #6)
> (In reply to Anshul from comment #5)
> > (In reply to Star Cheng [:scheng] from comment #3)
> > > (In reply to Anshul from comment #0)
> > > > We are often seeing an exception "JavascriptException: TypeError: Argument 1
> > > > of AudioContext.constructor 'null' is not a valid value for enumeration
> > > > AudioChannel" while running voice call related marionette tests.
> > > > 
> > > > I don't have a definitive STRs but in general below is what a test script is
> > > > doing.
> > > > 
> > > > 1. Place an outgoing call
> > > > 2. Place an incoming call
> > > > 3. Answer the incoming call
> > > > 4. Hangup all the calls
> > > 
> > > 
> > > Hi, 
> > > 
> > > Could you help to try to figure out the failure marionette tests. It is easy
> > > to check the Argument of AudioContext().
> > 
> > Could you please be more specific on what you want me to do?
> 
> Hi, Hsin-Yi
> 
> Do you know any marionette test about the STR?

Sorry I have no idea about Star's comment.

I also looked at log captured during gecko telephony marionette tests (not sure if they are what Anshul talked about) but didn't see this error.

Comment 8

[Ivan Tsay (:ITsay)]

I also don't think this is the blocker unless the exception causes the crash of the phone. It looks to me that the exception is handled in gecko as the response in comment 2 by Star. For further analysis, we also need a clear STR or the log for analysis. Backlog it for now.

Also ni? Star to clarify what he needs from our partner.

Comment 9

[Star Cheng [:scheng]]

(In reply to Ivan Tsay (:ITsay) from comment #8)
> I also don't think this is the blocker unless the exception causes the crash
> of the phone. It looks to me that the exception is handled in gecko as the
> response in comment 2 by Star. For further analysis, we also need a clear
> STR or the log for analysis. Backlog it for now.
> 
> Also ni? Star to clarify what he needs from our partner.

I am trying to find the marionette test which probably cause "JavascriptException: TypeError" in marionette folder. But I can't find it. If there is the marionette test which cause the "TypeError", it should be easy to analysis.

Comment 10

[Star Cheng [:scheng]]

Hi, Etienne

Could you help me to clarify the following question: 

In tone_player.js file, if we use .trashAudio() and then use .ensureAudio() in certain code flow. Does it cause to new AudioContext() constructor as "null"? Or this code flow is impossible.

Comment 11

[Anshul]

(In reply to Ivan Tsay (:ITsay) from comment #8)
> I also don't think this is the blocker unless the exception causes the crash
> of the phone. It looks to me that the exception is handled in gecko as the
> response in comment 2 by Star. For further analysis, we also need a clear
> STR or the log for analysis. Backlog it for now.
STRs are clearly mentioned in comment #0. This crash doesn't happen every time so running it once or twice might not be enough. We are seeing it 1 out of 10 times in our nightly run. Please find the logs attached. 


> Also ni? Star to clarify what he needs from our partner.

Comment 12

[Anshul]

Attachment: audio context exception

Comment 13

[Anshul]

Attachment: audio context exception

Comment 14

[Etienne Segonzac (:etienne)]

(In reply to Star Cheng [:scheng] from comment #10)
> Hi, Etienne
> 
> Could you help me to clarify the following question: 
> 
> In tone_player.js file, if we use .trashAudio() and then use .ensureAudio()
> in certain code flow. Does it cause to new AudioContext() constructor as
> "null"? Or this code flow is impossible.

I think it's possible :/
I'll wip up a patch so we can confirm this.

Comment 15

[Etienne Segonzac (:etienne)]

Attachment: 0001-Bug-1029979-WIP-fix-bad-audio-context-constructor.patch

Here's a wip, with no tests yet.
But can we confirm this fixes the issue? (without causing new ones ;))

Comment 16

[Star Cheng [:scheng]]

(In reply to Etienne Segonzac (:etienne) from comment #15)
> Created attachment 8450095 [details] [diff] [review]
> 0001-Bug-1029979-WIP-fix-bad-audio-context-constructor.patch
> 
> Here's a wip, with no tests yet.
> But can we confirm this fixes the issue? (without causing new ones ;))

Look good to me. 
Maybe we can push the patch to TPBL and re-trigger it more than 10 times to verify. If there is no marionette error, it should get better.

Comment 17

[Star Cheng [:scheng]]

Comment on attachment 8450095 [details] [diff] [review]
0001-Bug-1029979-WIP-fix-bad-audio-context-constructor.patch


It looks good to me.

Comment 18

[Etienne Segonzac (:etienne)]

(In reply to Star Cheng [:scheng] from comment #16)
> Look good to me. 
> Maybe we can push the patch to TPBL and re-trigger it more than 10 times to
> verify. If there is no marionette error, it should get better.

Yes we definitely should!
Which suite should we run?

Comment 19

[Etienne Segonzac (:etienne)]

see question in Comment 18

Comment 20

[Anshul]

Sure, I will try the patch on Monday and get back to you guys.

Comment 21

[bhavana bajaj [:bajaj]]

Changing the blocking flag, given this is crash is possible to hit after trying the STR and something we should fix if we can. Looks like Etienne has a patch for anshul already.

Comment 22

[Anshul]

Comment on attachment 8450095 [details] [diff] [review]
0001-Bug-1029979-WIP-fix-bad-audio-context-constructor.patch

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

This patch doesn't seem to fix the issue. I am still seeing the exact same issue. I did confirm that the fix was properly pushed to the device.

Comment 23

[Star Cheng [:scheng]]

(In reply to Etienne Segonzac (:etienne) from comment #18)
> (In reply to Star Cheng [:scheng] from comment #16)
> > Look good to me. 
> > Maybe we can push the patch to TPBL and re-trigger it more than 10 times to
> > verify. If there is no marionette error, it should get better.
> 
> Yes we definitely should!
> Which suite should we run?

Maybe we can use this try chooser option for marionette test. "try: -b o -p emulator -u marionette-webapi -t none" and re-trigger 10 times to check if there is any error.

Comment 24

[Eric Chou [:ericchou] [:echou]]

Hi Etienne,

According to comment 22, the patch didn't seem to work. Could you investigate more to see if there is any other possible causes? Star can help with answering Gecko questions so please feel free to ask.

Thanks.

Comment 25

[Etienne Segonzac (:etienne)]

Testing a new patch, it's on try
https://tbpl.mozilla.org/?tree=Try&rev=3e75098f3c7b

Comment 26

[Etienne Segonzac (:etienne)]

The first run went well, I've triggered 15 more.

Comment 27

[Etienne Segonzac (:etienne)]

The try looks good, looks like the only errors are known intermittents.
I'll send a patch for review today,

Comment 28

[Eric Chou [:ericchou] [:echou]]

(In reply to Etienne Segonzac (:etienne) from comment #27)
> The try looks good, looks like the only errors are known intermittents.
> I'll send a patch for review today,

Thanks , Etienne!

Comment 29

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

I know you love this code :)

Comment 30

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8455369 [details] [review]
Gaia PR

Awww, it actually took me a while to figure out what was going on. This LGTM but I'd like a better commit comment to explain why this change was made. Something like "prevent successive trash/ensureAudio() calls from creating an AudioContext with a null channel".

Comment 31

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/f31f95ec18882decc43c0e2f02df581580a32dc8

Comment 32

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/2c6c413ed729d465c52d6c2d5d458e2eee79e956

Comment 33

[Yeojin Chung [:YeojinC]]

The bug involves a Marionette JavaScript test. Unable to create test case