Anshul Reporter
	Description • 10 years ago

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

	howie [:howie]
	Comment 1 • 10 years ago

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

	Star Cheng [:scheng]
	Comment 2 • 10 years ago

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.

	Star Cheng [:scheng]
	Comment 3 • 10 years ago

(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().

	howie [:howie]
	Comment 4 • 10 years ago

ni Ivan for triage Comment 2, thanks!

	Anshul Reporter
	Comment 5 • 10 years ago

(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?

	Star Cheng [:scheng]
	Comment 6 • 10 years ago

(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?

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 7 • 10 years ago

(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.

	Ivan Tsay (:ITsay)
	Comment 8 • 10 years ago

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.

	Star Cheng [:scheng]
	Comment 9 • 10 years ago

(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.

	Star Cheng [:scheng]
	Comment 10 • 10 years ago

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.

	Anshul Reporter
	Comment 11 • 10 years ago

(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.

	Etienne Segonzac (:etienne) Assignee
	Comment 14 • 10 years ago

(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.

	Etienne Segonzac (:etienne) Assignee
	Comment 15 • 10 years ago

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

	Star Cheng [:scheng]
	Comment 16 • 10 years ago

(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.

	Star Cheng [:scheng]
	Comment 17 • 10 years ago

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


It looks good to me.

	Etienne Segonzac (:etienne) Assignee
	Comment 18 • 10 years ago

(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?

	Etienne Segonzac (:etienne) Assignee
	Comment 19 • 10 years ago

see question in Comment 18

	Anshul Reporter
	Comment 20 • 10 years ago

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

	bhavana bajaj [:bajaj]
	Comment 21 • 10 years ago

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.

	Anshul Reporter
	Comment 22 • 10 years ago

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.

	Star Cheng [:scheng]
	Comment 23 • 10 years ago

(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.

	Eric Chou [:ericchou] [:echou]
	Comment 24 • 10 years ago

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.

	Etienne Segonzac (:etienne) Assignee
	Comment 25 • 10 years ago

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

	Etienne Segonzac (:etienne) Assignee
	Comment 26 • 10 years ago

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

	Etienne Segonzac (:etienne) Assignee
	Comment 27 • 10 years ago

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

	Eric Chou [:ericchou] [:echou]
	Comment 28 • 10 years ago

(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!

	Etienne Segonzac (:etienne) Assignee
	Comment 29 • 10 years ago

I know you love this code :)

	Gabriele Svelto [:gsvelto]
	Comment 30 • 10 years ago

Comment on attachment 8455369 [details]
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".

	Etienne Segonzac (:etienne) Assignee
	Comment 31 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

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

	Yeojin Chung [:YeojinC]
	Comment 33 • 10 years ago

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