755638 - Uncaught SecurityException when attempting to sync multiple Fennecs to the same Sync account.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Scoobidiver (away)]

Seen in the first days of 14.0b1: bp-07364ce0-79d2-4e7d-aea9-f1c182120515.

java.lang.SecurityException: caller uid 10164 is different than the authenticator's uid
	at android.os.Parcel.readException(Parcel.java:1322)
	at android.os.Parcel.readException(Parcel.java:1276)
	at android.accounts.IAccountManager$Stub$Proxy.getUserData(IAccountManager.java:440)
	at android.accounts.AccountManager.getUserData(AccountManager.java:333)
	at org.mozilla.gecko.sync.syncadapter.SyncAdapter.getClientsCount(SyncAdapter.java:495)
	at org.mozilla.gecko.sync.syncadapter.SyncAdapter.getSyncInterval(SyncAdapter.java:352)
	at org.mozilla.gecko.sync.syncadapter.SyncAdapter.onPerformSync(SyncAdapter.java:334)
	at android.content.AbstractThreadedSyncAdapter$SyncThread.run(AbstractThreadedSyncAdapter.java:164)

More reports at:
https://crash-stats.mozilla.com/query/query?product=FennecAndroid&version=ALL%3AALL&range_value=1&range_unit=weeks&query_search=signature&query_type=contains&query=authenticator&do_query=1

Comment 1

[Richard Newman [:rnewman]]

This is the signature for users with Sync set up and more than one Firefox installed. It should occur once per user, then we'll disable the other Firefox for that Sync account to prevent it happening again.

I'd call this routine.

Comment 2

[Richard Newman [:rnewman]]

False dependency; timing coincidence.

Comment 3

[Scoobidiver (away)]

