Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

867 - Fix obvious NPE sync crash #884

Merged
merged 1 commit into from
Aug 22, 2019
Merged

867 - Fix obvious NPE sync crash #884

merged 1 commit into from
Aug 22, 2019

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Aug 21, 2019 •
edited
Loading

Fixes #867

This PR fixes an obvious dereference of a null pointer.

We have been unable to reproduce the original crash, but we need to fix this soon.

@jhugman jhugman requested a review from a team as a code owner August 21, 2019 12:02
@eliserichards
Copy link
Contributor

eliserichards commented Aug 21, 2019 •
edited
Loading

The sync support tokens are no longer valid after a short period of time (an hour, maybe?). When talking to the sync team, we should be resetting syncConfig every time we sync (assuming that it is no longer valid). From that conversation I would assume that the SyncUnlockInfo can never be null.

Are we assuming that this is a bug on their end? Or possibly in our resetSupport()?

EDIT (post-discussion with James): our resetSupport() is not actually doing what was thought -- it's resetting the local backend. The syncConfig should not be nullable, and is most likely causing this crash when the data flow order gets messed up and sync hasn't been called yet.

eliserichards
eliserichards previously requested changes Aug 21, 2019
View reviewed changes
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see some tests for this 😉 Also, could we consider changing syncConfig in FxASyncDataStoreSupport to be lateinit or something (not nullable)?

app/src/main/java/mozilla/lockbox/store/DataStore.kt Show resolved Hide resolved
@jhugman jhugman force-pushed the jhugman/867-fixup-npe-sync-crash branch from a5b0800 to af868ff Compare August 21, 2019 15:47
@jhugman jhugman mentioned this pull request Aug 21, 2019
Open
@jhugman jhugman requested a review from eliserichards August 21, 2019 16:06
@jhugman
Copy link
Contributor Author

jhugman commented Aug 21, 2019

Added tests, and filed followup.

eliserichards
eliserichards reviewed Aug 21, 2019
View reviewed changes
Copy link
Contributor

@eliserichards eliserichards left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending manual tests and lint this LGTM!

app/src/main/java/mozilla/lockbox/store/DataStore.kt
@@ -235,13 +237,21 @@ open class DataStore(
}
}

