1073410 - URL click-back calls disconnect 2 seconds after connecting when using Firefox (if link-clicker gUM is not accepted soon enough)

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Romain Testard [:RT]]

Steps to reproduce:
1 Generate URL on latest Nightly
2 Click URL on Firefox
3 Accept call with audio+video or audio only
4 Accept gUM on clicker side
5 The call tries to connect and terminates after 12 seconds of trying to connect 
6 Feedback form displayed on both ends

THIS DOES NOT HAPPEN WHEN THE LINK CLICKER USES CHROME.

Link clicker: Windows 7 / Firefox nightly
Link generator: Windows 8 / Firefox nightly

Browser console log (link generator):
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
Mandatory/optional in createOffer options is deprecated! Use {"mozDontOfferDataChannel":true} instead (note the case difference)!
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
addIceCandidate called without success/failure callbacks. This is deprecated, and will be an error in the future.
no element found ClientEvent:1
addIceCandidate called without success/failure callbacks. This is deprecated, and will be an error in the future.
addIceCandidate called without success/failure callbacks. This is deprecated, and will be an error in the future.
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
1501 "Session.subscribe :: InvalidStreamID" sdk.js:910
"OT.exception :: title: undefined (1501) msg: Session.subscribe :: InvalidStreamID" sdk.js:910
no element found ClientEvent:1
no element found ClientQos:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found


Web console log (link clicker side):
"Warning: enqueueUpdate(): Render methods should be a pure function of props and state; triggering nested component updates from render is not allowed. If necessary, trigger nested updates in componentDidUpdate." react-0.11.1.js:20242
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
Mandatory/optional in createOffer options is deprecated! Use {"mozDontOfferDataChannel":true} instead (note the case difference)!
no element found ClientEvent:1
Peer connection is closed sdk.js:15188
no element found ClientEvent:1
Error: Invariant Violation: replaceState(...): Can only update a mounted or mounting component. react-0.11.1.js:18697
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientQos:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found ClientEvent:1
no element found

Comment 1

[Mark Banner (:standard8)]

I've spoken to RT about this over irc. The main problem here seems to be the speed at which the gUM prompt is accepted on the link-clicker side.

If the user takes more than a few seconds, and then accepts the gUM, the stream set-up time for the call can easily exceed the server's connection timer of 10 seconds.

This is even worse if the user doesn't accept for that length of time.

I know there was some plans for camera preview - so if we didn't actually start the call "dialing" until the preview is accepted (just show the connecting window), that might fix the issue.

We'd also have to be careful for audio calls, that we make sure to get the microphone before starting.

Adam, is this what was intended? If not, do we need some other form of allowing the link clicker to take a long time over accepting the gUM prompt?

Also, if the preview request gUM, and then the Tokbox SDK requests it, are we going to get two gUM prompts?

Comment 2

[Romain Testard [:RT]]