(In reply to Richard Newman [:rnewman] from comment #2)
> False dependency; timing coincidence.
What are the difference between this generic bug and a bug with STR that makes you think they are unrelated although they share their signature and stack?

Comment 4

[Richard Newman [:rnewman]]

(In reply to Scoobidiver from comment #3)
> (In reply to Richard Newman [:rnewman] from comment #2)
> > False dependency; timing coincidence.
> What are the difference between this generic bug and a bug with STR that
> makes you think they are unrelated although they share their signature and
> stack?

The other bug is "crash on page". There is no causality: the page has nothing to do with it, so the dependency is misleading.

A background thread initiated by Android is dying because the user has more than one Fennec installed.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #4)

> The other bug is "crash on page". There is no causality: the page has
> nothing to do with it, so the dependency is misleading.

Note that I'm not saying the crash signature differs... if you like, you can dupe the other bug to this one. Just that there is no dependency relationship here.

Comment 6

[Andreea Pod]

I run into this crash: https://crash-stats.mozilla.com/report/index/bp-91ae0d10-9424-4e24-ad1e-e4f452120607 when I had Beta build installed, sync running and after that I installed nightly and tried to sync. 

Build: Firefox 16.0a1 (2012-06-07)
Device: LG Optimus 2X (Android 2.2.2)

Comment 7

[Richard Newman [:rnewman]]

(In reply to Andreea Pod from comment #6)
> I run into this crash:
> https://crash-stats.mozilla.com/report/index/bp-91ae0d10-9424-4e24-ad1e-
> e4f452120607 when I had Beta build installed, sync running and after that I
> installed nightly and tried to sync. 
> 
> Build: Firefox 16.0a1 (2012-06-07)
> Device: LG Optimus 2X (Android 2.2.2)

Yup, expected.

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1 	http://en.m.wikipedia.org/wiki/Standard_RAID_levels
1 	http://1cooks.co.uk/
1 	http://jsbin.com/3/ipelar.js

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

1. place nightly on the device
2. move nightly to SDcard
3. setup sync
4. reboot device
5. go back to sync and select nightly to sync
6. launch nightly

Expected: no crash
Actual: crash.

related to bug 768102

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Turns out that I had multiple firefox installed.  Scratch comment 9

STR:
1. place firefox release on device, place nightly on device
2. set up sync accounts in the sync section
3. set nightly to the version to sync
4. launch nightly

Expected, no crash
Actual: crash.

Comment 12

[Benoit Girard (:BenWa)]

I get this after every 20 seconds of uptime on my release build from the market. I have sync setup between multiple phones and desktop.

Comment 13

[Richard Newman [:rnewman]]

(In reply to Benoit Girard (:BenWa) from comment #12)
> I get this after every 20 seconds of uptime on my release build from the
> market. I have sync setup between multiple phones and desktop.

Do you have more than one channel installed on your phone? (Not "installed with Sync set up", just installed is enough.)

Comment 14

[Benoit Girard (:BenWa)]

I have 'Firefox' (release) installed with sync, and 'Fennec bgirard' (dev build) with sync. I don't currently have any other channel installed but it's likely I did at one point. I'm running jellybean.

Comment 15

[Richard Newman [:rnewman]]

(In reply to Benoit Girard (:BenWa) from comment #14)
> I have 'Firefox' (release) installed with sync, and 'Fennec bgirard' (dev
> build) with sync. I don't currently have any other channel installed but
> it's likely I did at one point. I'm running jellybean.

If your custom Fennec build is earlier than <https://hg.mozilla.org/mozilla-central/rev/28106e179603>, then this is your problem.

(Even if your build is after that, you shouldn't be able to have two different Sync accounts set up.)

Comment 16

[Scoobidiver (away)]

(In reply to Richard Newman [:rnewman] from comment #15)
> If your custom Fennec build is earlier than
> <https://hg.mozilla.org/mozilla-central/rev/28106e179603>, then this is your
> problem.
There are no crashes after 17.0a1/20120726, so it's fixed by bug 772645.

Comment 17

[Scoobidiver (away)]

To clarify the tracking flags, with combined signatures, it's #23 top crasher in 15.0b2 and #3 in 16.0a2.

Comment 18

[Richard Newman [:rnewman]]

(In reply to Scoobidiver from comment #16)

> There are no crashes after 17.0a1/20120726, so it's fixed by bug 772645.

Coincidence; see Bug 772645 comment 16.

My hypothesis is that after uplift to Aurora (train or not), this will reoccur. It would also reoccur if a user had two of {Aurora, Beta, Release} installed and syncing.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to Richard, if I'm reading this bug correctly there's a once-per-user crash when they have a second Firefox installed on their phones (and using Sync).  This is a poor experience and at a time where we're trying to grow our rating on the Play Store, it seems like trying to get rid of a crash we can reproduce should be a priority. What are the options here for clearing this up?

Comment 20

[Scoobidiver (away)]

(In reply to Lukas Blakk [:lsblakk] from comment #19)
> What are the options here for clearing this up?
If I understood the explanation (see bug 772645#c16), it will be fixed by bug 777800.

Comment 21

[Richard Newman [:rnewman]]

(In reply to Scoobidiver from comment #20)

> If I understood the explanation (see bug 772645#c16), it will be fixed by
> bug 777800.

Correct, in the general case, and some of the instances will be avoided by the partitioning introduced in Bug 772645.

(See the "Depends On" field.)

We can attempt to catch this exception at our first point of interaction with the Android accounts system, but I'd be hesitant to pile on too many Band-Aids in this code path.

Comment 22

[Richard Newman [:rnewman]]

Chenxia, will you have time to attend to this today?

If we can, I'd like to see if we can get a patch against mozilla-beta and mozilla-aurora that prevents any user-visible crash here, perhaps simply by catching SecurityException in performSync.

This is a travel week for me, so I'm hesitant to say I can fit this in myself, but I'll try if you can't.

Comment 23

[Chenxia Liu [:liuche]]

Testing and building the patches now, Richard. I'll need flags for landing on beta and aurora.

This should be a small fix, just adding handling for SecurityException (and uncaught exceptions) in SyncAdapter where there was none before.

Comment 24

[Chenxia Liu [:liuche]]

Attachment: mozilla-aurora patch v1

Catches and handles SecurityException. Unchecks sync checkbox automatically.

Comment 25

[Chenxia Liu [:liuche]]

Attachment: mozilla-beta patch v1

Catches and handles SecurityException. Unchecks sync checkbox automatically.

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 651513 [details] [diff] [review]
mozilla-beta patch v1

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

::: mobile/android/base/sync/syncadapter/SyncAdapter.java
@@ +154,3 @@
>        syncResult.stats.numIoExceptions++;
> +    } catch (Exception ex) {
> +      Logger.error(LOG_TAG, "Unknown exception. Aborting sync.", ex);

This doesn't need to be here. This method is called with an Exception, and the clauses don't take any action that would throw. The `finally` clause is just there so that each clause can `return`.

@@ +279,5 @@
> +        // Uncheck Sync checkbox because we cannot sync this instance.
> +        ThreadPool.run(new Runnable() {
> +          @Override
> +          public void run() {
> +            SyncAccounts.setSyncAutomatically(account, false);

How 'bout

  class SyncAccounts {
    public void backgroundSetSyncAutomatically(…) {
      …
    }
    …
  }

? That'll make this clearer.

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 651512 [details] [diff] [review]
mozilla-aurora patch v1

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

Same comment as for beta.

Comment 28

[Chenxia Liu [:liuche]]

Attachment: mozilla-beta patch v2

Addressed comments, pulled off-main-thread Account handling out into SyncAccounts.

Comment 29

[Chenxia Liu [:liuche]]

Attachment: mozilla-aurora patch v2

Addressed comments, pulled off-main-thread Account handling out into SyncAccounts.

Comment 30

[Richard Newman [:rnewman]]

Attachment: Beta patch, updated.

Changes from Chenxia's beta patch:

* Request beta, not aurora, approval;
* Moved syncAutomatically change into exception handler, not duplicated.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Old.

User impact if declined: 
  Subset of users with two Fennecs installed can see a crash on first sync of second version.

Testing completed (on m-c, etc.):
  None yet. Will port patch to inbound momentarily.

Risk to taking this patch (and alternatives if risky):
  Should only affect the exception path; slim risk.

String or UUID changes made by this patch: 
  None.

Comment 31

[Richard Newman [:rnewman]]

Attachment: Aurora patch, updated.

Same, but for Aurora.

Chenxia, back over to you. Please double-check my review changes, backport this to Git and m-c, verify, and so forth.

Much obliged!

Comment 32

[Chenxia Liu [:liuche]]

rnewman's beta and aurora patches both test out fine on my device.

Backported to git, tested, and pushed to develop, but waiting to for tree to open again.

Comment 33

[Richard Newman [:rnewman]]

(In reply to Chenxia Liu [:liuche] from comment #32)
> rnewman's beta and aurora patches both test out fine on my device.

Upload builds for QA? Tight on beta.

Comment 34

[Chenxia Liu [:liuche]]

Beta and Aurora builds: http://people.mozilla.org/~liuche/bug-755638/

STR:
1. Install release (or beta) from marketplace. Set up sync.
2. Install test apk.
3. Go to Settings > Accounts > (Sync Account). Both versions of FF should show up in the Data & Synchronization, with release checked, and test version unchecked.
4. Click on unchecked test version. It should immediately uncheck, and no crash should occur.

Over to QA. I'll be on PTO but have internet access, so if no one else is around, I will land it when this gets beta and aurora approval.

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 651632 [details] [diff] [review]
Aurora patch, updated.

Approving for Aurora, not sure need to take this on Beta at this point in the cycle if a user has to have two different Firefox products installed - that's an unlikely use case for our GA so this can ride the trains.

Comment 36

[Gregory Szorc [:gps]]

ttps://hg.mozilla.org/releases/mozilla-aurora/rev/1ec304d94205

Comment 37

[Chenxia Liu [:liuche]]

Inbound landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2759cfa65dc5

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2759cfa65dc5

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

updating status-15 to wontfix.