Closed
Bug 1092954
Opened 10 years ago
Closed 9 years ago
Before deleting a Loop room, the client should check room membership and disconnect all current participants
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox35+ verified, firefox36+ verified, firefox37 verified)
backlog | Fx35+ |
People
(Reporter: NiKo, Assigned: mikedeboer)
References
Details
(Whiteboard: [rooms])
Attachments
(2 files, 4 obsolete files)
8.70 KB,
patch
|
standard8
:
review+
Sylvestre
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
6.26 KB,
patch
|
standard8
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [rooms]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
Niko, does this look like the right approach to you?
Attachment #8524679 -
Flags: feedback?(nperriault)
Reporter | ||
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
Niko, how would you start writing a test for this and where?
Attachment #8524679 -
Attachment is obsolete: true
Attachment #8526800 -
Flags: review?(nperriault)
Reporter | ||
Comment 4•9 years ago
|
||
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!
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8526800 -
Attachment is obsolete: true
Attachment #8526800 -
Flags: review?(nperriault)
Attachment #8528310 -
Flags: review?(nperriault)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8528311 -
Flags: review?(nperriault)
Updated•9 years ago
|
Iteration: 36.3 → 37.1
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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 9•9 years ago
|
||
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 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
[Tracking Requested - why for this release]: We need this fix for "Rooms", which is already enabled in Fx35
tracking-firefox35:
--- → ?
tracking-firefox36:
--- → ?
Comment 12•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8528310 -
Attachment is obsolete: true
Attachment #8532304 -
Flags: review?(standard8)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8528311 -
Attachment is obsolete: true
Attachment #8532305 -
Flags: review?(standard8)
Comment 15•9 years ago
|
||
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 16•9 years ago
|
||
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+
Assignee | ||
Comment 17•9 years ago
|
||
(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 ```
Assignee | ||
Comment 18•9 years ago
|
||
Thanks! Landed on fx-team witeh comments addressed as: https://hg.mozilla.org/integration/fx-team/rev/73349687b426 https://hg.mozilla.org/integration/fx-team/rev/06e168312ef1
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/73349687b426 https://hg.mozilla.org/mozilla-central/rev/06e168312ef1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 20•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8532305 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8532304 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8532305 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c5c7200c6eb https://hg.mozilla.org/releases/mozilla-aurora/rev/caecc9103058
Comment 22•9 years ago
|
||
What should happen with the standalone client in this case ?
Comment 23•9 years ago
|
||
(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)
Comment 24•9 years ago
|
||
(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)
Comment 25•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mdeboer)
Comment 26•9 years ago
|
||
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 27•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8532304 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Flags: needinfo?(rjesup)
Comment 28•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/3e51010c8454 https://hg.mozilla.org/releases/mozilla-beta/rev/9240c2ca5cf7
Comment 29•9 years ago
|
||
(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)
Reporter | ||
Comment 30•9 years ago
|
||
(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 :)
Comment 31•9 years ago
|
||
I'll file in a bit a new bug for investigation, with all the details.
Comment 32•9 years ago
|
||
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)
Comment 33•9 years ago
|
||
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.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•