1180140 - The user is unable to join back the conversation after a crash

Status: RESOLVED FIXED

(Hello (Loop) :: Server, defect)

Description

[Cornel Ionce [:noni] [Hubs QA]]

Reproducing on:
*Win 10 x64
*Win 7 x64
*Ubuntu 14.04 32bit
*Mac OS X 10.9.5


STR:
1. Open Firefox.
2. Start a Hello call and wait for another person to join.
3. Crash the browser (I've used http://ted.mielczarek.org/mozilla/crashme.html).
4. Restart the browser and try to enter the conversation.

Expected Results:
The user rejoins the conversation without any issues.

Actual Results:
The user can no longer join the conversation.
"Something went wrong" is displayed in the conversation window and several errors are thrown in the console.


Notes:
Screenshot: http://i.imgur.com/86LrswO.png
Browser console errors: http://i.imgur.com/qHGxE9t.png

Reproduced with latest Nightly (build ID: 20150702030207) and Firefox 40 beta 1 (build ID: 20150702173756).
It also reproduced when the browser crashed after a tab share was started (bug 1154271).

Comment 1

[Mike de Boer [:mikedeboer]]

Adam, what would there be against allowing the link generator to _always_ re-enter their own room(s)?

Pardon me if this question has already been asked in another bug.

Comment 2

[Adam Roach [:abr]]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Adam, what would there be against allowing the link generator to _always_
> re-enter their own room(s)?
> 
> Pardon me if this question has already been asked in another bug.

So, the tricky thing is determining whether the link generator is re-entering a room, or entering from a different place. Consider the expected behavior with the following steps:

1) I log into FxA and create a Hello room. I stay in the room.
2) I pass the URL to you and you join the room.
3) I log into FxA on a second machine and join that same room.

Right now, on step 3, my other machine would be told that the room is full. With your proposal, what would you expect to happen instead?

To be clear, we could work out some more complicated ways of dealing with the situation (e.g., uniquely identifying clients and allowing a client to replace itself), but the very simple fix of not enforcing room limits on the creator leads to some unfortunate corner cases.

Comment 3

[Adam Roach [:abr]]

To be clear, if the room creator tries to join the room from the same client, the HAWK token should be identical (which won't be true if he's using a different client or profile). If we want the server to allow a user to replace themselves, this would be a way to detect that it's the same instance that crashed versus another client associated with the same account.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #3)
> To be clear, if the room creator tries to join the room from the same
> client, the HAWK token should be identical (which won't be true if he's
> using a different client or profile). If we want the server to allow a user
> to replace themselves, this would be a way to detect that it's the same
> instance that crashed versus another client associated with the same account.

I think I kinda like this... just supporting the identical HAWK credentials case, that is.

Comment 5

[Robert Kaiser]

[Tracking Requested - why for this release]:
I think this issue is annoying enough to be tracked.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Let's try to fix this in time for FF40 release.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

This is now wontfix for 40.

Mike/Adam - Can one of you take this bug?

Comment 8

[Adam Roach [:abr]]

The only place this can be fixed is server-side. Tagging Alexis and Rémy to see who can take it.

Comment 9

[Alexis Metaireau (:alexis)]

Just to add some more info, I believe this information expires on the server side after 5mn

https://github.com/mozilla-services/loop-server/blob/master/loop/config.js#L489-L493

We'll fix this behaviour on the server side to accept a participant to rejoin.

Comment 10

[Rémy Hubscher (:natim)]

I think the part to change is https://github.com/mozilla-services/loop-server/blob/master/loop/routes/rooms.js#L53-L65

I will write a test that reproduce the wanted behavior.

Comment 11

[Rémy Hubscher (:natim)]

Attachment: Link to Github PR — #353.

Add tests that reproduce the problem.

Comment 12

[Rémy Hubscher (:natim)]

Comment on attachment 8652220 [details] [review]
Link to Github PR — #353.

Add functionality.

Comment 13

[Adam Roach [:abr]]

Comment on attachment 8652220 [details] [review]
Link to Github PR — #353.

This is close, but I'm afraid it doesn't deal correctly with a logged-in user with two browsers. Consider: I'm in a room with Natim on my laptop. Then, without leaving the room, I go to my desktop (which is logged into the same FxA account) and try to join the room. We really, really don't want the server to let my desktop in under these circumstances.

As I explained in comment 3, you need to check for identical HAWK tokens. Simply being the same user is not sufficient.

Comment 14

[Rémy Hubscher (:natim)]

Actually I implemented exactly what you describe. You can try it, if you don't log twice with the same Hawk credentials you will not be in, req.user is the HawkSession not the FxA session.

Comment 15

[Adam Roach [:abr]]

Comment on attachment 8652220 [details] [review]
Link to Github PR — #353.

(In reply to Rémy Hubscher (:natim) from comment #14)
> Actually I implemented exactly what you describe. You can try it, if you
> don't log twice with the same Hawk credentials you will not be in, req.user
> is the HawkSession not the FxA session.

Ah, sorry -- the names are very confusing. Based on what you say here, this seems okay. Thanks.

Comment 16

[Rémy Hubscher (:natim)]

> the names are very confusing

Yes Alexis told me the same thing, I will try to add a big comment to explain what's going on in that part of the code. Thanks.

Comment 17

[Rémy Hubscher (:natim)]

https://github.com/mozilla-services/loop-server/commit/b7318d0b3c5e86ed5f1552550752d90912197160

Comment 18

[Rémy Hubscher (:natim)]

Comment on attachment 8652220 [details] [review]
Link to Github PR — #353.

@ametaireau give me a r+ directly on the github PR