820099 - if screen blanks while playing long tone we get errors in the console and keep playing the tone forever over the air

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect, P1)

Description

[David Flanagan [:djf]]

While on a call, if I press and hold a dialer key for a long time (maybe 5 seconds) I eventually start seeing this message in the console:

[JavaScript Error: "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable" {file: "app://communications.gaiamobile.org/dialer/js/keypad.js" line: 108}]

I think the audio buffer is overflowing because every 60ms we're feeding it 150ms of audio data

I think I have written code before that feeds the buffer only the amount of data that it has room for, so feel free to assign this bug to me.

Comment 1

[David Flanagan [:djf]]

This a result of the recently landed 815439, which added support for long tones.

Nominating it as blocking because it is easier to fix this than worry about the unknown consequences of overfilling the audio buffer.

Comment 2

[Ben Francis [:benfrancis]]

Triage: We probably wouldn't block the release on this unless there was a clear user impact. Is anything broken from the user's point of view? Are there other consequences of overfilling the audio buffer?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This may be a certification requirement.

Comment 4

[Dylan Oliver [:doliver]]

Triage:

Supporting the long tones is definitely a certification requirement and we need to leave that in place, so the real question here is whether we want to speculatively fix the buffer overflow. 

We are not inclined to block this without understanding if there is any real impact here. 

Leaving this nominated for further comment and qawanted to see if anyone can find user impact of this one.

Comment 5

[Beatriz Rodríguez [:brg]]

This is very important for certification and also user point of view.
DTMF tones are used to send passwords in bank systems. We should fix this issue to make DTMF tones work fine.
Please mark this issue as BB+

Comment 6

[David Flanagan [:djf]]

I mis-diagosed this bug.  There is no buffer overflow in the audio system.

What is happening may actually be a little trickier...  Now that we can play long tones we run into the issue of screen blanking while a long tone is being played.  Press and hold a key and then put the phone up to your ear (or put your hand over the light sensor).

The screen goes to sleep, and the dialer has code to turn off audio when the screen blanks. So our tone stops playing (though it is still being sent over the air) and we start getting errors in the console because we've destroyed the audio object but are still trying to put audio data into it.

So I suppose I should stop the screen from blanking if the user is playing a tone, or I should properly end the tone when the screen blanks.  The latter seems like the right thing to do: I can't imagine a product requirement that the user be able to play long tones while holding the phone to her ear.

My Android phone doesn't play long tones at all, so I can't compare.

Comment 7

[David Flanagan [:djf]]

So, this is an event handler issue in apps/communication/dialer/js/keypad.js.

Francisco is already working on event handling bugs in that file for #820002, so I'm going to assign this bug to him. If we both work on these concurrently we'll just end up overwriting each other's work.

Francisco: if you don't want to work on this yourself, you can assign it back to me once you're done with 820002.  Or you can assign both to me.

Note that one obvious fix for this bug is to call this.stop() in the visiblityChange handler.  That prevents the error messages in the console.  But that is not enough because we're still transmitting the DMTF tone over the air, and we don't get a mouseup event after the screen blanks, so we can't turn it off on that event, either.  So we've got to turn off both the local tone and the broadcast tone when the screen blanks.  I've just done a bit of testing but it looks like maybe we get a mouseup event after the screen comes back on, and that is causing an error message to the console because it generates another call to stop() when there is no _audio object.

I'm going to edit the summary line to properly describe this bug

Comment 8

[Francisco Jordano [:arcturus] [:francisco]]

I couldn't reproduce exactly the problem you comment.

What I saw in the current code is:

[JavaScript Error: "TypeError: this._audio is undefined" {file: "app://communications.gaiamobile.org/dialer/js/keypad.js" line: 108}]

Using the audio element after it's being removed.

Anyway I totally see the point of handling better when we stop the sound. In this case we will need to call the telephony API to stop the dtfm.

Thanks, will try to add a patch ASAP for you to review.

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to GitHub PR 7043

Comment 10

[David Flanagan [:djf]]

Francisco,

I haven't reviewed the patch yet, but will get to it today. The fact that you couldn't reproduce the long tones makes me think that maybe I was seeing that because I'd put a try/catch block around the error on line 108, and was then seeing the long tones on screen blanking because I had put that in there...

I'll review soon.

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

Thanks David!

Looking forward for the feedback!

Comment 12

[David Flanagan [:djf]]

Comment on attachment 692951 [details]
Pointer to GitHub PR 7043

The patch stops sending the tone over the air when I test it.  I'm not familiar enough with the mozTelephony API to know if it is okay to call stopTone() when no tone is playing, but I don't see any console errors when it gets called with no tone playing, so I'm going to assume that is okay.

