1122486 - Hello calls frequently fail to complete video connection between two users

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

I've seen this in the past, but I've been seeing it again whilst testing locally with local running loop servers. What happens is:

1) Generate a link clicker url
2) Open it in a second browser
3) Start the call
4) Accept the media prompt
5) Accept the incoming call on the first browser

Expected Results

=> Call sets up with two-way video

Actual Results

=> Call sets up with video going one-way only, even though both local videos are shown.
=> After a few seconds, the call aborts and says "Something went wrong" on the callee end, and "Your conversation has ended" on the end being called.

I think if we can work out what this is, it might provide some insight into the high levels of timeouts in bug 1119574.

Comment 1

[Mark Banner (:standard8)]

Ok, I've done some investigation into this. In the past I've seen it with calls, but now we're improving the stability of our functional tests with rooms - I'm also seeing it there as well.

The characteristics:

- Seems to be random, possibly up to 30% of the time on average (I get it frequently when running tests on my machine with debug build, may vary on different setups etc).
- No remote video element gets created/displayed (we just get a white background).
- I'm running a local setup, i.e. loop-server and standalone servers are local, TokBox using dev keys

Looking at the logs:

- The SDK is producing the following error (version 2.2.9.1):

TypeError: stream.connection is undefined sdk.js:20382:0

- I'm not seeing any notifications from the sdk that the remote stream has been received - this seems to tie in with the TypeError, as the streamCreatedHandler is throwing before it would send the notification to the Loop code.



Next I'm going to test this with the latest sdk, and check the client flow.

Comment 2

[Mark Banner (:standard8)]

Further investigation has shown that using version 2.4.0 of the sdk seems to fix the error - we want to update to that in bug 1093780.

However, that's quite a big update, so I took a look at 2.2.9.3 (also originally provided on bug 1093780). This seems to have had an explicit fix for something like the issue we're seeing - and tests reveal it to be fine as well.

Unfortunately 2.2.9.3 has a bug with the audio level display meter (it is shown, when it shouldn't be) - but I've worked up a patch for that.

So I think what we should do is:

- Get 2.2.9.3 on Aurora & Beta for testing to go out in the next releases.
- Get 2.4.0 on Nightly alongside the screensharing code and have it under testing as we go.

We may/may not land 2.2.9.3 on nightly as an intermediate.

Comment 3

[Mark Banner (:standard8)]

Attachment: Upgrade Loop's use of Tokbox SDK 2.2.9.3 to fix issues with calls and rooms intermitently failing to connect.

This upgrades us to 2.2.9.3 - I've tested it locally, and I've been running it through the functional tests lots - these were intermittently failing for me with this bug with the existing 2.2.9.3 code.

There's a separate patch coming for the audio display level meter fix, that I want to record separately.

Comment 4

[Mark Banner (:standard8)]

Attachment: Fix sdk audioLevelDisplayMeter to not always be displayed

This fixes the 2.2.9.3 of the sdk to not show the audioLevelDisplayMeter all the time. It is based on code I found in 2.4.0.

Is TokBox happy for us to ship this patched version for two cycles, whilst we deploy and test 2.4.0? (its a smaller patch, which means we're more likely to get approval on our beta train for the next release).

Comment 5

[Mark Banner (:standard8)]

[Tracking Requested - why for this release]: This is one possible cause of a lot of failures that we've been seeing for users. Its intermittent, so doesn't always show up on all environments, but given we know we have a higher than we'd like failure count, then we should fix be fixing this.

I'll nominate the fix once its got reviews & is ready for landing.

Comment 6

[msander]

Attachment: webrtc-js-2.2.9.6.tgz

Comment 7

[msander]

(In reply to Mark Banner (:standard8) from comment #4)
attachment 8551994 [details] has a slightly newer version of our JS SDK with a fix for the audio levels display bug. Could you give that one a try?

Comment 8

[Mark Banner (:standard8)]

(In reply to msander from comment #7)
> (In reply to Mark Banner (:standard8) from comment #4)
> attachment 8551994 [details] has a slightly newer version of our JS SDK with
> a fix for the audio levels display bug. Could you give that one a try?

I just tested this, and it seems to still display the audio versions. The actions I'm trying here are: Join a room, mute video. At which stage the audio level is still displayed.

Comment 9

[Mark Banner (:standard8)]

Mike: I've just checked the diffs between 2.2.9.3 and 2.2.9.6 and I can't see anything in there that would change the audio level display issue. Am I missing something or did I get the wrong version?

Comment 10

[msander]

Mark: I see the bug in 2.2.9.1 but not 2.2.9.3 or 2.2.9.6. Ankur is going to take a look.

Comment 11

[aoberoi]

Attachment: loop-opentok-testbed.zip

audioLevelDisplayMode works as expected.

Comment 12

[aoberoi]

actually, i was wrong. it works in chrome but not in firefox.

Comment 13

[Mark Banner (:standard8)]

(In reply to aoberoi from comment #11)
> Created attachment 8554042 [details]
> loop-opentok-testbed.zip
> 
> audioLevelDisplayMode works as expected.

Using FirefoxNightly (38), and Chrome release (40), I see the audio level indicator in both of them.

Comment 14

[aoberoi]

@standard8, you see in in chrome when testing with the attached sample code?

Comment 15

[Mark Banner (:standard8)]

(In reply to aoberoi from comment #14)
> @standard8, you see in in chrome when testing with the attached sample code?

Yes, using the 2.2.9.6 version (I unpacked and ran it locally on Mac) - you have to talk to see the green indicator come into the corner.

Comment 16

[msander]

Attachment: webrtc-js-2.2.9.7.tgz

Here is a patch for the 2.2.9 SDK with a fix for the audio levels ui.

Comment 17

[Mark Banner (:standard8)]

Attachment: Upgrade Loop's use of Tokbox SDK 2.2.9.7 to fix issues with calls and rooms intermitently failing to connect.

The new 2.2.9.7 version does fix the audio level indicator regression that 2.2.9.3/6 both had.

I've also run this through the functional tests a few times (with a couple of additional patches) and it has run successfully with none of the intermittent failing to connect that I was seeing before.

Comment 18

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8555108 [details] [diff] [review]
Upgrade Loop's use of Tokbox SDK 2.2.9.7 to fix issues with calls and rooms intermitently failing to connect.

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

Built latest fx-team with this patch and the one for bug 1126036 applied, ran a full call, worked smoothly. Looks good to me.

Comment 19

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/4631f0283363

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4631f0283363

Comment 21

[Mark Banner (:standard8)]

Attachment: Branch Patch

Adjusted patch for branches.

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello, possible the previous upgrade regressed it, but that was before we shipped in 34/35.
[User impact if declined]: Occasionally a room or call may start up but fail to connect properly with no obvious reason as to why. This is timing related, it could be that some users see this more than others, but we haven't done an analysis.
[Describe test coverage new/current, TreeHerder]: Partner supplied sdk. Landed on central, nightly builds now have functional test coverage run outside of treeherder.
[Risks and why]: Low - partner supplied sdk, manual testing. Repeated local functional testing against central showed no errors.
[String/UUID change made/needed]: None

Comment 22

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d04c214b5479
https://hg.mozilla.org/releases/mozilla-beta/rev/787bb877e60b