Avoid race conditions when deleting Firefox Accounts

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nalexander, Unassigned, Mentored)

Tracking

Trunk
Firefox 44
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Right now there's a race between pickling/unpickling and deleting Firefox Accounts in Fennec.  You can delete an account and then see it unpickled immediately, either by the system or by your own action (like opening the Settings Activity again).

We could do a few things to mitigate:

* delete harder (tricky due to threading);
* add a flag to the account bundle pickle that gets written when deleted and prevents unpickling (similar to the above, and tricky with threading);
* keep an in-memory or in-SharedPreferences sentinel saying an account is deleted;
* use a single-threaded executor service (or intent service) to serialize deletions and unpicklings.

I think the first and last are likely to be most fruitful.

This is a good mentor ticket, but not a good first ticket.

Comment 1

4 years ago
i am new here please help me. how i can solve this bug

Comment 2

4 years ago
i want to work on this bug
(In reply to Amit singh from comment #1)
> i am new here please help me. how i can solve this bug

Hi Amit, nice to meet you!  As I said in the description, this isn't a good first bug.  I think Bug 1147211 might be a better first ticket.  See you over there!
No longer blocks: 1179978
Bug 1182270 - AccountPickler public interface synchronized to avoid race condition r?nalexander.
Attachment #8634386 - Flags: review?(nalexander)
Comment on attachment 8634386 [details]
MozReview Request: Bug 1182270 - AccountPickler public interface synchronized to avoid race condition r?nalexander.

https://reviewboard.mozilla.org/r/13389/#review12969

This is a good patch, and I think we should apply it, but it doesn't address the actual race.  The issue is that we "clean up" in an intent handler, which happens some time after the account is deleted.  So:

1) Sync, write pickle file.
2) Choose to delete account.  Android removes account and we queue intent.
3) Have something happen (Sync request, or some Fennec action) that unpickles the old Android Account.
4) Intent is handled, but the account has been re-created and we don't delete it.

So mark this is a pre: commit and leave-open the ticket after you land.
Attachment #8634386 - Flags: review?(nalexander) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/0a2ea3f59b1b3ee44b484cf728827706b0a9a098
changeset:  0a2ea3f59b1b3ee44b484cf728827706b0a9a098
user:       vivek <vivekb.balakrishnan@gmail.com>
date:       Thu Jul 16 00:24:37 2015 +0300
description:
Bug 1182270 - Pre: AccountPickler public interface synchronized to avoid race condition r=nalexander.

Comment 8

4 years ago
I can take this if nobody's working on it currently.
(In reply to swaroop.rao from comment #8)
> I can take this if nobody's working on it currently.

vivek: are you interested in mentoring Swaroop?  I can help out with final reviews, etc.
Flags: needinfo?(vivekb.balakrishnan)
@Nick: Yes, I'm interested in mentoring this bug.

Hi Swaroop,


The simplest solution here would be to check for the account and delete it if it exists in deletePickle call (try harder). As the methods were synchronized in pre-commit we don't need to worry about threading issues mentioned in comment 1 (\O/)

I made a silly mistake in my pre-commit and I would be grateful if you can correct it as part of your commit. Can you please change the AccountPickler import in FxAccountDeletedService.java to  org.mozilla.gecko.fxa.authenticator.AccountPickler.

We can easily simulate the scenario mentioned in comment 5 by attaching a debugger and stepping through break points.

Please feel free to ping me if you need help.

Happy coding :)
Flags: needinfo?(vivekb.balakrishnan)

Comment 11

4 years ago
Will do, Vivek. Thanks for the pointers. I have a build set up and ready to go, so I will start working on this tomorrow.

Comment 12

4 years ago
Vivek, I've changed the import statement. Now, about the other stuff, I'm not really sure I understand what needs to be done. I see the following:
1. A call to deletePickle() in FxAccountDeletedService.java
2. A local method in FxAccountDeletedService.java
3. A static method called deletePickle() in org.mozilla.gecko.fxa.authenticator.AccountPickler which is called by the deletePickle() method above.

Part of the reason is that I don't understand what pickling means. If I had to guess, I would say that it is a way to persist a Firefox account in the device. So, when an account is deleted, we need to delete the pickle as well? And, instead, currently we are doing something like a soft delete?

While I await your response, I will try stepping through the code in a debugger like you suggested.
Hi Swaroop,

Yes, you are right about pickling. You can equate pickle/un-Pickle operation to marshalling/unmarshalling to/from a file. We pickles fxa account properties to a json file during each sync. Fennec un-pickle this file to recreate the account if the file exists.

When the account is removed, we fire an intent in FxAccountAuthenticator to start the FxAccountDeletedService [1]. We clean up all the account related resources like the pickled file and cached oauth tokens etc in FxAccountDeletedService. However, even before this IntentService executes, Fennec may unpickle the file and create the account back. We are trying to avoid this scenario in this bug.

FxAccountDeletedService.deletePickle() calls synchronized static AccountPickler.deletePickle(). Here we delete the pickeled File.

Please feel free to ping me back if the above explanation is not clear.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java?offset=0#356

Comment 14

4 years ago
Vivek, sorry for the delay. My Linux VM is giving me problems. Looks like I will have to reinstall it and set up the code base all over again. I'll  do it tonight and try out what you have suggested.

Comment 15

4 years ago
Hi Vivek,

I think I've grasped the problem well enough to be able to solve it.