If you press and hold a key to send a long tone, and then blank the screen by shading the light sensor, the long tone shuts off.  However, if you then release the key and unshade the light sensor so the screen comes back, there is an error on line 119.  (that's after applying the patch from the other bug, too.).  It looks like stop() is being called again when the screen comes back, and in this case this._audio is null, and there is an error on this._audio.src = ''.  It seems benign, but worth fixing with an if (this._audio) 

Finally (and this is probably unrelated, but I want to bring it up) I see this error in the console when I shade the light sensor to blank the screen:

E/GeckoConsole(  415): [JavaScript Warning: "HTTP "Content-Type" of "text/html" is not supported. Load of media resource app://communications.gaiamobile.org/dialer/oncall.html failed." {file: "app://communications.gaiamobile.org/dialer/oncall.html#unlocked" line: 0}]

Is that a known error?  Is the app hardcoding http:// somewhere instead of using location.protocol?

r+ if you fix the error on line 119

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Just fixed the problem in line 119, being sure we don't have a null.

Regarding the warning, I've been reading and apparently is something related to reading a music file without the proper content header.

Which annoys me a bit, I though we were generating the tones.

I'll land this, but will ask to :etienne for more insights about the warning.

Comment 14

[Etienne Segonzac (:etienne)]

E/GeckoConsole(  415): [JavaScript Warning: "HTTP "Content-Type" of "text/html" is not supported. Load of media resource app://communications.gaiamobile.org/dialer/oncall.html failed." {file: "app://communications.gaiamobile.org/dialer/oncall.html#unlocked" line: 0}]

is driving me crazy too. But I've heard from a credible source that:
* setting src to '' is the only way to release everything properly
* removing this warning on the gecko side was discussed at some point but rejected

So we'll have to live with it :)

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

Thanks a lot :etienne.

Landing the PR:

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

Comment 16

[David Flanagan [:djf]]

(In reply to Etienne Segonzac (:etienne) from comment #14)
> E/GeckoConsole(  415): [JavaScript Warning: "HTTP "Content-Type" of
> "text/html" is not supported. Load of media resource
> app://communications.gaiamobile.org/dialer/oncall.html failed." {file:
> "app://communications.gaiamobile.org/dialer/oncall.html#unlocked" line: 0}]
> 
> is driving me crazy too. But I've heard from a credible source that:
> * setting src to '' is the only way to release everything properly
> * removing this warning on the gecko side was discussed at some point but
> rejected

Etienne: are you saying that that message is caused by the this._audio.src = '' line?  Wow.

I've gotten rid of similar errors for the video alement with video.deleteAttribute('src'); video.load(). I've gotten confirmation from media hackers that (for video at least) doing that releases all the resources.

> So we'll have to live with it :)

Maybe we can do it the way I did it for video.

Comment 17

[Etienne Segonzac (:etienne)]

(In reply to David Flanagan [:djf] from comment #16)
> Etienne: are you saying that that message is caused by the this._audio.src =
> '' line?  Wow.
> 

Yep.

> I've gotten rid of similar errors for the video alement with
> video.deleteAttribute('src'); video.load(). I've gotten confirmation from
> media hackers that (for video at least) doing that releases all the
> resources.
> 
> > So we'll have to live with it :)
> 
> Maybe we can do it the way I did it for video.

I'll ask the audio guy :) Would be really cool to get rid of it.

Comment 18

[Paul Adenot (:padenot)]

djf, Etienne, I've checked again to make sure.

For a given <video> |v| (or <audio>, keep in mind it is mostly the same code underneath, in content/html/content/src/nsHTMLMediaElement.cpp, and content/media/*), to release resources (threads, memory) on Gecko, you can do:

> v.removeAttribute("src");
> v.load();

This will release the media decoder and its thread, the audio stream (if any) and its thread, the state machine (and its thread if there is no other media element currently living), and various other things related to the media code you don't care about.

There won't be a warning in the console if you do this. The spec is actually mentioning our exact use case, even though the paragraph is noted as being non-normative [1]:

> Playing audio and video resources on small devices such as set-top boxes or
> mobile phones is often constrained by limited hardware resources in the device.
> For example, a device might only support three simultaneous videos. For this
> reason, it is a good practice to release resources held by media elements when
> they are done playing, either by being very careful about removing all
> references to the element and allowing it to be garbage collected, or, even
> better, by removing the element's src attribute and any source element
> descendants, and invoking the element's load() method.

[1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#best-practices-for-authors-using-media-elements

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Assigning to Steven for verification/distribution.

Comment 20

[Tony Chung [:tchung]]

Verified fix in build 20130113070202.  no more long tone playing forever.