-
Notifications
You must be signed in to change notification settings - Fork 984
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
add empty check for keychain credentials server #19242
Conversation
Jenkins BuildsClick to see older builds (4)
|
85% of end-end tests have passed
Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't reproduce it either. Let's hope this is the culprit
77% of end-end tests have passed
Failed tests (10)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (37)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
25abf4b
to
6172957
Compare
100% of end-end tests have passed
Passed tests (10)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
|
Hi @Parveshdhull ! Thanks for analysing this tricky issue! In your PR I've tested both platforms using such logout scenarios:
I hope your fix is success! 🤞 |
6172957
to
7fa12ab
Compare
fixes #19239
Summary
status-mobile/src/status_im/common/keychain/events.cljs
Lines 64 to 66 in 614b03d
We have empty check for save-auth-method, but
get-credentials
,reset-credentials
etc. are missing this empty check. So if for some reason key-uid is nil, it will cause crash.Testing notes
Please test all account types
(this one is most suspicious, maybe also check in develop if this one is causing the bug)
Note: I am not able to reproduce this issue, but most likely cause mentioned in Summary is culprit. As these are only effects with string/lower-case in logout method.
status: ready