getUserMedia errors not handled #1

Closed
stuartlangridge opened this Issue May 15, 2015 · 15 comments

Comments

Projects
None yet
2 participants

If a browser throws an error when opening a getUserMedia stream, recorder.js fires a streamError event. Sadly, the voice memos code doesn't listen to the event and so the user isn't warned that something went wrong.

@paullewis paullewis closed this in 0b0d011 May 18, 2015

Contributor

paullewis commented May 18, 2015

Would you mind checking this now?

Yep, that now pops up a dialog saying that it couldn't get access to the microphone. Two minor things: the first is that that didn't happen until I refreshed the site, despite getting a thing saying "app updated" before refresh (I know there are SW considerations here, but if "app updated" really means "it will be updated next time you load it, then the UI ought to be clearer about that, I think), and second, if one taps the record button one gets the "boooo!" dialog, but then if you clear the dialog and press the record button again, one does not get the dialog. If one cancels the recording and then hits + to start a new one and then hits the record button again, the dialog works again. I think it ought to show whenever hitting the record button, not just the first time for a new recording.

Contributor

paullewis commented May 18, 2015

Totally fair on both. I've just updated again (version 0.1.24, you can check at the bottom of the side nav if needed). You won't see the updated notification message for this update, because it'll use the old, cached version until it's replaced with this version (which is always confusing!). Meanwhile I now properly kill the stream and change recording status back to false, so hopefully this time you'll get the error every time you hit the button.

I appreciate you testing this, because I don't have any devices here that claim getUserMedia and Web Audio, but fail on creating the stream.

Contributor

paullewis commented May 18, 2015

For all my testing here, I can't replicate it at all. Here it's doing a full reset every time :(

OK. I think that the issue is this: when I hit record the first time, recorder.js throws the error, it gets caught and the dialog is shown. Then when I press it again, we call startRecording again, which checks if this.recording is true... and it still is true. I believe that the error handler needs to set recording to false. I'd point at actual code lines here, but reverse engineering from a compressed JS file to where those lines actually are in the uncompressed uncompiled untranspiled sources is really hard! (Hence http://kryogenix.org/days/2012/05/29/things-that-compile-to-javascript/ :))

@paullewis paullewis reopened this May 18, 2015

Contributor

paullewis commented May 18, 2015

Weirdly, I do close the stream and reset this.recording back to false:

https://github.com/GoogleChrome/voice-memos/blob/master/src/scripts/controller/RecordController.js#L277

As far as I can tell, that code isn't ending up in the actual served version: the devtools, after reformatting, show the code as

this.recorder.addEventListener("streamError", function() {
                                m["default"]().then(function(e) {
                                    var t = !0;
                                    return e.show("Booooo!", "There is a problem getting access to the microphone.", t)
                                })["catch"](function() {
                                })
                            })

Perhaps I still have a cached version? (Although I've hit refresh a few times.) Or the compilation isn't picking it up right?

Contributor

paullewis commented May 18, 2015

Aha, yeah so on the current live version (0.1.26) you should see:

y["default"]().then(function(e) {
  var t = !0;
  return e.show("Booooo!", "There is a problem getting access to the microphone.", t)
}).then(function() {
  e.killStream(), e.stopRecording()
})["catch"](function() {})

So it's mainly the following then you seem to be missing. Presume caching is getting in the way here.

Contributor

paullewis commented May 18, 2015

Again, I really appreciate you helping me out with this! You're very kind.

It seems to be. I have refreshed a bunch and I still have 0.1.24 according to the side menu. Can I force it somehow?

(note: it wouldn't surprise me if it not refreshing properly is some sort of bug in the Ubuntu browser's handling of serviceworker, but I don't know how to debug that because you can't inspect a service worker over a remote debugging connection.)

Contributor

paullewis commented May 18, 2015

At least as far as Chrome is concerned, it will only replace the current SW if there are no open connections to the current one. Which generally means closing all tabs / added-to-home-screen web apps and then restarting them. On desktop it's a ton easier because you can either Cmd+Shift+R which will load sans Service Worker (allowing the new one to take over), or you remote debug and do the same thing for Chrome on Android.

I dunno if it's a Chromium port you're running, but you could go to about:serviceworker-internals if it is, and then unregister it. In any case that feels a little extreme, but I do feel a little better that I might have actually fixed your bug, caching issues notwithstanding!

Right. Looking at the code on desktop, I see the changes. Killing the mobile browser (which is a wrapper around Oxide, which is basically Blink) and restarting it means I get 0.1.26 in the version number but the changed code still isn't there. But this is, at this point, clearly some sort of Ubuntu Oxide caching issue and if I could work out how to clear the cache (about: URLs don't work :( ) then I'm reasonably sure it'd be OK, so I think you're good. I'll try to get someone else to test it :-)

Contributor

paullewis commented May 18, 2015

Thanks! Right, I'll close it for real this time. If nothing else the fix actually helped out #4 anyway, so that's double good 🎉

@paullewis paullewis closed this May 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment