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

[Push] Messages received after subscription is removed throws a StorageError #2282

Closed
jonalmeida opened this issue Nov 26, 2019 · 7 comments · Fixed by #2355
Closed

[Push] Messages received after subscription is removed throws a StorageError #2282

jonalmeida opened this issue Nov 26, 2019 · 7 comments · Fixed by #2355

Comments

@jonalmeida
Copy link
Collaborator

jonalmeida commented Nov 26, 2019

See mozilla-mobile/fenix#6663 (comment).

When signing out on a device, we invoke unsubscribe for push immediately to to destroy that subscription. On Fenix, we're receiving a push message after the unsubscribe, so we fail at decrypting the push message.

The StorageError seems a bit too broad of an error type to catch (in case we end up hiding other bugs).

Possible solutions:

  • FxA should filter out sending the message the client that's disconnecting (we do something similar in the a-c FxaDeviceConstellation).
  • Use a different error type for clients to filter out as non-fatal.

┆Issue is synchronized with this Jira Story

@rfk
Copy link
Contributor

rfk commented Nov 27, 2019

FxA should filter out sending the message the client that's disconnecting
(we do something similar in the a-c FxaDeviceConstellation).

I believe this is done because the device could be disconnected remotely (e.g. from the FxA settings page on the web) and so we want to immediately tell that device that its sessionToken has been revoked, allowing it to transition to some sort of "reconnect" state straight away.

We could consider only notifying the device if it was disconnected using some other sessionToken but having a separate "not found" error of some kind seems more robust to me.

@jdragojevic
Copy link
Contributor

@jonalmeida what is the urgency on this?

@jonalmeida
Copy link
Collaborator Author

@jonalmeida what is the urgency on this?

This race-condition affects users that sign out of FxA on a device and causes their devices to crash. See the linked Fenix ticket for STRs.

@rfk
Copy link
Contributor

rfk commented Dec 3, 2019

Would a fix here also make the migration-related crash in mozilla-mobile/android-components#5041 less crashy?

@jonalmeida
Copy link
Collaborator Author

@rfk I incorrectly thought those two were related. The migration crash is related to missing a-c support for subscriptions that are not GV/FxA (our initial implementation for Send Tab let us ignore that implementation for the MVP).

With 0.44.0 of a-s we can add that support now.

@grigoryk
Copy link
Contributor

grigoryk commented Dec 4, 2019

Regardless of what we receive from the autopush servers, clients should not crash. Failing to decrypt a message is something we can swallow and march on. We can report caught exception to Sentry/Soccoro without crashing.

Ideally, we'll only report cases which aren't "we just disconnected and, as expected, received a message we couldn't decrypt" - those are just noise.

@jonalmeida let's make sure that even without a server-side fix, we handle this gracefully in Fenix/a-c.

@jonalmeida
Copy link
Collaborator Author

Ideally, we'll only report cases which aren't "we just disconnected and, as expected, received a message we couldn't decrypt" - those are just noise.

Yes, that's what this bug should only be covering.

Push Component Build automation moved this from To do to Done Dec 6, 2019
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jan 17, 2020
bors bot pushed a commit to mozilla-mobile/android-components that referenced this issue Jan 20, 2020
5629: Closes #5627: Catch RecordNotFoundError exception in Push r=grigoryk a=jonalmeida

This is the a-c fix for
mozilla/application-services#2282.




Co-authored-by: Jonathan Almeida <jalmeida@mozilla.com>
pocmo pushed a commit to mozilla-mobile/android-components that referenced this issue Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

4 participants