Closed Bug 1073410 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Windows 8
defect
Points:
8

Tracking

(firefox35 verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- verified
firefox36 --- verified
Blocking Flags:
backlog Fx34+

People

(Reporter: RT, Assigned: dmose)

References

Details

(Whiteboard: [standalone][todo: see user story])

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.

X write tests for adam's patch
X see if they expose any unknown issues
X test appropriate views call reset
X test/implement making "something went wrong" show up on cancel page
X test/implement whether camera frees up at end of call
X figure out what's up with errorCb console message on second call in a row
X test [retry a call] view
X declining gUM permissions leaves start button unfixably non-functional
* if gUM dialog hides or is hidden, start button non-functional until gUM dialog is re-opened and answered.

* decide if timeout necessary for not-now case; implement
* look at chrome behavior; decide what to do

Attachments

(3 files, 17 obsolete files)

10.42 KB, patch
Details | Diff | Splinter Review
40.31 KB, image/png
Details
33.33 KB, patch
dmose
: review+
Details | Diff | Splinter Review
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
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?
Flags: needinfo?(adam)
Summary: URL click-back calls disconnect 2 seconds after connecting when using Firefox → URL click-back calls disconnect 2 seconds after connecting when using Firefox (if link-clicker gUM is not accepted soon enough)
(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?
(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.
Flags: needinfo?(adam) → needinfo?(standard8)
Blocks: 1074517
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.
Depends on: 1039224
Flags: needinfo?(standard8)
I'm taking these two bugs to look at as soon as I finish hooking up direct calling.
Assignee: nobody → standard8
Target Milestone: --- → mozilla35
Iteration: --- → 35.3
Depends on: 1078747
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.
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.
Flags: qe-verify+
backlog: --- → Fx34+
Duplicate of this bug: 1083160
No longer blocks: 1074517
Whiteboard: [standalone]
Target Milestone: mozilla35 → ---
Blocks: 1039224
No longer depends on: 1039224
Assignee: standard8 → nobody
Iteration: 35.3 → 36.1
Points: --- → 5
User Story: (updated)
Whiteboard: [standalone] → [standalone][todo: see user story]
Blocks: 1047040
Blocks: 1069031
Points: 5 → 8
User Story: (updated)
Flags: firefox-backlog+
Iteration: 36.1 → ---
QA Contact: anthony.s.hughes → dmose
User Story: (updated)
Attached patch gUM tomfoolery, v2 (obsolete) — Splinter Review
Attachment #8500716 - Attachment is obsolete: true
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!
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).
Flags: needinfo?(dhenein)
Priority: -- → P1
Assignee: nobody → dmose
Iteration: --- → 36.2
QA Contact: dmose → anthony.s.hughes
Flags: firefox-backlog+
(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.
Please note that visual help to help a user grant gUM access it dealt with on bug 1047040 with visuals already proposed.
User Story: (updated)
User Story: (updated)
Attachment #8512310 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8513045 - Attachment is obsolete: true
Attachment #8513657 - Attachment is obsolete: true
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.
Blocks: 1091316
Attachment #8514438 - Attachment is obsolete: true
Attachment #8514523 - Attachment is obsolete: true
Attachment #8514569 - Attachment is obsolete: true
User Story: (updated)
Added the tests for the implementation of reset that we did in our last rev.
User Story: (updated)
User Story: (updated)
Attachment #8514657 - Attachment is obsolete: true
User Story: (updated)
Attachment #8500716 - Attachment is obsolete: false
Attachment #8515132 - Attachment is obsolete: true
User Story: (updated)
Attachment #8515195 - Attachment is obsolete: true
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.
(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 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.
(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.
(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.
:abr this is all standalone code, so it's not subject to the alpha/beta desktop timelines.
User Story: (updated)
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.
Attachment #8516282 - Flags: review+
Attachment #8516282 - Flags: feedback?(adam)
Flags: in-testsuite+
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.
Attachment #8516282 - Attachment description: get gUM perms earlier for Loop calls (paired with jaws) → [landed fx-team] get gUM perms earlier for Loop calls (paired with jaws)
(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.
https://hg.mozilla.org/mozilla-central/rev/73ab1f5493e3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Flags: firefox-backlog+
Spun off the gUM delay issue mentioned in comment 43 into bug 1093753.
Blocks: 1093787
Depends on: 1093793
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
Attachment #8516282 - Flags: approval-mozilla-aurora?
Adding status-firefox34:affected since this is an Fx34 blocker for Loop.
(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.
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.
Attachment #8516282 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(dhenein)
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.
Attachment #8516282 - Flags: feedback?(adam)
> - 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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.