Closed Bug 1092954 Opened 5 years ago Closed 5 years ago

Before deleting a Loop room, the client should check room membership and disconnect all current participants

Categories

(Hello (Loop) :: Client, defect, P1, major)

defect
Points:
2

Tracking

(firefox35+ verified, firefox36+ verified, firefox37 verified)

VERIFIED FIXED
mozilla37
Iteration:
37.1
Tracking Status
firefox35 + verified
firefox36 + verified
firefox37 --- verified
Blocking Flags:
backlog Fx35+

People

(Reporter: NiKo, Assigned: mikedeboer)

References

Details

(Whiteboard: [rooms])

Attachments

(2 files, 4 obsolete files)

From the spec[0]:

> Client implementation note: Before deleting a room, the client should
> check room membership and forceDisconnect() all current participants.

[0]: https://wiki.mozilla.org/Loop/Architecture/Rooms#DELETE_.2Frooms.2F.7Btoken.7D
Blocks: 1074671
backlog: --- → Fx35+
Priority: -- → P1
Depends on: 1089722
Whiteboard: [rooms]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Niko, does this look like the right approach to you?
Attachment #8524679 - Flags: feedback?(nperriault)
Comment on attachment 8524679 [details] [diff] [review]
Patch v1: oust all users from a room when it's deleted

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

Sorry for the latency :/ Looks like it's on the right track! I suspect there's also loop-server side work to do as well, but that's for another bug.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +132,5 @@
> +
> +      Object.keys(this.connections).forEach(function(id) {
> +        var connection = this.connections[id];
> +        this.session.forceDisconnect(connection, noop);
> +      }.bind(this));

Nit: You can pass `this` as the second arg of forEach. Alternatively:

for (var id in this.connections) {
  this.session.forceDisconnect(this.connections[id], noop);
}
Attachment #8524679 - Flags: feedback?(nperriault) → feedback+
Niko, how would you start writing a test for this and where?
Attachment #8524679 - Attachment is obsolete: true
Attachment #8526800 - Flags: review?(nperriault)
Comment on attachment 8526800 [details] [diff] [review]
Patch v2: oust all users from a room when it's deleted

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

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Created attachment 8526800 [details] [diff] [review]
> Patch v2: oust all users from a room when it's deleted
> 
> Niko, how would you start writing a test for this and where?

Respectively in activeRoomStore_test.js and otSdkDriver_test.js in the loop/test/shared folder; You'll find plenty of tests written to test similar things.

Don't hesitate pinging me on IRC if you need more help; if you want, we could even pair on this!
Attachment #8526800 - Attachment is obsolete: true
Attachment #8526800 - Flags: review?(nperriault)
Attachment #8528310 - Flags: review?(nperriault)
Attached patch Patch 2: tests (obsolete) — Splinter Review
Attachment #8528311 - Flags: review?(nperriault)
Iteration: 36.3 → 37.1
Comment on attachment 8528310 [details] [diff] [review]
Patch 1.3: oust all users from a room when it's deleted

Niko hasn't been feeling well.  So I'm adding Mark as an additional reviewer.  We only need a review from one.
Attachment #8528310 - Flags: review?(standard8)
Comment on attachment 8528311 [details] [diff] [review]
Patch 2: tests

Niko hasn't been feeling well.  So I'm adding Mark as an additional reviewer.  We only need a review from one.
Attachment #8528311 - Flags: review?(standard8)
Comment on attachment 8528310 [details] [diff] [review]
Patch 1.3: oust all users from a room when it's deleted

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

I can't see how this would work at the moment - the panel is generating the deleteRoom action, but there's nothing that links it to the conversation window.

I think the easiest way around this, is to do this as/just after the room is deleted - i.e. make activeRoomStore listen to mozLoop.rooms.on("delete:<token>"...). When it gets that, it should dispatch an action such as "roomDeleted" (to be consistent with the way we handle our actions).

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +438,5 @@
> +      if (actionData.roomToken != this.getStoreState().roomToken) {
> +        return;
> +      }
> +
> +      this._sdkDriver.forceDisconnectAll();

