Ending a call while tab sharing is active results in an error and no listeners removed

RESOLVED FIXED in Firefox 38

Status

Hello (Loop)
Client
P1
blocker
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: standard8)

Tracking

unspecified
mozilla39
Points:
3
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox38+ fixed, firefox39 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
So when I click the red 'Hang Up' button to end a conversation with tab sharing turned on, I currently get the following thrown in the browser console:

"hawkRequest: /rooms/YKip4lFG-Bg" 1 MozLoopService.jsm:639
"promiseRegisteredWithServers: registration already completed or in progress:" 1 MozLoopService.jsm:385
"[Dispatcher] Dispatching action caused an exception: " Error: Invariant Violation: replaceState(...): Cannot update during an existing state transition (such as within `render`). Render methods should be a pure function of props and state.
Stack trace:
[140]</invariant@chrome://browser/content/loop/shared/libs/react-0.12.2.js:18683:15
validateLifeCycleOnReplaceState@chrome://browser/content/loop/shared/libs/react-0.12.2.js:6225:37
[40]</ReactCompositeComponentMixin.replaceState@chrome://browser/content/loop/shared/libs/react-0.12.2.js:6663:5
[40]</ReactCompositeComponentMixin.setState@chrome://browser/content/loop/shared/libs/react-0.12.2.js:6644:1
ActiveRoomStoreMixin._onActiveRoomStateChanged@chrome://browser/content/loop/js/roomViews.js:45:9
triggerEvents@chrome://browser/content/loop/shared/libs/backbone-1.1.2.js:206:32
Backbone.Events.trigger@chrome://browser/content/loop/shared/libs/backbone-1.1.2.js:148:19
baseStorePrototype.setStoreState@chrome://browser/content/loop/shared/js/store.js:54:9
loop.store.RoomStore<._onActiveRoomStoreChange@chrome://browser/content/loop/shared/js/roomStore.js:147:7
triggerEvents@chrome://browser/content/loop/shared/libs/backbone-1.1.2.js:206:32
Backbone.Events.trigger@chrome://browser/content/loop/shared/libs/backbone-1.1.2.js:148:19
baseStorePrototype.setStoreState@chrome://browser/content/loop/shared/js/store.js:56:7
ActiveRoomStore<._leaveRoom@chrome://browser/content/loop/shared/js/activeRoomStore.js:580:7
ActiveRoomStore<.windowUnload@chrome://browser/content/loop/shared/js/activeRoomStore.js:498:7
Dispatcher.prototype._dispatchNextAction/<@chrome://browser/content/loop/shared/js/dispatcher.js:76:11
Dispatcher.prototype._dispatchNextAction@chrome://browser/content/loop/shared/js/dispatcher.js:74:7
Dispatcher.prototype.dispatch@chrome://browser/content/loop/shared/js/dispatcher.js:48:7
init/<@chrome://browser/content/loop/js/conversation.js:181:7
DesktopRoomConversationView<.closeWindow@chrome://browser/content/loop/js/roomViews.js:214:7
DesktopRoomConversationView<.render@chrome://browser/content/loop/js/roomViews.js:265:11
[40]</ReactCompositeComponentMixin._renderValidatedComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:7030:29
[75]</ReactPerf.measure/wrapper@chrome://browser/content/loop/shared/libs/react-0.12.2.js:12914:16
[40]</ReactCompositeComponentMixin.updateComponent<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:6957:25
[75]</ReactPerf.measure/wrapper@chrome://browser/content/loop/shared/libs/react-0.12.2.js:12914:16
[40]</ReactCompositeComponentMixin._performComponentUpdate@chrome://browser/content/loop/shared/libs/react-0.12.2.js:6901:1
[40]</ReactCompositeComponentMixin.performUpdateIfNecessary@chrome://browser/content/loop/shared/libs/react-0.12.2.js:6844:7
runBatchedUpdates@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15142:7
[107]</Mixin.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:16876:13
[107]</Mixin.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:16876:13
[91]</<.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15088:1
[91]</flushBatchedUpdates<@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15167:9
[75]</ReactPerf.measure/wrapper@chrome://browser/content/loop/shared/libs/react-0.12.2.js:12914:16
[107]</Mixin.closeAll@chrome://browser/content/loop/shared/libs/react-0.12.2.js:16949:1
[107]</Mixin.perform@chrome://browser/content/loop/shared/libs/react-0.12.2.js:16890:11
[54]</ReactDefaultBatchingStrategy.batchedUpdates@chrome://browser/content/loop/shared/libs/react-0.12.2.js:9142:7
batchedUpdates@chrome://browser/content/loop/shared/libs/react-0.12.2.js:15103:3
[63]</ReactEventListener.dispatchEvent@chrome://browser/content/loop/shared/libs/react-0.12.2.js:10598:7
 dispatcher.js:78:10

