Closed Bug 1180140 Opened 4 years ago Closed 4 years ago

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

Categories

(Hello (Loop) :: Server, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cornel_ionce, Assigned: rhubscher)

References

Details

Attachments

(1 file)

56 bytes, text/x-github-pull-request
rhubscher
: review+
Details | Review
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).
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.
Flags: needinfo?(adam)
(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.
Flags: needinfo?(adam)
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.
(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.
[Tracking Requested - why for this release]:
I think this issue is annoying enough to be tracked.
Let's try to fix this in time for FF40 release.
This is now wontfix for 40.

Mike/Adam - Can one of you take this bug?
Flags: needinfo?(mdeboer)
Flags: needinfo?(adam)
The only place this can be fixed is server-side. Tagging Alexis and Rémy to see who can take it.
Component: Client → Server
Flags: needinfo?(rhubscher)
Flags: needinfo?(alexis+bugs)
Flags: needinfo?(adam)
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.
Flags: needinfo?(alexis+bugs)
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.
Flags: needinfo?(rhubscher)
Flags: needinfo?(mdeboer)
Add tests that reproduce the problem.
Comment on attachment 8652220 [details] [review]
Link to Github PR — #353.

Add functionality.
Attachment #8652220 - Flags: review?(alexis+bugs)
Attachment #8652220 - Flags: feedback?(adam)
Assignee: nobody → rhubscher
Status: NEW → ASSIGNED
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.
Attachment #8652220 - Flags: feedback?(adam) → feedback-
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 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.
Attachment #8652220 - Flags: feedback- → feedback+
> 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.
https://github.com/mozilla-services/loop-server/commit/b7318d0b3c5e86ed5f1552550752d90912197160
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8652220 [details] [review]
Link to Github PR — #353.

@ametaireau give me a r+ directly on the github PR
Attachment #8652220 - Flags: review?(alexis+bugs) → review+
Duplicate of this bug: 1232650
You need to log in before you can comment on or make changes to this bug.