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.

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.

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?

This may be a certification requirement.

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.

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+

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.

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

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.

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.

Thanks David! Looking forward for the feedback!

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

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.

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 :)

Thanks a lot :etienne. Landing the PR: https://github.com/mozilla-b2g/gaia/commit/c62ca974fcb7fd5d54efe79740efd04119d81503

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

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

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

Assigning to Steven for verification/distribution.

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