"hawkRequestInternal: " 1 "/rooms/YKip4lFG-Bg" "POST" MozLoopService.jsm:551
The connection to wss://media001-ams.tokbox.com/rumorwebsocketsv2 was interrupted while the page was loading. sdk.js:7539:0
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://hlg.tokbox.com/prod/logging/ClientEvent?_=e8b517a8-b681-4822-87ec-20e346c428b2. (Reason: CORS request failed). <unknown>
"PushSocket: Message received: " "{"messageType":"notification","updates":[{"channelID":"19d3f799-a8f3-4328-9822-b7cd02765832","version":1425487060}]}" MozLoopPushHandler.jsm:118
"PushHandler: ping timeout restart" MozLoopPushHandler.jsm:297
"PushHandler: notification: version = " 1425487060 ", channelID = " "19d3f799-a8f3-4328-9822-b7cd02765832" MozLoopPushHandler.jsm:684
"hawkRequest: /rooms?version=1425487060" 1 MozLoopService.jsm:639
"promiseRegisteredWithServers: registration already completed or in progress:" 1 MozLoopService.jsm:385
"PushHandler: PusherServer 'ack': " Array [ Object ] MozLoopPushHandler.jsm:695
"PushSocket: Message sent: " "{"messageType":"ack","updates":[{"channelID":"19d3f799-a8f3-4328-9822-b7cd02765832","version":1425487060}]}" MozLoopPushHandler.jsm:179
"hawkRequestInternal: " 1 "/rooms?version=1425487060" "GET" MozLoopService.jsm:551
"notifyStatusChanged with reason:" "room-update" MozLoopService.jsm:244
"on: callback function was lost." MozLoopAPI.jsm:154
"on error:" "update:YKip4lFG-Bg" MozLoopAPI.jsm:157
"on: callback function was lost." MozLoopAPI.jsm:154
"on error:" "update:YKip4lFG-Bg" MozLoopAPI.jsm:157
"on: callback function was lost." MozLoopAPI.jsm:154
"on error:" "update:YKip4lFG-Bg" MozLoopAPI.jsm:157
couldn't look up addon: fx-devtools nsBrowserGlue.js:571:0

It looks like we're doing something wrong according to React's logic.

Mark, can you take a look at this?
Flags: qe-verify-
Flags: needinfo?(standard8)
Flags: firefox-backlog+
(Assignee)

Comment 1

3 years ago
I'm working on this. Its an issue highlighted by the new dispatcher code which has try/catch in there.

The issue is that:

- the hangup button dispatches an action `LeaveRoom`
- the action updates the activeRoomStore state for roomState to ROOM_STATES.ENDED
- the DesktopRoomConversationView then attempts to render the view for the ENDED state which does window.close()
- window.close() is caught in an unload handle and dispatches an action `windowUnload`
- that action then tries to update the activeRoomStore state for roomState to ROOM_STATES.CLOSING

=> at this point React complains because we're modifying the state whilst we're rendering.

This obviously shouldn't be happening.

The view is decided if to close the window or display the feedback view and I think that's wrong - we should leave the ENDED state for the feedback view.

I think we do need the hangup button to determine if the room has been used or not, i.e. whether or not we should display feedback or just close the window. We could do the decision making in the store, but then the store would have to close the window (or risk another issue like this).
Assignee: nobody → standard8
Flags: needinfo?(standard8)
(Reporter)

Comment 2

3 years ago
[Tracking Requested - why for this release]: Loop/ Hello window & tab sharing is slated for Fx38, this requesting to track this bug for uplift when it's resolved.
Status: NEW → ASSIGNED
status-firefox38: --- → affected
tracking-firefox38: --- → ?
(Assignee)

Comment 3

3 years ago
Created attachment 8573456 [details] [diff] [review]
Fix an issue with trying to update the Loop desktop room view's state whilst already rendering; This could cause items like tab sharing to still look like they were active even though they weren't.

This fixes the issue that I described and how I expected to fix it in comment 1.

Note that there's multiple versions of the same test for different actions - I'm testing at the api level, and because the code around cleaning up rooms is important, I'm purposely being more explicit that perhaps necessary (the rest of the tests for those actions are the same as well).
Attachment #8573456 - Flags: review?(jaws)
Attachment #8573456 - Flags: review?(jaws) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/f4374981db86
Target Milestone: --- → mozilla39
https://hg.mozilla.org/mozilla-central/rev/f4374981db86
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
tracking-firefox38: ? → +
Mark, can we have an uplift request to aurora? Thanks
Flags: needinfo?(standard8)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8573456 [details] [diff] [review]
Fix an issue with trying to update the Loop desktop room view's state whilst already rendering; This could cause items like tab sharing to still look like they were active even though they weren't.

Approval Request Comment
[Feature/regressing bug #]: Loop's Conversations (aka rooms) - noticed as part of tab sharing
[User impact if declined]: Tab sharing could look like it is still active when it isn't.
[Describe test coverage new/current, TreeHerder]: Has unit tests, landed in m-c
[Risks and why]: Low, fixes a UX issue for users.
[String/UUID change made/needed]: None
Flags: needinfo?(standard8)
Attachment #8573456 - Flags: approval-mozilla-aurora?
Attachment #8573456 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 8

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8089ba15c56
status-firefox38: affected → fixed
You need to log in before you can comment on or make changes to this bug.