Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

[Bug] Crash @kotlin.KotlinNullPointerException: at mozilla.components.service.fxa.manager.FxaAccountManager.postAuthenticated(FxaAccountManager.kt:36) #7536

Closed
grigoryk opened this issue Jun 26, 2020 · 7 comments · Fixed by #7856 or #7915

Comments

@grigoryk
Copy link
Contributor

grigoryk commented Jun 26, 2020

Filed from mozilla-mobile/fenix#12000

https://crash-stats.mozilla.org/signature/?product=Fenix&signature=kotlin.KotlinNullPointerException%3A%20at%20mozilla.components.service.fxa.manager.FxaAccountManager.postAuthenticated%28FxaAccountManager.kt%3A36%29

kotlin.KotlinNullPointerException
	at mozilla.components.service.fxa.manager.FxaAccountManager.postAuthenticated(FxaAccountManager.kt:36)
	at mozilla.components.service.fxa.manager.FxaAccountManager$postAuthenticated$1.invokeSuspend(FxaAccountManager.kt)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:2)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:19)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1115)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:590)
	at java.lang.Thread.run(Thread.java:818)

┆Issue is synchronized with this Jira Task

@grigoryk
Copy link
Contributor Author

The obvious thing that can NPE in that stacktrace (unfortunately line numbers are wrong) is fxaDeviceId = account.getCurrentDeviceId()!!.

@grigoryk grigoryk added 🐞 bug Something isn't working 💥 crash labels Jun 26, 2020
@grigoryk
Copy link
Contributor Author

getCurrentDeviceId ends up reading from local state in https://github.com/mozilla/application-services/blob/master/components/fxa-client/src/device.rs#L334-L339:

match self.state.current_device_id {
            Some(ref device_id) => Ok(device_id.to_string()),
            None => Err(ErrorKind::NoCurrentDeviceId.into()),
        }

self.state.current_device_id is set in two places - ensure_capabilities or initialize_device (in fxaclient's device.rs). In the most common scenario (just logging-in, either manually or via pairing) we call initialize_device just before going into postAuthenticated. But, that needs to talk to network, and can fail.
Currently, we assume that this call actually succeeded. If it didn't, we won't have the current_device_id set, and will blow up in postAuthenticated.

@pocmo
Copy link
Contributor

pocmo commented Jul 29, 2020

According to Google Play this is our top crash in 79.0.0.

@pocmo
Copy link
Contributor

pocmo commented Jul 29, 2020

The following Fenix issue has STRs:
mozilla-mobile/fenix#13064

bors bot pushed a commit that referenced this issue Jul 29, 2020
7915: Make sure device finalization succeeds before proceeding to postAuth r=csadilek a=grigoryk

Closes #7536 

Proper fix is coming in #7856, but this gets the crashes to go away.



Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
@bors bors bot closed this as completed in d23dd9d Jul 30, 2020
A-C: Android Components Sprint Planning automation moved this from 🏃‍♀️ In Progress to 🏁 Done Jul 30, 2020
Android: Crash Tracking automation moved this from Top issues to Closed Jul 30, 2020
@Diana-Rus
Copy link

Hi, issue is still reproducible with Samsung Galaxy S9 (Android 8) and Google Pixel 3XL (Android 9) on Firefox Release 79.0.1.

As long as you leave the "Approval now required" page, after a second or two and before on desktop is displayed "Firefox Account is connected with Firefox for Android."

Crash link - Samsung Galaxy S9 (Android 8): https://crash-stats.mozilla.org/report/index/25257b31-c0f6-49ec-9892-2b4df0200731
Crash link - google Pixel 3 XL (Android 9): https://crash-stats.mozilla.org/report/index/b4d6be88-4130-4bf7-96cb-e4b8d0200731

Same STR, exception is the timing in step 6:

  1. Open browser go to Settings/ Sign in with a QR code.
  2. Tap on the "Allow" button from the dialog box.
  3. On Firefox Nightly desktop go to firefox.com/pair and tap on the "Show code" button.
  4. Scan the provided QR code.
  5. Confirm pairing on both mobile and desktop.
  6. Leave "Approval now required" page by tapping on deivce back button, after 1 or 2 seconds.
  7. Go to Settings/Synced tabs.
  8. Wait a couple of seconds.

@grigoryk
Copy link
Contributor Author

Thanks, @Diana-Rus. These STRs are quite different now, and the stacktrace is different. Before we were dealing with error handling around network failures, and this is something else now. I've filed #7944 to investigate. Thanks for reporting :)

@grigoryk
Copy link
Contributor Author

@Diana-Rus landed a fix in #7944 - I'll close this issue for now.

We've cut 48.0.12 with this fix in it, which should be available in 79.0.2 fenix release that will be available later today.

Android: Crash Tracking automation moved this from Needs triage to Closed Jul 31, 2020
bors bot pushed a commit that referenced this issue Sep 22, 2020
7856: FxA Account Manager refactor r=csadilek a=grigoryk

### State machine changes:
- "profile" states have been removed, `profile` is now handled outside of the state machine
- states and events have been split into two subtypes: external and internal. External states are the `AccountState`s (authenticated, not authenticated, auth problem, etc) and internal states are `InternalState` (beginning auth, completing auth, recovering from auth problems, etc)
- this split should hopefully make navigating the state machine more intuitive
- retry logic and error handling was added for all internal transitions which require networking (Closes #7536)

### account manager changes:
- removed ability to "set sync config" from the public API. we weren't using it (and have no plans to use this), so the added complexity wasn't benefiting anything.

### async/suspend API changes:
account manager, account and device constellation APIs are now `suspend`, which makes both internal code and consuming of these APIs nicer and more flexible. Actual semantics of what's going on didn't change.

### sync manager changes:
introduced a delay to the periodic sync scheduling, so that we can properly manage what goes on during startup (Closes #7335)  

Tests are half-way being updating, so this is still a WIP.

cc @csadilek @rfk @eoger @jonalmeida 



Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Something isn't working 💥 crash <firefox-accounts> Component: FxA
Projects
No open projects
5 participants