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

Firefox loses session without explicit logout #2726

Closed
vext01 opened this issue Dec 9, 2016 · 18 comments

Comments

@vext01
Copy link

@vext01 vext01 commented Dec 9, 2016

Hi,

I cam to my computer today and found that my riot.im session had "expired", along with my encryption keys. I'm using firefox-50.0.2 on OpenBSD.

As I understand it, my session and keys should persist unless I explicitly log out.

As a side issue. Shouldn't it be possible (without a great deal of effort on the user's part) to log out, but keep your encryption keys?

Related:
#2068
#2108

Thanks

@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 9, 2016

(The session had been in daily use for a good week or two prior.)

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 9, 2016

It's not the first time we have had reports for this; they seem to affect Firefox more than Chrome.

vext01 reports that this does not happen every time he restarts the browser, and is generally not repeatable. This makes it unlikely that it's an issue with privacy settings in his browser, or an extension clearing the storage.

@vext01: was this a new riot tab, or had an existing tab logged itself out overnight?

Assuming the former, some other possibilities:

  • The user switched from vector.im to riot.im (or similar). Let's assume that this is not the case.
  • The user cleaned out history at some point in the previous browsing session, but he says he didn't.
  • Firefox decided to proactively purge localStorage. The spec permits it to do so, though afaict it doesn't.
  • The user navigated to the app with query-params which override the settings in localstorage, but this should only happen during the initial registration flow.
  • Some exception occurred when trying to reinstate the session, but I can't really think what.

Either way, if anyone sees this again, browser console logs would be very helpful.

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 9, 2016

vext01 reports that this was a new riot tab.

From the server logs, I see a request to set the user's presence, followed by three (!) typing notifications, followed by the start of the guest registration flow. All of the observed requests gave 200s.

This is very odd, because the only code which sends typing notifications is the MessageComposer, which shouldn't even exist until /sync completes - and we do not see a /sync request at all.

Nevertheless the fact that the client is using an existing access token confirms that the accesstoken was not vaped from the localstore.

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 9, 2016

Utterly bamboozled as to how the client can give the behaviour observed. Please try to get a console log if you see it again!

@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 10, 2016

OK, will do.

By console log, you mean? The browser's javascript console? or?

@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 11, 2016

I have more info.

I usually upgrade my OpenBSD snapshot every Saturday, which includes upgrading all of the third party packages. Is it possible that upgrading to a newer firefox could affect local storage?

On a related note today I have the following screen:

riot

But let me stress, that this is a somewhat exceptional circumstance. I installed a new SSD and:

  • installed the OS afresh, which may include a newer firefox.
  • copied my $HOME from my old spinning disk to the new SSD.

I copied with cp -rp, so as to preserve metadata:

     -p      Preserve in the copy as many of the modification time, access
             time, file flags, file mode, user ID, and group ID as allowed by
             permissions.

So the error you see in the screenshot may have been caused by either:

  • the copy
  • a newer package
  • a matrix/riot bug

I can't say which.

Can you give me an idea what the o.Account is not a constructor message means? Is that a language level bug in riot?

If the copy caused this, then that's my own fault of course.

Do you see anything in the server log? This was about 10am(ish) this morning.

@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 11, 2016

The OpenBSD firefox package was updated 10 days ago, from 50.0.1 to 50.0.2. Since I update once a week, and since there is a day or two lag before packages become available, it is entirely possible that I did get a new firefox when I installed the OS afresh.

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 11, 2016

@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 11, 2016

There is a stack trace. Is any of this sensitive?

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 11, 2016

Can you give me an idea what the o.Account is not a constructor message means? Is that a language level bug in riot?

Yeah, it is (obviously) an error you aren't meant to see. Specifically, it happens when trying to restore the cryptography keys for your device, and means that it couldn't find a function that was meant to exist.

The crypto code lives in a separate file called olm.js (because US export law), which complicates the matter here slightly. o.Account lives in olm.js so it's possible you've got a corrupt version of that module, somehow.

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 11, 2016

There is a stack trace. Is any of this sensitive?

Well, the console log will contain room ids/aliases, as well as userids of other members of those rooms: you may not want everybody to know who you hang out with and where.

Also while encryption is in development, we currently log some event keys to the console.

A stacktrace should be fine, but if you're in any doubt, PM me.

@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 11, 2016

I've PM'd you traces.

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 11, 2016

vext01's traces include the following:

Successfully compiled asm.js code (loaded from cache in 82ms)  olm.5c7db09b769f18dc076c.js
TypeError: asm.js link error: Unable to prepare ArrayBuffer for asm.js use  olm.5c7db09b769f18dc076c.js:4:19706
uncaught exception: out of memory  (unknown)

In other words, we get an exception when we try to allocate the fixed heap area for the Olm enscripten code; this will indeed dump you out to a login screen.

I think there are two things we can do here:

  • handle exceptions in this bit of code better. If we can't initialise the Olm library, we ought to fail back to an encryption-disabled variant, rather than logging you out
  • Currently we allocate 16M here. I'm pretty sure that's unnecessary, since we are very careful with our memory management. We should reduce this allocation so we're less likely to hit the problem in the first place.
@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 11, 2016

Also, when I woke my laptop up from suspend (so a different machine altogether, which had not had the re-install treatment) it too had kicked me off and was showing the same screen as above. So the whole reinstall/cp thing is a red herring.

@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 12, 2016

@richvdh let me know when the fix is live and I can test it for you.

richvdh added a commit that referenced this issue Dec 14, 2016
16M is somewhat excessive: configure olm to use a bit less.

Requires changes to the olm library to do anything useful, but will be harmless
without them.

Partial fix to #2726.
@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 15, 2016

https://riot.im/develop should now be better:

@richvdh richvdh closed this Dec 15, 2016
@vext01

This comment has been minimized.

Copy link
Author

@vext01 vext01 commented Dec 18, 2016

@richvdh

This comment has been minimized.

Copy link
Member

@richvdh richvdh commented Dec 23, 2016

This is now live on https://riot.im/app.

Half-Shot added a commit to Half-Shot/riot-web that referenced this issue Feb 9, 2017
16M is somewhat excessive: configure olm to use a bit less.

Requires changes to the olm library to do anything useful, but will be harmless
without them.

Partial fix to vector-im#2726.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.