Not a P1.

Sorry for not fleshing out this ticket. This is of moderate to high priority. The account pickling code is what allows us to survive an SD card ejection -- Android will delete your account if Firefox is installed on the SD card. If we don't implement this ticket, users will have to set up Firefox Account every time they plug their phone into their computer, or eject the SD card.

Is this what you are looking for? It would be helpful if you could answer my TODOs while you're in there. Otherwise, still TODO: * Call pickle/unpickle from main code base * Add tests

Comment on attachment 8386397 [details] Patch (github) Reviewed on GitHub.

Comment on attachment 8386397 [details] Patch (github) Tested locally for proper behavior without sdcard and with sdcard ejection on Galaxy Nexus and Galaxy S4 respectively. I have not tested version sharing.

Version sharing (Aurora, Nightly) appears to work properly - the account is removed for both channels. I did not test with sdcard ejection because it's probably not worth my time. Builds (w/ additional debug output): Aurora: https://people.mozilla.com/~mcomella/apks/mcomella-957894_aurora.apk Nightly: https://people.mozilla.com/~mcomella/apks/mcomella-957894_nightly.apk

I reviewed commit fb70c4ba. It's very difficult to review un-folded work, so I made notes and am pasting them here. This is looking good. There's a frightening amount of duplicated code, but in some halcyon future when we delete Sync entirely, that'll go away, right? Major issue: The unpickle should happen in getFirefoxAccounts, not firefoxAccountsExist. The latter delegates to the former, but consumers can call either. Minor issues: Is it possible that something in FirefoxAccounts (namely, firefoxAccountsExist) is called from main thread? (Must check Fennec.) * Based on calls to postToUiThread, appears that BrowserApp.handleMessage is always on a background thread. * Tabs.java call is fairly clearly background thread. * Not clear that FxAccountStatusActivity call is backgrounded correctly. Sorry for duplication -- this is because StatusActivity must inherit from FragmentActivity. Further to this: In FxAccountGetStartedActivity, prefer CountDownLatch to synchronized and wait. Even better would be to make FirefoxAccounts.* bullet-proof: always latch yourself, do the right thing in case of InterruptedException, and don't make the caller be on the background thread. Great comment in FxAccountDeletedService! Through-out, prefer class.getSimpleName() for LOG_TAGs, please. The persistBundle/unbundle comments should reference the input ExtendedJSONObject as well; otherwise the links in the comment look like typos. I think the static authorities landed, so this needs rebasing and updating. + // TODO: Not the same context as when it was created. Okay? Yeah; the context is just used transiently, to add the account, etc. Future issues: With FxAccounts, we have multiple ways to modify an Account. It's not clear to me that writing the pickle *only from the SyncAdapter* is sufficient. We might want to move the AndroidFxAccount to an edit()/submit() paradigm, and write the pickle on submit(). This is definitely follow-up.

Comment on attachment 8386397 [details] Patch (github) Fixed up for the review comments. Caveats below: (In reply to Nick Alexander :nalexander from comment #7) > Further to this: In FxAccountGetStartedActivity, prefer CountDownLatch to > synchronized and wait. Even better would be to make FirefoxAccounts.* > bullet-proof: always latch yourself, do the right thing in case of > InterruptedException, and don't make the caller be on the background thread. Done. It could arguably be done better if we let ThreadPool.run return the `Future`, however, I'm not familiar with API so I opted for latches. > The persistBundle/unbundle comments should reference the input > ExtendedJSONObject as well; otherwise the links in the comment look like > typos. Not quite sure if I did this like you wanted. > I think the static authorities landed, so this needs rebasing and updating. They have not, the PR is [1]. > Future issues: > > With FxAccounts, we have multiple ways to modify an Account. It's not clear > to me that writing the pickle *only from the SyncAdapter* is sufficient. We > might want to move the AndroidFxAccount to an edit()/submit() paradigm, and > write the pickle on submit(). This is definitely follow-up. Is this something to file? If so, can you do it? I'm not quite sure I entirely understand. Also, are there any other TODOs to be addressed? In particular, [2] bothers me (github often messes up these links so that's the TODO on the prefs version changing underneath us). [1]: https://github.com/mozilla-services/android-sync/pull/426 [2]: https://github.com/mcomella/android-sync/commit/bc3ebb1c516b54d88599722caef94061f16025c5#diff-197204d8e9fa49eb1e993cf90f21e828R104

Comment on attachment 8386397 [details] Patch (github) Again, if it works for you, it works for me. Looks good!

Comment on attachment 8397276 [details] [diff] [review] Update account pickling code for Firefox Accounts. (from github) [Approval Request Comment] Bug caused by (feature/regressing bug #): Firefox Accounts User impact if declined: Users who install Firefox on the SDCard and eject it will have to set up Firefox Accounts again when they reinsert the card. nalexander's tentative plan in bug 988437 comment 2 also requires this to be uplifted before bug 988437 can be (though I'm honestly not sure what the reasons are). Testing completed (on m-c, etc.): 2 unit tests. rnewman mentioned via IRC that there will be additional hand-testing before uplift. Risk to taking this patch (and alternatives if risky): Medium. A non-critical code path, but a fair amount of code was added. This implementation largely follows the previous Sync implementation (which should mitigate some unforseen risks). There should be some greater concern over odd use cases that don't affect most users (e.g. one bug was on account removal, where the account may be accidentally re-added by the unpickling code, which was fixed by the deletion service). String or IDL/UUID changes made by this patch: N/A

(In reply to Michael Comella (:mcomella) from comment #12) > Users who install Firefox on the SDCard and eject it will have to set up > Firefox Accounts again when they reinsert the card. nalexander's tentative > plan in bug 988437 comment 2 also requires this to be uplifted before bug > 988437 can be (though I'm honestly not sure what the reasons are). We plan to rely on unpickling to support the splitting of account types: if you previously used an account with two Firefoxes, one will steal it, and the other will unpickle a duplicate. The pickling code needs to go out first, because it needs to write the pickle file in advance!

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e565e66ead5 https://hg.mozilla.org/releases/mozilla-beta/rev/3946a4eb8302