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

Closes #6354 - Take Lazy references to storage in FennecMigrator #6395

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

grigoryk
Copy link
Contributor

This is a follow-up to #6304, in which I forget that FennecMigrator exists.


Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #6395 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6395      +/-   ##
============================================
+ Coverage     78.58%   78.73%   +0.15%     
+ Complexity     4626     4597      -29     
============================================
  Files           610      612       +2     
  Lines         22200    22126      -74     
  Branches       3253     3212      -41     
============================================
- Hits          17446    17422      -24     
+ Misses         3418     3394      -24     
+ Partials       1336     1310      -26
Impacted Files Coverage Δ Complexity Δ
...lla/components/support/migration/FennecMigrator.kt 62.31% <100%> (ø) 38 <0> (ø) ⬇️
...onents/support/sync/telemetry/BaseGleanSyncPing.kt 100% <0%> (ø) 11% <0%> (ø) ⬇️
...ents/ui/autocomplete/InlineAutocompleteEditText.kt
...omponents/feature/accounts/FxaWebChannelFeature.kt 83.76% <0%> (ø) 8% <0%> (?)
...nts/feature/accounts/FirefoxAccountsAuthFeature.kt 91.3% <0%> (ø) 5% <0%> (?)
...nents/lib/push/amazon/AbstractAmazonPushService.kt 72.97% <0%> (ø) 10% <0%> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 653ce4a...d10c9e2. Read the comment docs.

Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

Leaving the merge to be synchronized with the breaking changes needed in Fenix.

@jonalmeida jonalmeida added the 🛬 needs landing PRs that are ready to land label Mar 26, 2020
@grigoryk
Copy link
Contributor Author

Fenix PR, for when this is published in a nightly - mozilla-mobile/fenix#9417

@grigoryk
Copy link
Contributor Author

bors r=jonalmeida

@bors
Copy link

bors bot commented Mar 26, 2020

Build succeeded

@bors bors bot merged commit 0ac4ade into mozilla-mobile:master Mar 26, 2020
grigoryk pushed a commit to grigoryk/android-components that referenced this pull request May 13, 2020
This fixes fallback from mozilla-mobile#6395 in which
FennecMigrator changed to accept a Lazy-wrapped reference to the account manager.

This was done so that during regular (non-migrating) startup, a simple act of building a no-op
migrator will not cause account manager initialization.

However, during a migrating startup this creates a race to initialize account manager.
Two participants in this race are:
1) FenixApplication running expensive operations (such as storage init, account manager init)
after visual completeness. However, currently that just entails a 5 second delay from `onStart`.
2) FxA migration, which runs roughly at the mid-point of the migration.

Account manager currently can't be initialized on a background thread (or it will crash).
FxA migration runs on a background thread, so if (2) wins (that is, migrations before FxA migration take
less than 5 seconds), then migration will crash.

This patch adds an eager initialization right before we kick-off migrations but are still on the main thread,
iff an FxA migration will be executed.
bors bot pushed a commit that referenced this pull request May 13, 2020
6964: Initialize accountManager on the main thread during migration r=pocmo a=grigoryk

Fenix bug for this - mozilla-mobile/fenix#10432

This fixes fallout from #6395 in which
FennecMigrator changed to accept a Lazy-wrapped reference to the account manager.

This was done so that during regular (non-migrating) startup, a simple act of building a no-op
migrator will not cause account manager initialization.

However, during a migrating startup this creates a race to initialize account manager.
Two participants in this race are:
1) FenixApplication running expensive operations (such as storage init, account manager init)
after visual completeness. However, currently that just entails a 5 second delay from `onStart`.
2) FxA migration, which runs roughly at the mid-point of the migration.

Account manager currently can't be initialized on a background thread (or it will crash).
FxA migration runs on a background thread, so if (2) wins (that is, migrations before FxA migration take
less than 5 seconds), then migration will crash.

This patch adds an eager initialization right before we kick-off migrations but are still on the main thread,
iff an FxA migration will be executed.



Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
pocmo pushed a commit that referenced this pull request May 13, 2020
This fixes fallback from #6395 in which
FennecMigrator changed to accept a Lazy-wrapped reference to the account manager.

This was done so that during regular (non-migrating) startup, a simple act of building a no-op
migrator will not cause account manager initialization.

However, during a migrating startup this creates a race to initialize account manager.
Two participants in this race are:
1) FenixApplication running expensive operations (such as storage init, account manager init)
after visual completeness. However, currently that just entails a 5 second delay from `onStart`.
2) FxA migration, which runs roughly at the mid-point of the migration.

Account manager currently can't be initialized on a background thread (or it will crash).
FxA migration runs on a background thread, so if (2) wins (that is, migrations before FxA migration take
less than 5 seconds), then migration will crash.

This patch adds an eager initialization right before we kick-off migrations but are still on the main thread,
iff an FxA migration will be executed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants