1076794 - Loop calls incorrectly end as "conversation ended" (aka successful) when the network drops

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Steps to reproduce:

1) Set up a Loop call via the link-clicker UI between two computers, so that the streams are ongoing.
2) Disconnect the network (e.g. turn off wifi)
3) Wait ~ 20 to 30 seconds

Actual results:

"Your conversation has ended" is displayed in the title

Expected results (after bug 1047406):

"Something went wrong" is displayed in the title.

Comment 1

[Mark Banner (:standard8)]

The issue here is that sometime in the past, the SDK changed from emitting a "networkDisconnected" event from the session, to emitting "sessionDisconnect" with a reason of "networkDisconnected"

https://tokbox.com/opentok/libraries/client/js/reference/SessionDisconnectEvent.html

Not sure if this changed without us knowing, or if its just a general programming error at some stage.

Comment 2

[u279076]

Nils, do you think you could get Tokbox to look into this?

Comment 3

[Mark Banner (:standard8)]

I don't think we need to get Tokbox to look at this. I suspect its a programming issue on our part.

Reason: I just went back to the first version of the sdk we landed in mozilla-central, and I couldn't see any event being fired for "networkDisconnected".

So I suspect this was a programming error (misunderstanding docs) rather than an issue because of Tokbox changes. At this stage I'm happy just to go forward with us correcting the mistake.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Mark Banner (:standard8) from comment #3)
> I don't think we need to get Tokbox to look at this. I suspect its a
> programming issue on our part.
> 
> Reason: I just went back to the first version of the sdk we landed in
> mozilla-central, and I couldn't see any event being fired for
> "networkDisconnected".
> 
> So I suspect this was a programming error (misunderstanding docs) rather
> than an issue because of Tokbox changes. At this stage I'm happy just to go
> forward with us correcting the mistake.

I guess you may find this comment interesting, have a look at [1]. Hope it helps.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1067946#c7

Comment 5

[Andrei Oprea [:andreio]]

I'm having a hard time trying to get this to reproduce. Went through the STR but I get the following sequence of events "session:peer-hangup" followed by "session:ended"

Comment 6

[Mark Banner (:standard8)]

Reminding myself to look at this tomorrow.

Comment 7

[Mark Banner (:standard8)]

Attachment: sessiondisconnects.txt

Andrei, I think you were probably testing with disconnecting the peer's end, rather than the local end.

When testing disconnecting at the peer's end, https://bugzilla.mozilla.org/show_bug.cgi?id=1067946#c7 is definitely relevant here.

These are roughly the changes I was thinking about for this bug. They would need some tidy up and tests writing, Andrei, would you have time to do that?

Comment 8

[Andrei Oprea [:andreio]]

Yes I could do that.

Comment 9

[Andrei Oprea [:andreio]]

Attachment: Handle networkDisconnected events and present proper error message

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8511612 [details] [diff] [review]
Handle networkDisconnected events and present proper error message

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

I've not tested it, but it definitely looks like what I was expecting.

::: browser/components/loop/content/shared/js/models.js
@@ +331,3 @@
>        this.set("connected", false)
>            .set("ongoing", false)
> +          .trigger("session:ended", event)

Do we really want to trigger ended all the time? I'd have expected this a failure to replace ended (you'll also end up sending session:ended twice in the case of nice disconnection).

Comment 11

[Andrei Oprea [:andreio]]

You're right. How about something like this

>     _signalEnd: function(eventName, event) {
>       this.set("connected", false)
>           .set("ongoing", false)
>           .trigger(eventName, event);
>        if (eventName === "session:ended") {
>           this.trigger("session:ended", event);
>        }
>     },

Comment 12

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #11)
> You're right. How about something like this
> 
> >     _signalEnd: function(eventName, event) {
> >       this.set("connected", false)
> >           .set("ongoing", false)
> >           .trigger(eventName, event);
> >        if (eventName === "session:ended") {
> >           this.trigger("session:ended", event);
> >        }
> >     },

Why do you need the second if statement? That's just doing the trigger again for eventName...

Comment 13

[Mark Banner (:standard8)]

Attachment: Make Loop calls handle networkDisconnected events properly so that the correct messages get displayed.

With the extra trigger removed as it was unnecessary.

I've reviewed it and given it a fuller test as I can, and it works fine. This should fix the remaining network handling.

Giving this r=Standard8 as Andrei hasn't been about just to finish this last bit, but we want to get this landed.

Comment 14

[Mark Banner (:standard8)]

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

Comment 15

[Andrei Oprea [:andreio]]

(In reply to Mark Banner (:standard8) from comment #13)
> Created attachment 8522141 [details] [diff] [review]
> Make Loop calls handle networkDisconnected events properly so that the
> correct messages get displayed.
> 
> With the extra trigger removed as it was unnecessary.
> 
> I've reviewed it and given it a fuller test as I can, and it works fine.
> This should fix the remaining network handling.
> 
> Giving this r=Standard8 as Andrei hasn't been about just to finish this last
> bit, but we want to get this landed.

I didn't get it from your comment that it was so trivial to fix.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c44a85b519ed

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/e48ee58c73e7

Comment 18

[u279076]

Assuming the provided test coverage is sufficient. Please need-info me to request further test coverage.