We need to do more that just to disconnect everything - we should also call the private function _leave, and I think we should then follow-up with window.close() to fully shut everything down.

Although now I think of it, calling window.close() may be enough, because there's an unload handler that ends up calling _leave.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +131,5 @@
> +      }
> +
> +      Object.keys(this.connections).forEach(function(id) {
> +        var connection = this.connections[id];
> +        this.session.forceDisconnect(connection, noop);

the callback is optional, so we don't actually need to specify it here.
Attachment #8528310 - Flags: review?(standard8)
Attachment #8528310 - Flags: review?(nperriault)
Attachment #8528310 - Flags: review-
Comment on attachment 8528311 [details] [diff] [review]
Patch 2: tests

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

The general ideas here looks fine, but you'll obviously need to update for the changes for the main patch.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +312,5 @@
>  
>            sinon.assert.calledOnce(dispatcher.dispatch);
>            sinon.assert.calledWithExactly(dispatcher.dispatch,
>              new sharedActions.RemotePeerConnected());
> +          expect(driver.connections).to.include.keys("remoteUser");

We generally prefer to keep individual checks in different it()s because then it makes it clearer what a particular function does.
Attachment #8528311 - Flags: review?(standard8)
Attachment #8528311 - Flags: review?(nperriault)
[Tracking Requested - why for this release]:
We need this fix for "Rooms", which is already enabled in Fx35
Hi Mike, do you have time to finish this for this week?  If not reach out to Mark and he can work with you to bang it out quick.  trying to land this week to uplift for next beta build.
Severity: normal → major
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
Attached patch Patch 2.1: testsSplinter Review
Attachment #8528311 - Attachment is obsolete: true
Attachment #8532305 - Flags: review?(standard8)
Comment on attachment 8532304 [details] [diff] [review]
Patch 1.4: oust all users from a room when it's deleted

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

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +408,5 @@
>  
>        // If we're closing the window, we can stop listening to updates.
> +      var roomToken = this.getStoreState().roomToken;
> +      this._mozLoop.rooms.off("update:" + roomToken, this._handleRoomUpdate.bind(this));
> +      this._mozLoop.rooms.off("delete:" + roomToken, this._handleRoomDelete.bind(this));

This needs bitrot fixing in the latest code. In particular, we found that the function parameter wasn't necessary, and was somehow causing the listener to not actually be removed.
Attachment #8532304 - Flags: review?(standard8) → review+
Comment on attachment 8532305 [details] [diff] [review]
Patch 2.1: tests

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

r+ with the changes mentioned.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +312,5 @@
>  
>            sinon.assert.calledOnce(dispatcher.dispatch);
>            sinon.assert.calledWithExactly(dispatcher.dispatch,
>              new sharedActions.RemotePeerConnected());
> +          expect(driver.connections).to.include.keys("remoteUser");

Please make this addition a separate:

it("should store the connection details for a remote user"...

@@ +322,5 @@
>              connection: {id: "localUser"}
>            });
>  
>            sinon.assert.notCalled(dispatcher.dispatch);
> +          expect(driver.connections).to.not.include.keys("localUser");

Please make this addition a separate:

it("should not store the connection details for a local user"...
Attachment #8532305 - Flags: review?(standard8) → review+
(In reply to Mark Banner (:standard8) from comment #15)
> This needs bitrot fixing in the latest code. In particular, we found that
> the function parameter wasn't necessary, and was somehow causing the
> listener to not actually be removed.

Well, that code puzzled me as well! The problem lies in the `.bind(this)` part; this yields a different function Object when invoked:

```js
let func = function() {};
func.bind(this) === func.bind(this); // false
func.bind(this) == func.bind(this);  // false
```
https://hg.mozilla.org/mozilla-central/rev/73349687b426
https://hg.mozilla.org/mozilla-central/rev/06e168312ef1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8532304 [details] [diff] [review]
Patch 1.4: oust all users from a room when it's deleted

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: If a room is deleted, people in that room will sit there not realizing it.

[Describe test coverage new/current, TBPL]: Includes test in second patch on bug.

[Risks and why]: moderately low risk; new function; confined to rooms.  At this moment only asking for Aurora approval; may ask for beta after it's on Aurora and further verified.

[String/UUID change made/needed]: none
Attachment #8532304 - Flags: approval-mozilla-aurora?
Attachment #8532305 - Flags: approval-mozilla-aurora?
Attachment #8532304 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8532305 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
What should happen with the standalone client in this case ?
(In reply to Paul Silaghi, QA [:pauly] from comment #22)
> What should happen with the standalone client in this case ?
Because the standalone client still remains on the call.
Flags: needinfo?(mreavy)
(In reply to Paul Silaghi, QA [:pauly] from comment #23)
> (In reply to Paul Silaghi, QA [:pauly] from comment #22)
> > What should happen with the standalone client in this case ?
> Because the standalone client still remains on the call.

My understanding is that this will be fixed when the Loop server is upgraded to 0.14.1 this week.  Alexis and/or Mike -- Am I correct?
Flags: needinfo?(mreavy)
Flags: needinfo?(mdeboer)
Flags: needinfo?(alexis+bugs)
Yes, to be able to force disconnect the other peers, the room owner needs a "moderator" role. This was solved as part of bug 1107743 and will be shipped with 0.14.1.
Depends on: 1107743
Flags: needinfo?(alexis+bugs)
Flags: needinfo?(mdeboer)
Do we need to uplift here for Beta 35?  If so please nominate by Mon Dec 22 to get this into next week's only desktop beta.
Flags: needinfo?(rjesup)
Comment on attachment 8532304 [details] [diff] [review]
Patch 1.4: oust all users from a room when it's deleted

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

This seems stable on aurora and m-c.  Risk is low and fairly confined.
Attachment #8532304 - Flags: approval-mozilla-beta?
Attachment #8532304 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(rjesup)
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #24)
> (In reply to Paul Silaghi, QA [:pauly] from comment #23)
> > (In reply to Paul Silaghi, QA [:pauly] from comment #22)
> > > What should happen with the standalone client in this case ?
> > Because the standalone client still remains on the call.
> 
> My understanding is that this will be fixed when the Loop server is upgraded
> to 0.14.1 this week.  Alexis and/or Mike -- Am I correct?

Here is what I see on the standalone page on the latest nightly: http://i.imgur.com/5VkQ0hS.png
'Leave' button still active, no error message explaining what's happening (ie: the conversation is no longer available), a small rectangle on the bottom right corner.
Thoughts ?
Flags: needinfo?(mreavy)
(In reply to Paul Silaghi, QA [:pauly] from comment #29)
> Here is what I see on the standalone page on the latest nightly:
> http://i.imgur.com/5VkQ0hS.png

That's bad :(

> 'Leave' button still active, no error message explaining what's happening
> (ie: the conversation is no longer available), a small rectangle on the
> bottom right corner.
> Thoughts ?

Did you have a chance to copy the browser console logs? Would greatly help to figure out what possibly happened.

Also, if you have steps to reproduce, that would also help a little :)
I'll file in a bit a new bug for investigation, with all the details.
Depends on: 1118246
Thanks, Pauly.  I can repro this.  There is nothing in the browser or web consoles.

Niko -- if the desktop user deletes a room while in a call (admittedly, an edge case), you'll see what Pauly is reporting.  NOTE also: if the room is deleted while the standalone user is in the room (but the desktop user is not), nothing appears to happen on the standalone side.

This has to be a server or standalone issue (not a desktop client issue).  I'm discussing it with Mark (standard8) on irc now, and he's doing investigation.  We can discuss the details on the new bug that Pauly just filed (bug 1118246).
Flags: needinfo?(mreavy)
Marking this verified since the desktop client is closing correctly when deleting the active conversation.
Verified fixed FF 35 RC, 36.0a2 (2015-01-06), 37.0a1 (2015-01-06) Win 7.
Remaining work continues in bug 1118246.
You need to log in before you can comment on or make changes to this bug.