(In reply to Mark Banner (:standard8) from comment #1)
> I've spoken to RT about this over irc. The main problem here seems to be the
> speed at which the gUM prompt is accepted on the link-clicker side.
> 
> If the user takes more than a few seconds, and then accepts the gUM, the
> stream set-up time for the call can easily exceed the server's connection
> timer of 10 seconds.
> 
> This is even worse if the user doesn't accept for that length of time.
> 
> I know there was some plans for camera preview - so if we didn't actually
> start the call "dialing" until the preview is accepted (just show the
> connecting window), that might fix the issue.
> 
Preview will come in Fx35 with rooms
I think we need to fix it in Fx34 as it makes URL click backs very hard to use.

> We'd also have to be careful for audio calls, that we make sure to get the
> microphone before starting.
> 
> Adam, is this what was intended? If not, do we need some other form of
> allowing the link clicker to take a long time over accepting the gUM prompt?
> 
> Also, if the preview request gUM, and then the Tokbox SDK requests it, are
> we going to get two gUM prompts?

Comment 3

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #1)
> I've spoken to RT about this over irc. The main problem here seems to be the
> speed at which the gUM prompt is accepted on the link-clicker side.
> 
> If the user takes more than a few seconds, and then accepts the gUM, the
> stream set-up time for the call can easily exceed the server's connection
> timer of 10 seconds.

Just to make sure we're all on the same page here - what we're talking about is the setup call sequence here:

https://wiki.mozilla.org/images/1/1e/Loop-call-progress.png

And the problem you're talking about is that the time between the camera prompt (orange note between steps 6 and 7) and the time that the link clicker accepts the camera takes longer than the time between the camera prompt and the expiration of the connection timer (purple on this diagram).

The reason I want to clear this up is because:

> This is even worse if the user doesn't accept for that length of time.


> Adam, is this what was intended? If not, do we need some other form of
> allowing the link clicker to take a long time over accepting the gUM prompt?

If this is actually a problem in the field -- if the link clicker, actively engaged in making the call start, is taking longer to grant access than it takes the called user to notice the call, answer it, and *then* wait ten seconds on a regular basis -- then we can play around with the timer model design.

One relatively non-intrusive option would be to reset the "Connection Timer" to 30 seconds or so once the call state reaches "half-connected," which would give the link clicker an *additional* 30 seconds to accept the camera prompt. A more radical change would be to add another state to the call setup states, "waiting for camera access," and for the Loop server to hold off notifying the push server until after the link clicker indicates that media access has been granted (via a new WSS "action" value). But that's a pretty big change this late in the game.

> Also, if the preview request gUM, and then the Tokbox SDK requests it, are
> we going to get two gUM prompts?

Yeah, that's going to be an issue. When we add self-view-without-sending, we probably need to ask TB to add some methods to the publisher that initiate video self-view without putting it on the network.

I suspect the root cause here to be one of two things: either (1) The server has implemented timers in a way other than is specified at <https://wiki.mozilla.org/Loop/Architecture/MVP#Timer_Supervision>, or (2) the mechanics of calling one's self are not conducive to doing everything in the time frame allotted by the timers.

The comment about "This is even worse if the user doesn't accept for that length of time" makes me think that the answer is (1), although I do note that the fact that the gUM prompt disapperars when the browser window loses focus makes me think that (2) is at least as likely.

Mark: can you verify your comment about the problem being *worse* (rather than *better*, which is what I would expect) when the called party take a long time to answer? Because, if that's the case, then we need to look very closely at the server's timer implementation.

Comment 4

[Mark Banner (:standard8)]

We discussed this on IRC, and if I remember correctly the discussion centred around the fact that we're not prompting as soon as the user clicks "start".

Currently we wait until the session is connected for that.

The proposal we came up with was to prompt the user for device access as soon as they press start. Then wait until the accept before we move forward.

We might want to do this with camera preview, which the patch in bug 1039224 might help, or I think there may be another way that we can do this.

Comment 5

[Mark Banner (:standard8)]

I'm taking these two bugs to look at as soon as I finish hooking up direct calling.

Comment 6

[Adam Roach [:abr]]

Attachment: Prompt for camera before call

Mark -- this is the approach I mentioned over IRC. You'll note that the
resulting behavior is that the user is prompted for the microphone and camera
before the Loop server is contacted. This actually feels quite natural to me,
and it certainly makes the call setup go more smoothly.

Adding a self view on top of this patch is trivial (I have another patch that
does so in four lines of code), but we should land it as part of a different
bug.

There's one remaining flaw here: I can't seem to get the camera to release
when the user cancels the call. Perhaps you can poke at that aspect.

Comment 7

[Adam Roach [:abr]]

Oh, the patch also needs handling of the situation in which media capture fails (e.g., due to user rejecting the camera prompt). It should be easy to see where this should happen.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

(Commenting on User Story)
> Expected end result:
> 
> - When a caller visits a url, and clicks "Start", they are prompted for gUM
> straight away
> 
> - The POST /calls/<token> to the server (and hence prompt the callee) does
> NOT happen until gUM has been accepted
> 
> - Whilst waiting for gUM accepted, I'd propose, show the existing pending
> UI, but with a string like "Please accept the browser's prompt" (needs
> feedback from UX, but I'd go with simple for this bug, complicated later).
> 
> - If gUM is denied, show the "Something went wrong" page
> 
> - gUM should not be re-prompted when the call actually starts.
> 
> - Media should be released if the call attempt fails or is cancelled.
> 
> ToDo:
> 
> - Adam's patch provides a good basis for getting the media and remembering
> the streams until the sdk is ready.
> 
> * write tests for adam's patch
> * see if they expose any unknown issues
> * test/implement making cancel work
> * test/implement making camera release at end of call work

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: gUM tomfoolery, v2

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Updated version of abr's patch, rebased against fx-team.  Put in automated unit test shims.  Various things need to be considered and cleaned up around the testing bits, particularly around getting the existing tests to pass again, as well as how we want to glue this all in to the core code (the existing approach may be fine; not sure yet).

Thanks for getting this bootstrapped, abr!

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Darrin, it'd be helpful to have your feedback on the overall flow described in the user story here, both in general, but most especially any feedback and/or mocks that you'd propose regarding:

- Whilst waiting for gUM accepted, I'd propose, show the existing pending UI, but with a string like "Please accept the browser's prompt" (needs feedback from UX, but I'd go with simple for this bug, complicated later).

Comment 13

[Adam Roach [:abr]]

Attachment: Prompt to grant camera access

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #12)
> Darrin, it'd be helpful to have your feedback on the overall flow described
> in the user story here, both in general, but most especially any feedback
> and/or mocks that you'd propose regarding:
> 
> - Whilst waiting for gUM accepted, I'd propose, show the existing pending
> UI, but with a string like "Please accept the browser's prompt" (needs
> feedback from UX, but I'd go with simple for this bug, complicated later).

One of the approaches that I've seen that I thought was pretty novel was showing a representation of the browser indicating what the user is expected to do. Something like the image I'm attaching here (keeping in mind that I'm an engineer, not a graphic designer -- I'm sure these could be made to look nice). This avoids the "bouncing arrow" approach that a lot of sites try, since it's not (as) dependent on browser layout.

What would be really cool is if we could animate the image, so as to show people how to get the camera prompt back on Firefox in case they accidentally dismissed it.

Comment 14

[Romain Testard [:RT]]

Please note that visual help to help a user grant gUM access it dealt with on bug 1047040 with visuals already proposed.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: make standalone client request perms earlier

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: further progress on the tests for multiplex gum

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: get gUM perms earlier for Loop calls

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: get gUM perms earlier for Loop calls

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Cleaned up clownshoes, renamed module to loop.standaloneMedia, and got the various tests happy again.

Calls no longer call prompt for permissions twice, but they're not quite working yet either.  reset (nee resetMedia) still remains to be implemented.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: get gUM perms earlier for Loop calls

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: get gUM perms earlier for Loop calls

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: get gUM perms earlier for Loop calls

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: get gUM perms earlier for Loop calls

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Added the tests for the implementation of reset that we did in our last rev.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: get gUM perms earlier for Loop calls

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: get gUM perms earlier for Loop calls

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: get gUM perms earlier for Loop calls

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: get gUM perms earlier for Loop calls

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: get gUM perms earlier for Loop calls

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: get gUM perms earlier for Loop calls

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

This latest iteration has the fixed version of InitateConversationView.startCall that calls multiplexGum.reset() on error (and has a unit test for that).  

Unfortunately, this doesn't actually fix "declining gUM permissions leaves start button unfixably non-functional".  There are error messages about "too much recursion" from a few different spots in multiplexGum.js in the standalone console.

Comment 32

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #31)
> Unfortunately, this doesn't actually fix "declining gUM permissions leaves
> start button unfixably non-functional".

I notice that this patch hasn't yet implemented view changes for when start is clicked, but before the prompt is accepted. I suggested this in the original user story:

====
Whilst waiting for gUM accepted, I'd propose, show the existing pending UI, but with a string like "Please accept the browser's prompt" (needs feedback from UX, but I'd go with simple for this bug, complicated later).
====

This was intended to be an intermediate between this bug and doing something like bug 1047040.

I think if you implemented this intermediate view, it's a better prompt to the user that they've got to do something. You also get a cancel button, so they could swap back to the start view (and then possibly see the prompt when they hit start again).

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8515474 [details] [diff] [review]
get gUM perms earlier for Loop calls

Review of attachment 8515474 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/standalone/content/js/multiplexGum.js
@@ +73,5 @@
> +    /**
> +     * XXX give better name that makes side effects more obvious?
> +     */
> +    reset: function() {
> +      // When called from the ctor, userMedia is not created yet.

We should create a private variable _resetting and set that to true when entering this function and clear it when exiting the function. Return early if already resetting. This is necessary in case the error callback that is passed to getPermsAndCacheMedia is the reset function.

Comment 34

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) from comment #32)

> Whilst waiting for gUM accepted, I'd propose, show the existing pending UI,
> but with a string like "Please accept the browser's prompt" (needs feedback
> from UX, but I'd go with simple for this bug, complicated later).
> ====
> 
> This was intended to be an intermediate between this bug and doing something
> like bug 1047040.

But that requires new strings, which I don't believe would be feasible to land on Beta or even Alpha (right?) -- so something language neutral is actually more expedient, assuming we can get designs from Sevaan (or Darrin) quickly enough.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #33)

The other route would be to clone the error callbacks array, clear the original array, and then do the forEach on the cloned array.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

:abr this is all standalone code, so it's not subject to the alpha/beta desktop timelines.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: get gUM perms earlier for Loop calls

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: get gUM perms earlier for Loop calls

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: [landed fx-team] get gUM perms earlier for Loop calls (paired with jaws)

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8516282 [details] [diff] [review]
[landed fx-team] get gUM perms earlier for Loop calls (paired with jaws)

Review of attachment 8516282 [details] [diff] [review]:
-----------------------------------------------------------------

paired/reviewed by jaws and me.  This has grown large and complex enough that we want to land it sooner rather than later so we can get more testing, and we still do need to do something about the hidden gUM prompt.  I'll discuss with a few folks and probably file another bug for that.

Adam, if you have any thoughts on this patch when you get a chance, we'd love to hear them.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/73ab1f5493e3

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

Chatted with mreavy; the plan is either to move forward with bug 1047040 sooner or else for me to file an interim bug with a couple of options based on the stuff already discussed here.

Comment 43

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #42)
> Chatted with mreavy; the plan is either to move forward with bug 1047040
> sooner or else for me to file an interim bug with a couple of options based
> on the stuff already discussed here.

I've just tested this, and there's a small issue with the way the start button works. When the gUM prompt first opens there's typically a few seconds delay. This can make the user think they've not clicked the start button (which hasn't been disabled), and try clicking it again. At this time, its highly likely the prompt will then be hidden, and if they do find it, then there's some cases where it can abort the call (it tries to start the call twice and gets itself confused).

If we did the interim bug to create a basic view with some information in it, this would fix this issue and create the view that we'll need to help fix bug 1047040.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73ab1f5493e3

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

Spun off the gUM delay issue mentioned in comment 43 into bug 1093753.

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27553a983295

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8516282 [details] [diff] [review]
[landed fx-team] get gUM perms earlier for Loop calls (paired with jaws)

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 48

[u279076]

Adding status-firefox34:affected since this is an Fx34 blocker for Loop.

Comment 49

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #48)
> Adding status-firefox34:affected since this is an Fx34 blocker for Loop.

This is actually an issue in the standalone code, and not desktop. We haven't shipped it yet as there's some follow-on issues still being resolved. The uplifting to 35 is useful though, as it means less bitrot for any patches that rely on it.

Comment 50

[Maire Reavy [:mreavy]]

I removing "affected" from status-firefox34 since this is a standalone issue -- and we will be shipping a new standalone app within the next week.

Comment 51

[Adam Roach [:abr]]

Comment on attachment 8516282 [details] [diff] [review]
[landed fx-team] get gUM perms earlier for Loop calls (paired with jaws)

Clearing my f?, as it appears to have been overtaken by events.

Sorry I couldn't get to this before it landed. It looks good to me on a quick skim. Thanks for wrestling this to the ground.

Comment 52

[Paul Silaghi, QA [:pauly]]

> - When a caller visits a url, and clicks "Start", they are prompted for gUM straight away
Verified fixed FF 35b3, 36.0a2 (2014-12-16) Win 7