Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

People section disappeared #3331

Closed
turt2live opened this issue Feb 27, 2017 · 17 comments · Fixed by matrix-org/matrix-react-sdk#829
Closed

People section disappeared #3331

turt2live opened this issue Feb 27, 2017 · 17 comments · Fixed by matrix-org/matrix-react-sdk#829
Assignees
Labels
A-Room-List P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Milestone

Comments

@turt2live
Copy link
Member

Description

A few days ago the people section disappeared in riot-web for me. Whatever went wrong propagated the de-tagging of the rooms to riot-android (therefore I'm under the impression that it got persisted). Re-tagging the room does not work on riot-web, and just throws an error:

Uncaught TypeError: Cannot read property 'guessDMRoomTarget' of undefined
    at Object._onClickDM (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:93300:49)
    at Object.ReactErrorUtils.invokeGuardedCallback (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:52068:17)
    at executeDispatch (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:51601:22)
    at Object.executeDispatchesInOrder (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:51624:6)
    at executeDispatchesAndRelease (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:27778:23)
    at executeDispatchesAndReleaseTopLevel (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:27789:11)
    at Array.forEach (native)
    at forEachAccumulated (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:86904:10)
    at Object.processEventQueue (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:27992:8)
    at runEventQueueInBatch (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:198903:19)
    at Object.handleTopLevel [as _handleTopLevel] (https://riot.im/develop/bundles/116734ce7f2e3ee27ac5/bundle.js:198914:6)

Steps to reproduce

  • Use /develop
  • Note that the people section is missing
  • Note that rooms cannot be re-tagged as 1:1

Describe how what happens differs from what you expected.
I would expect that my People list would be maintained

Log: not sent (can't)

Version information

  • Platform: web (in-browser)

For the web app:

  • Browser: Chrome 56
  • OS: Windows
  • URL: riot.im/develop
@ara4n
Copy link
Member

ara4n commented Feb 28, 2017

@kegsay please don't yell at me, but i'm wondering if the missing tags may be due to having failed to persist some sync data (a bit like the theming data wasn't being properly picked up)?

@ara4n ara4n added T-Defect X-Cannot-Reproduce P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround A-Room-List labels Feb 28, 2017
@kegsay
Copy link
Contributor

kegsay commented Feb 28, 2017

You asked this before on #riot-dev and I confirmed that it could very well be due to the same issue as the dark theme bug (that is, global account data would be restored without triggering event emitters). I don't know how the DM list gets saved (frequency, what triggers it, etc) to confirm that this would've meant that it would've saved an empty list or not.

Either way, this issue was fixed in matrix-org/matrix-js-sdk#377 - but I'm not confident in saying "yes, this was the cause of this bug" as I don't know how DM stuff works entirely.

@turt2live
Copy link
Member Author

I rolled back riot-js-sdk to the commit prior to matrix-org/matrix-js-sdk#377 and rolled riot-web and matrix-react-sdk to the commit just prior to the commit prior to the time that PR was merged. I wasn't able to get the people list to disappear.

Others have reported that the People list randomly disappears (as mentioned in #3355), however those reports haven't shown up recently (at least that I've seen).

For reference:

  • I haven't had the primary issue of the list disappearing since reporting this issue
  • The earlier build of Riot did not cause the room list to disappear (although my testing may have been flawed)
  • I don't have a more refined time span for when this issue actually occurred, so figuring out which commit(s) caused the regression (and possibly fixed it) are probably too difficult to find.
    • ... and client logs are long gone by now

I'll try to see if synapse has any interesting information, although I doubt it.

@turt2live
Copy link
Member Author

Synapse shows a curious-looking PUT request for account data at 5am local time on the 23rd of February. I normally keep Riot open overnight, but don't give it attention (because sleep). 5am would be out of character for me to be using Riot (let alone tagging rooms), and the client that invoked the request doesn't have logs that go back far enough. It was riot-web though running on riot.im/develop.

That client would have most likely been at the following commits at the time of the strange request:

Running a build of riot-web with those commit hashes still hasn't yielded the problem, however :(

@kegsay
Copy link
Contributor

kegsay commented Mar 6, 2017

Thanks for digging into this @turt2live - so it looks like:

PUT request for account data at 5am local time on the 23rd of February

is the reason why you lost the DMs given on 27 Feb you said:

A few days ago the people section disappeared

That gets set via the JS SDK call setAccountData which is:

$ grep -rn setAccountData ../matrix-react-sdk/src/
../matrix-react-sdk/src/components/views/rooms/RoomList.js:296:            MatrixClientPeg.get().setAccountData('m.direct', newMDirectEvent).done();
../matrix-react-sdk/src/UserSettingsStore.js:130:        return MatrixClientPeg.get().setAccountData("org.matrix.preview_urls", {
../matrix-react-sdk/src/UserSettingsStore.js:149:        return MatrixClientPeg.get().setAccountData("im.vector.web.settings", settings);
../matrix-react-sdk/src/Rooms.js:123:    return MatrixClientPeg.get().setAccountData('m.direct', dmRoomMap);

We're only interested in Rooms.js:setDMRoom and RoomList.js:getRoomLists. It looks like a whole bunch of random things will do that PUT (including getting invites from people, having a membership list with just 2 people (including self), having the RoomList load (!) if it is converting from "fake recents" to "fake direct", etc.

@richvdh richvdh changed the title People section disappeared and cannot be restored on /develop People section disappeared Mar 29, 2017
@richvdh
Copy link
Member

richvdh commented Mar 29, 2017

I believe the 'cannot be restored' aspect of this has been fixed (per #3355)

@richvdh
Copy link
Member

richvdh commented Mar 29, 2017

I just got bitten fairly spectacularly by this. Unfortunately I'm not quite sure what I did to provoke it. I think it happened when I opened https://riot.im/app on my account from an incognito tab, but obviously repeating that doesn't reproduce it :/

@richvdh
Copy link
Member

richvdh commented Mar 29, 2017

The API request in question appears to have been:

2017-03-29 17:43:44,633 - synapse.access.http.8008 - 59 - INFO - PUT-617679- 83.166.71.16 - 8008 - Received request: PUT /_matrix/client/r0/user/@richvdh:sw1v.org/account_data/m.direct?access_token=<redacted>

Correlating access tokens in the apache logs suggests that the request did indeed come from the /app instance I ran in an incognito tab. I guess there may have been something in the account data which made /app fail to parse it, and then it went and reset all the m.direct data?

@ara4n
Copy link
Member

ara4n commented Mar 29, 2017

sounds plausible :(

@lampholder lampholder modified the milestones: RW002, RW002 - candidates, RW003 - candidates Apr 3, 2017
@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Apr 3, 2017

As @kegsay points out, setAccountData is called in two places. Both make sure that the result from getAccountData('m.direct') is undefined before sending something new to the server. But there's no difference between "m.direct hasn't been received down sync yet" and "m.direct has never been set".

So the client appears able to reset the m.direct account data because it thinks it's never been set.

@lukebarnard1
Copy link
Contributor

The error in the original post was fixed by #3448, which corrected the Rooms import

@dbkr
Copy link
Member

dbkr commented Apr 10, 2017

I've just had this upgrading my electron client to the new version, which I worry may mean everyone on /app or the desktop version will hit this bug when they upgrade. Marking as release blocker until I can investigate.

@dbkr
Copy link
Member

dbkr commented Apr 11, 2017

I can't repro this again and think that my losing my m.direct event was due to #3610, so removing the release blocker tag

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Apr 21, 2017

I am now losing my people on a regular basis. Not entirely sure why, but I've been logging in and out in a myriad of environments, including incognito mode (which sounds like #3331 (comment)), so it could be a race of some flavour.

@lampholder lampholder modified the milestones: RW003 - candidates, RW002 Apr 21, 2017
@turt2live
Copy link
Member Author

turt2live commented Apr 21, 2017

I managed to catch this in the act while setting up a riot-web development environment. Full log is here as a gist.

Environment: Windows 10, Chrome 57, running at the following points in time:

This appears to be the fun section of the log:

src\vector\rageshake.js:63 Resetting room DM state to be {}
consoleObj.(anonymous function) @ src\vector\rageshake.js:63
getRoomLists @ \..\..\src\components\views\rooms\RoomList.js:420
RoomList_componentWillMount @ \..\..\src\components\views\rooms\RoomList.js:76
(anonymous) @ ~\react-dom\lib\ReactCompositeComponent.js:348
measureLifeCyclePerf @ ~\react-dom\lib\ReactCompositeComponent.js:75
performInitialMount @ ~\react-dom\lib\ReactCompositeComponent.js:347
mountComponent @ ~\react-dom\lib\ReactCompositeComponent.js:258
mountComponent @ ~\react-dom\lib\ReactReconciler.js:46
mountChildren @ ~\react-dom\lib\ReactMultiChild.js:238
_createInitialChildren @ ~\react-dom\lib\ReactDOMComponent.js:697
mountComponent @ ~\react-dom\lib\ReactDOMComponent.js:516
mountComponent @ ~\react-dom\lib\ReactReconciler.js:46
performInitialMount @ ~\react-dom\lib\ReactCompositeComponent.js:371
mountComponent @ ~\react-dom\lib\ReactCompositeComponent.js:258
mountComponent @ ~\react-dom\lib\ReactReconciler.js:46
performInitialMount @ ~\react-dom\lib\ReactCompositeComponent.js:371
mountComponent @ ~\react-dom\lib\ReactCompositeComponent.js:258
mountComponent @ ~\react-dom\lib\ReactReconciler.js:46
mountChildren @ ~\react-dom\lib\ReactMultiChild.js:238
_createInitialChildren @ ~\react-dom\lib\ReactDOMComponent.js:697
mountComponent @ ~\react-dom\lib\ReactDOMComponent.js:516
mountComponent @ ~\react-dom\lib\ReactReconciler.js:46
mountChildren @ ~\react-dom\lib\ReactMultiChild.js:238
_createInitialChildren @ ~\react-dom\lib\ReactDOMComponent.js:697
mountComponent @ ~\react-dom\lib\ReactDOMComponent.js:516
mountComponent @ ~\react-dom\lib\ReactReconciler.js:46
performInitialMount @ ~\react-dom\lib\ReactCompositeComponent.js:371
mountComponent @ ~\react-dom\lib\ReactCompositeComponent.js:258
mountComponent @ ~\react-dom\lib\ReactReconciler.js:46
_updateRenderedComponent @ ~\react-dom\lib\ReactCompositeComponent.js:765
_performComponentUpdate @ ~\react-dom\lib\ReactCompositeComponent.js:724
updateComponent @ ~\react-dom\lib\ReactCompositeComponent.js:645
performUpdateIfNecessary @ ~\react-dom\lib\ReactCompositeComponent.js:561
performUpdateIfNecessary @ ~\react-dom\lib\ReactReconciler.js:157
runBatchedUpdates @ ~\react-dom\lib\ReactUpdates.js:150
perform @ ~\react-dom\lib\Transaction.js:140
perform @ ~\react-dom\lib\Transaction.js:140
perform @ ~\react-dom\lib\ReactUpdates.js:89
flushBatchedUpdates @ ~\react-dom\lib\ReactUpdates.js:172
closeAll @ ~\react-dom\lib\Transaction.js:206
perform @ ~\react-dom\lib\Transaction.js:153
batchedUpdates @ ~\react-dom\lib\ReactDefaultBatchingStrategy.js:62
enqueueUpdate @ ~\react-dom\lib\ReactUpdates.js:200
enqueueUpdate @ ~\react-dom\lib\ReactUpdateQueue.js:24
enqueueSetState @ ~\react-dom\lib\ReactUpdateQueue.js:209
ReactComponent.setState @ ~\react\lib\ReactComponent.js:63
_onLoggedIn @ \..\..\src\components\structures\MatrixChat.js:759
onAction @ \..\..\src\components\structures\MatrixChat.js:572
_invokeCallback @ ~\matrix-react-sdk\~\flux\lib\Dispatcher.js:198
dispatch @ ~\matrix-react-sdk\~\flux\lib\Dispatcher.js:174

Edit: Some more information as to what happened.

I noticed my people section disappeared before my rooms/messages/account data loaded. I use the dark theme, and the riot I was logging in to was noticeably not using the dark theme, was not listing my rooms, and clearly had not loaded my account data yet (opening the settings proved this). The sync took a while to complete.

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Apr 21, 2017

@turt2live thanks for this.

@lampholder just experienced this exact failure mode. The fix will be to prevent the RoomList from sending an update to the dm room map if it hasn't already received account data. We currently rely on the sync spinner from hiding the RoomList, and apparently the spinner has stopped appearing following a login (sometimes).

Todo:

  • Don't send m.direct data if the client hasn't already received account data from the server
  • Investigate why the spinner sometimes doesn't appear during initial sync

(See matrix-org/matrix-react-sdk#829)

@lukebarnard1
Copy link
Contributor

Although the problem is likely fixed now, we still need a button to initiate the guessing of DMs. This shall be tracked here #3803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Room-List P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants