Fix error handling on session restore #1859
Conversation
Fix a number of failures that meant the excellent error handling we had for failing to restore a session didn't work. 1. .catch on the promise rather than try/catch: it's async 2. Explicit cancel method in SessionRestoreErrorDialog that invokes onFinished with `false` because even with the catch fixed, this was getting the event as its first arg which is truthy, so clicking cancel still deleted your data. 3. DialogButtons: Don't pass onCancel straight into the button event handler as this leaks the MouseEvent through as an argument. Nothing is using it and it exacerbates failures like this because there are surprise arguments. Fixes element-hq/element-web#6616
|
Actually sorry, this error handling should be moved up a level or two as there are classes of errors that still dump you at a login screen. |
Everywhere else, onFinished takes a boolean indicating whether the dialog was confirmed on cancelled, and had function that were expecting this variable and getting undefined.
The user might (probably does) have a session even if we haven't actually tried to load it yet, so wrap the whole loadSession code in the error handler we were using for restoring sessions so we gracefully handle exceptions that happen before trying to restore sessions too. Remove the catch in MatrixChat that sent you to the login screen. This is never the right way to handle an error condition: we should only display the login screen if we successfully determined that the user has no session, or they explicitly chose to blow their sessions away.
|
OK, ready again now. |
|
Tests appear to be legitimately failing |
|
Hurrah for legitimate test failures! |
|
Apparently eslint is sad. |
|
right, actually green & ticky now |
src/Lifecycle.js
Outdated
| guest: true, | ||
| }, true).then(() => true); | ||
| } | ||
| return Promise.resolve().then(() => { |
There was a problem hiding this comment.
This is quite difficult to read, any chance of making loadSession async?
There was a problem hiding this comment.
Yeah, probably better. Done, plus the restore function.
| homeserverUrl: guestHsUrl, | ||
| identityServerUrl: guestIsUrl, | ||
| guest: true, | ||
| }, true).then(() => true); |
There was a problem hiding this comment.
I'm wondering whether we should really be returning true/false instead of throwing exceptions. Do you have thoughts on this @dbkr?
There was a problem hiding this comment.
I think the logic here is: true == logged in, false == there was no session, exception == it's all screwed, so they have distinct useful meanings which are now hopefully clearer in async / await form rather than promise chains.
src/Lifecycle.js
Outdated
| // legacy key name | ||
| isGuest = localStorage.getItem("matrix-is-guest") === "true"; | ||
| } | ||
| return Promise.resolve().then(() => { |
There was a problem hiding this comment.
I'm finding it quite strange that the whole thing is wrapped in a resolved promise. Should this be an async function?
| }).then((loadedSession) => { | ||
| if (!loadedSession) { | ||
| // fall back to showing the login screen | ||
| dis.dispatch({action: "start_login"}); | ||
| } | ||
| }); | ||
| // Note we don't catch errors from this: we catch everything within | ||
| // loadSession as there's logic there to ask the user if they want | ||
| // to try logging out. | ||
| }).done(); |
There was a problem hiding this comment.
I don't think we need this?
| @@ -36,6 +36,9 @@ export default React.createClass({ | |||
|
|
|||
| propTypes: { | |||
| // onFinished callback to call when Escape is pressed | |||
| // Take a boolean which is true is the dialog was dismissed | |||
| @@ -36,6 +36,9 @@ export default React.createClass({ | |||
|
|
|||
| propTypes: { | |||
| // onFinished callback to call when Escape is pressed | |||
There was a problem hiding this comment.
Is this now false? What about all the dialogs that depend on it being true?
There was a problem hiding this comment.
They were all either taking no args or relying on cancel == called with false
| @@ -36,6 +36,9 @@ export default React.createClass({ | |||
|
|
|||
| propTypes: { | |||
| // onFinished callback to call when Escape is pressed | |||
| // Take a boolean which is true is the dialog was dismissed | |||
| // with a positive / confirm action or false if it was | |||
| // cancelled (from BaseDialog, this is always false). | |||
There was a problem hiding this comment.
Also, onFinished is never called with a truthy value, so the next comment is not true.
There was a problem hiding this comment.
As in the 'this is always false' comment? Yeah, that was what I meant. Have tried to be clearer.
There was a problem hiding this comment.
Oh, well if the BaseDialog has no way to provide an affirmative response, this makes sense. Thanks for clarifying.
Fix a number of failures that meant the excellent error handling
we had for failing to restore a session didn't work.
onFinished with
falsebecause even with the catch fixed, thiswas getting the event as its first arg which is truthy, so
clicking cancel still deleted your data.
handler as this leaks the MouseEvent through as an argument.
Nothing is using it and it exacerbates failures like this
because there are surprise arguments.
Lots of re-indenting has gone on too: reading commits individually may be useful.
Fixes element-hq/element-web#6616