Closed Bug 1076794 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Points:
1

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: andreio)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
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
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"
Reminding myself to look at this tomorrow.
Flags: needinfo?(standard8)
Attached 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)
Yes I could do that.
Flags: needinfo?(andrei.br92)
backlog: --- → Fx36+
backlog: Fx36+ → Fx35?
Attachment #8511612 - Flags: feedback?(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).
Attachment #8511612 - Flags: feedback?(standard8) → feedback+
backlog: Fx35? → Fx35+
Priority: -- → P2
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)
(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)
Assignee: nobody → standard8
Attachment #8504666 - Attachment is obsolete: true
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+
https://hg.mozilla.org/integration/fx-team/rev/c44a85b519ed
Assignee: standard8 → andrei.br92
Iteration: --- → 36.3
Points: --- → 1
Target Milestone: --- → mozilla36
(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: 6 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.