private fun sync() {
@VisibleForTesting(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic!

app/src/main/java/mozilla/lockbox/store/DataStore.kt Show resolved Hide resolved
@jhugman jhugman force-pushed the jhugman/867-fixup-npe-sync-crash branch from af868ff to 237bd05 Compare August 21, 2019 16:40
@jhugman jhugman dismissed eliserichards’s stale review August 21, 2019 17:53

Addressed comments

@jhugman jhugman requested a review from eliserichards August 21, 2019 17:53
eliserichards
eliserichards approved these changes Aug 21, 2019
View reviewed changes
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@jhugman jhugman merged commit 4a3226d into master Aug 22, 2019
@jhugman jhugman deleted the jhugman/867-fixup-npe-sync-crash branch August 22, 2019 13:01
jhugman added a commit that referenced this pull request Aug 22, 2019
@jhugman
867 - Fix obvious NPE sync crash (#884)
e4b3ea6
@jhugman
Copy link
Contributor Author

jhugman commented Aug 27, 2019

Update this is rolled out to 20% of users over the weekend. No obvious side effects.

Proposal: roll out to 100% of users from today.

eliserichards pushed a commit that referenced this pull request Sep 11, 2019
Release v1.1.4 (#917)
074c269 
* Add release notes for 1.1.2.4405

* Bump version to 1.2.0

* Add ignore to the flaky test. This needs to be fixed later. (#793)

* 626, 627: Add kebab menu for delete  (#764)

* Add kebab menu icon and strings. Set up fragment for addition to toolbar.

* Create fragment and dialog.

* Add confirmation dialog, actions, and presenters

* Routing

* Ensure item is actually deleted and item list is refreshed before navigating back.

* Refactor and remove edit for now

* Keeping edit button, just not doing any functionality with it

* Test for item detail presenter

* Ellipsize long item titles in detail view

* Delete confirmation toast

* Delete toast and tests

* Consumable toast notification for delete

* Specify return type on Optional extension

* ItemListPresenterTest needed a consumable for the delete toast test

* Cleanup

* Fix toast notifications

* Import strings from android-l10n (#809)

State: mozilla-l10n/android-l10n@63a5240

* 806-uitest-add-delete (#808)

* Adjust spinner dropdown size to fit longer words (l10n) (#812)

* Ellipsize text in item list (#811)

* 810: Reset support when syncing (#826)

* Reset support when info is null

* Reset support every time we sync.

* Removing timeout to be fixed in #791 (#833)

* Removing timeout to be fixed in #791

* Remove strings

* 828-l10nScreenshotsTest-delete (#829)

* Add dot to sentence (#837)

* 814: Telemetry for sync (#835)

* Add telemetry actions for sync, list update, and error states.

* Sync timeout instead of sync end

* Add sync complete event

* Add errors and ids to telemetry calls

* Update metrics docs to reflect telemetry sync changes (#841)

* Revert delete PRs (#836)

* Revert "828-l10nScreenshotsTest-delete (#829)"

This reverts commit bab9fe8.

* Revert "806-uitest-add-delete (#808)"

This reverts commit ec56009.

* Revert "626, 627: Add kebab menu for delete  (#764)"

This reverts commit 17605c3.

* Remove unused imports and fix lint error.

* Rebase and lint

* Update to version 1.1.3 (#844)

* Update version to 1.1.3

* Add release notes for 1.1.3

* Update releases.md (#847)

* Add more context to release notes

* Add descriptions for proguard rules upload.

* Revert "Revert delete PRs (#836)" (#842)

This reverts commit 974a585.

* 816: string updates for localization (#852)

* Revert "Revert delete PRs (#836)"

This reverts commit 974a585.

* Lockwise placeholder in delete_description and descriptive comment in edit string

* Update app services, android components, and megazord configuration (#865)

* Update app services, android components, and megazord configuration

* Update a-c to v7.0.0

* Send hashed UID ping when sync completes successfully.

* Remove handling for synctelemetryping, to be completed in separate issue. Update AS to 0.37.1

* Update Readme with l10n process (#872)

This is something we've been adding to all Readmes of mobile repos to direct people to Pontoon for localization (vs GitHub repos, where work isn't supposed to happen).

* 867 - Fix obvious NPE sync crash (#884)

* 871: Create infrastructure for feature flags (#882)

* Delete feature flag

* Move feature flags to separate support file

* Create support to check build configs. Create flags for CUD

* Add telemetry probes to syncIfRequired trigger syncs (#887)

* Import strings from l10n (#893)

* Import strings from android-l10n

State: mozilla-l10n/android-l10n@349efdb

* Commenting unused string for Espanol

* 791: Remove fake timeout from sync (#889)

* Remove fake timeout from sync

* Remove all sync timeout strings

* Rebase and lint

* Fail fast if the edge between to routes is not in the nav graph. (#895)

* Auto-signin with mozilla AuthProvider (#879) — behind a feature flag.

* WIP Automatic FxA sign-in support

* Cleanup welcome fragment/account detection

* Upgrade to Android Components 9.0.0

* Ensure landable, even if not releasable

* gradlew lint

* 628: edit view and routing (#843)

* Edit xml layout

* Add route, presenter, and fragment

Button clicks and dialog

Update dependencies

Edit presenter tests

Routing stuff

Usable state with spinner

Buttons working

Password visibility in edit mode

Lint

View

Create popup item and insert menu into detail view's kebab buttton

* Popup menu and click listener

Dropdown menu and formatting

Update list - in progress

DataStoreTest - mocked up test, stuck on init

Datastore update item detail test

* Save edited changes

Lint

Update dependencies

Edit unit tests

Add sync to datastore editing. Update var name in robot and screenshot tests

Reformatting dropdown menu and edit view

Adding inclusive popup to edit->detail view nav definition

Address nullable server passwords for delete and edit. Remove unused string.

Remove learn more clicks from test

Refactoring pushError into a helper method.

* Add fxalogin to autofill onboarding route. (#901)

* 798: routing back to itemlist on login/search/feedback forms (#890)

* Investigation

* Add locking routes.

* Explicitly ignore autofill on search and edit fields

* Ignore autofill on webviews

* Remove comments and clean up lint

* Add null text value for empty usernames (#902)

* Add null text value for empty usernames

* gradlew lint

* 907 fix ui tests (#908)

* 907: fix testSettings

* fixing routing issues

* fixing more routes

* changing button name and adding route

* fixing conflicts after rebase

* Update release-notes.md with v1.1.4

* Fix lint errors

* Update release-notes.md to correct bitrise version
eliserichards pushed a commit that referenced this pull request Sep 12, 2019
Release v2.0.0 (#918)
ac23112 
* Add release notes for 1.1.2.4405

* Bump version to 1.2.0

* Add ignore to the flaky test. This needs to be fixed later. (#793)

* 626, 627: Add kebab menu for delete  (#764)

* Add kebab menu icon and strings. Set up fragment for addition to toolbar.

* Create fragment and dialog.

* Add confirmation dialog, actions, and presenters

* Routing

* Ensure item is actually deleted and item list is refreshed before navigating back.

* Refactor and remove edit for now

* Keeping edit button, just not doing any functionality with it

* Test for item detail presenter

* Ellipsize long item titles in detail view

* Delete confirmation toast

* Delete toast and tests

* Consumable toast notification for delete

* Specify return type on Optional extension

* ItemListPresenterTest needed a consumable for the delete toast test

* Cleanup

* Fix toast notifications

* Import strings from android-l10n (#809)

State: mozilla-l10n/android-l10n@63a5240

* 806-uitest-add-delete (#808)

* Adjust spinner dropdown size to fit longer words (l10n) (#812)

* Ellipsize text in item list (#811)

* 810: Reset support when syncing (#826)

* Reset support when info is null

* Reset support every time we sync.

* Removing timeout to be fixed in #791 (#833)

* Removing timeout to be fixed in #791

* Remove strings

* 828-l10nScreenshotsTest-delete (#829)

* Add dot to sentence (#837)

* 814: Telemetry for sync (#835)

* Add telemetry actions for sync, list update, and error states.

* Sync timeout instead of sync end

* Add sync complete event

* Add errors and ids to telemetry calls

* Update metrics docs to reflect telemetry sync changes (#841)

* Revert delete PRs (#836)

* Revert "828-l10nScreenshotsTest-delete (#829)"

This reverts commit bab9fe8.

* Revert "806-uitest-add-delete (#808)"

This reverts commit ec56009.

* Revert "626, 627: Add kebab menu for delete  (#764)"

This reverts commit 17605c3.

* Remove unused imports and fix lint error.

* Rebase and lint

* Update to version 1.1.3 (#844)

* Update version to 1.1.3

* Add release notes for 1.1.3

* Update releases.md (#847)

* Add more context to release notes

* Add descriptions for proguard rules upload.

* Revert "Revert delete PRs (#836)" (#842)

This reverts commit 974a585.

* 816: string updates for localization (#852)

* Revert "Revert delete PRs (#836)"

This reverts commit 974a585.

* Lockwise placeholder in delete_description and descriptive comment in edit string

* Update app services, android components, and megazord configuration (#865)

* Update app services, android components, and megazord configuration

* Update a-c to v7.0.0

* Send hashed UID ping when sync completes successfully.

* Remove handling for synctelemetryping, to be completed in separate issue. Update AS to 0.37.1

* Update Readme with l10n process (#872)

This is something we've been adding to all Readmes of mobile repos to direct people to Pontoon for localization (vs GitHub repos, where work isn't supposed to happen).

* 867 - Fix obvious NPE sync crash (#884)

* 871: Create infrastructure for feature flags (#882)

* Delete feature flag

* Move feature flags to separate support file

* Create support to check build configs. Create flags for CUD

* Add telemetry probes to syncIfRequired trigger syncs (#887)

* Import strings from l10n (#893)

* Import strings from android-l10n

State: mozilla-l10n/android-l10n@349efdb

* Commenting unused string for Espanol

* 791: Remove fake timeout from sync (#889)

* Remove fake timeout from sync

* Remove all sync timeout strings

* Rebase and lint

* Fail fast if the edge between to routes is not in the nav graph. (#895)

* Auto-signin with mozilla AuthProvider (#879) — behind a feature flag.

* WIP Automatic FxA sign-in support

* Cleanup welcome fragment/account detection

* Upgrade to Android Components 9.0.0

* Ensure landable, even if not releasable

* gradlew lint

* 628: edit view and routing (#843)

* Edit xml layout

* Add route, presenter, and fragment

Button clicks and dialog

Update dependencies

Edit presenter tests

Routing stuff

Usable state with spinner

Buttons working

Password visibility in edit mode

Lint

View

Create popup item and insert menu into detail view's kebab buttton

* Popup menu and click listener

Dropdown menu and formatting

Update list - in progress

DataStoreTest - mocked up test, stuck on init

Datastore update item detail test

* Save edited changes

Lint

Update dependencies

Edit unit tests

Add sync to datastore editing. Update var name in robot and screenshot tests

Reformatting dropdown menu and edit view

Adding inclusive popup to edit->detail view nav definition

Address nullable server passwords for delete and edit. Remove unused string.

Remove learn more clicks from test

Refactoring pushError into a helper method.

* Add fxalogin to autofill onboarding route. (#901)

* 798: routing back to itemlist on login/search/feedback forms (#890)

* Investigation

* Add locking routes.

* Explicitly ignore autofill on search and edit fields

* Ignore autofill on webviews

* Remove comments and clean up lint

* Add null text value for empty usernames (#902)

* Add null text value for empty usernames

* gradlew lint

* 907 fix ui tests (#908)

* 907: fix testSettings

* fixing routing issues

* fixing more routes

* changing button name and adding route

* fixing conflicts after rebase

* Update release-notes.md with v1.1.4

* Update release notes for v2.0.0

* Update version code and version name to v2.0.0
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eliserichards eliserichards eliserichards approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KotlinNullPointerException in DataStore.sync
2 participants
@jhugman @eliserichards