Since you've said that you've solved the threading problem using the "synchronized" keyword, that leaves us with these options that Nick has mentioned:

> 1- Delete harder. 
I'm not sure what Nick meant by that.

> 2- Add a flag to the account bundle pickle that gets written when deleted and prevents unpickling. 
That is what I prefer as that is the most one that seems obvious to me.

> 3- Use a single-threaded executor service (or intent service) to serialize deletions and unpicklings.
I guess that it is doable but that would make us to create a base Service that contains a static 
HandlerThread to be able to share the same handler across the child classes and be able to serialize the delections and unpickling.

Let me know what you think and please correct me If I said anything wrong.
Flags: needinfo?(vivekb.balakrishnan)
Hi Ahmed,

My apologies for not replying earlier.
I would answer your question in reverse order.

3) We don't need that anymore. The synchronized keyword should take care of serializing deletion and unpickling

2) I am in favor of adding a flag to the account pickle.

1) As Nick commented in irc,  delete harder means deleting the pickle by blocking in main thread. This approach may work but I think approach 2) is better.


Can you please address the mistake I mentioned in comment 10 as part of your patch
Flags: needinfo?(vivekb.balakrishnan)

Comment 17

4 years ago
Hi vivek, 

According to our agreement we've opted to the deleted harder approach by deleting the pickle file but before deleting we add an assertion to assert that the deletion happens in a background thread.

Let me know what do you think.
Attachment #8661978 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8661978 [details] [diff] [review]
avoid_race_condition_delete_harder.patch

Review of attachment 8661978 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Ahmed,

Thanks for the patch, it looks great.
I have a couple of minor changes, can you please address them

Do you have access to try server?  
Can you do a try build for the updated patch.

::: mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java
@@ +355,5 @@
>      // to include private information such as a password.
>      final AndroidFxAccount androidFxAccount = new AndroidFxAccount(context, account);
> +
> +    // Delete the pickle file to prevent recreation of the account.
> +    // Assert that we are running this code from a background thread to verify our assumption,

Can you please explain what our assumption is.
Also comments should be a complete sentence.

@@ +356,5 @@
>      final AndroidFxAccount androidFxAccount = new AndroidFxAccount(context, account);
> +
> +    // Delete the pickle file to prevent recreation of the account.
> +    // Assert that we are running this code from a background thread to verify our assumption,
> +    assert Looper.myLooper() != Looper.getMainLooper();

Use ThreadUtils.assertNotOnUiThread() instead of this

@@ +370,5 @@
>  
>      return result;
>    }
> +
> +  private static void deletePickle(final Context context) {

This doesn't need to be static
Attachment #8661978 - Flags: review?(vivekb.balakrishnan) → feedback+

Comment 19

4 years ago
Hi vivek,

I've done your suggestions, please review the comment that I've written in the code and inform me if there is something wrong about it.

I'll push a try build right away.
Attachment #8661978 - Attachment is obsolete: true
Attachment #8662065 - Flags: review+

Comment 21

4 years ago
Sorry I posted the wrong try build.

Here is the correct one
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21a948b49093
Comment on attachment 8662065 [details] [diff] [review]
avoid_race_condition_delete_harder.patch

Review flag updated.
Adding :Nick for a feedback
Attachment #8662065 - Flags: review?(vivekb.balakrishnan)
Attachment #8662065 - Flags: review+
Attachment #8662065 - Flags: feedback?(nalexander)
Comment on attachment 8662065 [details] [diff] [review]
avoid_race_condition_delete_harder.patch

Review of attachment 8662065 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. 
Please remove the import in your final patch.
You don't need to flag me for review. 

Thanks for the patch.

::: mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java
@@ +38,5 @@
>  import android.accounts.NetworkErrorException;
>  import android.content.Context;
>  import android.content.Intent;
>  import android.os.Bundle;
> +import android.os.Looper;

Sorry for not mentioning this earlier, we don't need this import
Attachment #8662065 - Flags: review?(vivekb.balakrishnan) → review+
Comment on attachment 8662065 [details] [diff] [review]
avoid_race_condition_delete_harder.patch

Review of attachment 8662065 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.  Good work Ahmed and vivek!
Attachment #8662065 - Flags: feedback?(nalexander) → feedback+

Comment 25

4 years ago
Hi vivek,

I've removed the unused import, I'll add "check-in" needed keyword.
Attachment #8662065 - Attachment is obsolete: true
Attachment #8663317 - Flags: review+

Comment 26

4 years ago
Sorry I did a qref on the wrong top.
Attachment #8663317 - Attachment is obsolete: true
Attachment #8663318 - Flags: review+

Updated

4 years ago
Hi, this doesn't apply to fx-team cleanly it seems:

applying avoid_race_condition_delete_harder.patch
patching file mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java
Hunk #1 FAILED at 23
1 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/fxa/authenticator/FxAccountAuthenticator.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh avoid_race_condition_delete_harder.patch

can you take a look, thanks!
Flags: needinfo?(ahmedibrahimkhali)
Keywords: checkin-needed

Comment 28

4 years ago
Hi Carsten,

I've uploaded a new patch that is rebased on the latest mozilla-central commit. 

I also posted a new try build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1700eb4d4200
Attachment #8663318 - Attachment is obsolete: true
Flags: needinfo?(ahmedibrahimkhali)
Attachment #8663834 - Flags: review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56eb6c4edf06
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.