Last Comment Bug 753175 - Race condition in Sync Migration / Preference:Get
: Race condition in Sync Migration / Preference:Get
Status: RESOLVED FIXED
: dataloss
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 15
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-08 16:14 PDT by Gian-Carlo Pascutto [:gcp]
Modified: 2012-05-10 07:40 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
beta+


Attachments
patch (4.11 KB, patch)
2012-05-08 17:18 PDT, :Margaret Leibovic
gpascutto: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Review

Description Gian-Carlo Pascutto [:gcp] 2012-05-08 16:14:31 PDT
https://bugzilla.mozilla.org/show_bug.cgi?id=752907#c9

Profile Migrator should not unregister its Preferences event listener until it actually received the preferences it asked for. It will currently get confused if someone else requests preferences at the same time.

D/ProfileMigrator( 1410): Received event: Preferences:Data
W/ProfileMigrator( 1410): Could not recover setting for = ui.zooming.animation_frames
Comment 1 Matt Brubeck (:mbrubeck) 2012-05-08 16:15:44 PDT
Nominating for blocking because this can prevent profile migration from finishing successfully.
Comment 2 :Margaret Leibovic 2012-05-08 17:18:20 PDT
Created attachment 622221 [details] [diff] [review]
patch
Comment 3 :Margaret Leibovic 2012-05-08 17:19:30 PDT
I tested this patch with the STR from bug 752907 comment 0, and it indeed fixes the sync migration problem.
Comment 4 Gian-Carlo Pascutto [:gcp] 2012-05-08 17:31:39 PDT
Comment on attachment 622221 [details] [diff] [review]
patch

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

Our Preferences:Get design won't hold up in the long run. If you would fire a request for only one of the preferences elsewhere, we'd still misfire and think this was a reply to our request. We'll have to keep this in mind for the longer term.

::: mobile/android/base/ProfileMigrator.java
@@ +456,5 @@
>                      JSONObject jPref = jsonPrefs.getJSONObject(i);
>                      final String prefName = jPref.getString("name");
> +
> +                    // Check to make sure we're working with preferences we requested.
> +                    if (!Arrays.asList(SYNC_SETTINGS_LIST).contains(prefName))

I think this is guaranteed to be in mSyncSettingsList already.
Comment 5 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-09 07:12:30 PDT
> Our Preferences:Get design won't hold up in the long run. If you would fire
> a request for only one of the preferences elsewhere, we'd still misfire and
> think this was a reply to our request. We'll have to keep this in mind for
> the longer term.

Yeah, I ran into this problem when adding prefs to GeckoLayerClient and PanZoomController. I've filed bug 753312 for it.
Comment 6 Gian-Carlo Pascutto [:gcp] 2012-05-09 10:14:45 PDT
Comment on attachment 622221 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Sync will not migrate successfully dependent on timing.
Comment 9 Ed Morley [:emorley] 2012-05-10 07:40:09 PDT
https://hg.mozilla.org/mozilla-central/rev/228971745c99

Note You need to log in before you can comment on or make changes to this bug.