Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

getUserMedia errors not handled #1

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

getUserMedia errors not handled #1

stuartlangridge opened this issue May 15, 2015 · 15 comments

Comments

@stuartlangridge
Copy link

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
Copy link
Contributor

Would you mind checking this now?

@stuartlangridge
Copy link
Author

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.

@paullewis
Copy link
Contributor

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.

@stuartlangridge
Copy link
Author

stuartlangridge commented May 18, 2015 via email

@paullewis
Copy link
Contributor

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

@stuartlangridge
Copy link
Author

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
@paullewis
Copy link
Contributor

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

@stuartlangridge
Copy link
Author

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?

@paullewis
Copy link
Contributor

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.

@paullewis
Copy link
Contributor

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

@stuartlangridge
Copy link
Author

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?

@stuartlangridge
Copy link
Author

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

@paullewis
Copy link
Contributor

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!

@stuartlangridge
Copy link
Author

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

@paullewis
Copy link
Contributor

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 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants