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

RESOLVED FIXED in Firefox 35

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: andreio)

Tracking

unspecified
mozilla36
Points:
1
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
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.
Keywords: regression
Nils, do you think you could get Tokbox to look into this?
Flags: needinfo?(drno)
QA Contact: anthony.s.hughes → drno
Reporter

Comment 3

5 years ago
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.
Flags: needinfo?(drno)
Keywords: regression
(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
Assignee

Comment 5

5 years ago
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"
Reporter

Comment 6

5 years ago
Reminding myself to look at this tomorrow.
Flags: needinfo?(standard8)
Reporter

Comment 7

5 years ago
Posted patch sessiondisconnects.txt (obsolete) — Splinter Review
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?
Flags: needinfo?(standard8) → needinfo?(andrei.br92)
Assignee

Comment 8

5 years ago
Yes I could do that.
Flags: needinfo?(andrei.br92)

Updated

5 years ago
backlog: --- → Fx36+
backlog: Fx36+ → Fx35?
Assignee

Updated

5 years ago
Attachment #8511612 - Flags: feedback?(standard8)
Reporter

Comment 10

5 years ago
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).
Attachment #8511612 - Flags: feedback?(standard8) → feedback+

Updated

5 years ago
backlog: Fx35? → Fx35+
Priority: -- → P2
Assignee

Comment 11

5 years ago
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);
>        }
>     },
Flags: needinfo?(standard8)
Reporter

Comment 12

5 years ago
(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...
Flags: needinfo?(standard8)

Updated

5 years ago
Assignee: nobody → standard8
Reporter

Updated

5 years ago
Attachment #8504666 - Attachment is obsolete: true
Reporter

Comment 13

5 years ago
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.
Attachment #8511612 - Attachment is obsolete: true
Attachment #8522141 - Flags: review+
Reporter

Comment 14

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/c44a85b519ed
Assignee: standard8 → andrei.br92
Iteration: --- → 36.3
Points: --- → 1
Target Milestone: --- → mozilla36
Assignee

Comment 15

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/c44a85b519ed
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assuming the provided test coverage is sufficient. Please need-info me to request further test coverage.
Flags: qe-verify-
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.