836838 - Avoid race condition in distribution initialization

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We should make sure Distribution.java is done copying the distribution files out of the APK before we do our distribution initialization in gecko.

Comment 1

[:Margaret Leibovic]

Attachment: patch

We only need to worry about this race when the distribution files are being copied out of the APK, which only happens on first run right now.

To avoid sending extra messages, I'm only sending a "Distribution:Set" when copyFiles runes (and when it actually finds distribution files). And to avoid calling getPrefs() twice, I'm setting a flag to see if we've already found and used a preferences.json. I actually found that we're finding the copied file in init before we even get the "Distribtion:Set" notification, so we might as well set the prefs in there, then just use this notification as a safety net.

Comment 2

[:Margaret Leibovic]

Comment on attachment 708737 [details] [diff] [review]
patch

Actually, I'm not sure that I like this patch as much as a different version I made earlier. I was concerned that the "Distribution:Set" notification might fire before we register our observer, but I don't think I actually need to worry about that because of the way events are handled on the event loop.

It seems simpler to just let Java take care of telling us whether or not to set up the distribution stuff, and that would also prevent us from ever trying to read the JSON file in JS if there aren't any distribution files.

Comment 3

[:Margaret Leibovic]

Attachment: alternate patch

I don't know that I like the way I'm setting an int pref like this in Distribution.java, but I wanted to only send "Distribution:Set" when there are distribution files present, and this seemed like the easiest way to do that.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 708814 [details] [diff] [review]
alternate patch

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

::: mobile/android/base/Distribution.java
@@ +108,5 @@
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Distribution:Set", null));
> +
> +        SharedPreferences settings = activity.getPreferences(Activity.MODE_PRIVATE);
> +        String keyName = activity.getPackageName() + ".distribution_state";
> +        settings.edit().putInt(keyName, STATE_SET).commit();

This might be cleaner if copyFiles() returned the distributionSet boolean, and the caller was responsible for this part. It seems weird that copyFiles() would be responsible for setting prefs or sending Gecko messages.

::: mobile/android/chrome/content/browser.js
@@ +8128,5 @@
> +
> +      case "prefservice:after-app-defaults":
> +        Services.obs.removeObserver(this, "prefservice:after-app-defaults");
> +        this.getPrefs();
> +        break;

I think this can be removed since you got rid of the observer.

Comment 5

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #4)

> > +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Distribution:Set", null));
> > +
> > +        SharedPreferences settings = activity.getPreferences(Activity.MODE_PRIVATE);
> > +        String keyName = activity.getPackageName() + ".distribution_state";
> > +        settings.edit().putInt(keyName, STATE_SET).commit();
> 
> This might be cleaner if copyFiles() returned the distributionSet boolean,
> and the caller was responsible for this part. It seems weird that
> copyFiles() would be responsible for setting prefs or sending Gecko messages.

Oh, duh, that's much smarter. I think my brain got confused with async JS functions.

> ::: mobile/android/chrome/content/browser.js
> @@ +8128,5 @@
> > +
> > +      case "prefservice:after-app-defaults":
> > +        Services.obs.removeObserver(this, "prefservice:after-app-defaults");
> > +        this.getPrefs();
> > +        break;
> 
> I think this can be removed since you got rid of the observer.

I didn't remove it, I just moved it to the "Distribution:Set" handler. Maybe that's not really clear (and not really a big win), so I could just keep it in init.

Comment 6

[:Margaret Leibovic]

Attachment: alternate patch (v2)

Updated to address comments.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 708856 [details] [diff] [review]
alternate patch (v2)

It'd be nice if we could just call getPrefs() in Distribution:Set and drop the "prefservice:after-app-defaults" observer, but I guess we'd have to worry about calling getPrefs() too early then.

Comment 8

[:Margaret Leibovic]

Comment on attachment 708856 [details] [diff] [review]
alternate patch (v2)

Flagging mfinkle to get input on variable naming (his specialty).

Maybe something like this?

STATE_FIRST_RUN = 0
STATE_NONE = 1
STATE_SET = 2

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 708856 [details] [diff] [review]
alternate patch (v2)

>diff --git a/mobile/android/base/Distribution.java b/mobile/android/base/Distribution.java

>+    private static final int STATE_NOT_INITIALIZED = 0;
>+    private static final int STATE_INITIALIZED = 1;
>+    private static final int STATE_SET = 2;

STATE_NOT_INITIALIZED -> STATE_UNKNOWN
STATE_INITIALIZED -> STATE_NONE
STATE_SET -> STATE_SET  (this is good enough I think and we use "Xxx:Set" for the message the Gecko)

Comment 10

[:Margaret Leibovic]

Okay, one more try run before I land these 3 patches:
https://tbpl.mozilla.org/?tree=Try&rev=5165a08fb5fb

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef8b1c93882

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3ef8b1c93882

Comment 13

[:Margaret Leibovic]

Comment on attachment 708856 [details] [diff] [review]
alternate patch (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 834681
User impact if declined: no distribution support (we want to uplift this so that partners can use it sooner rather than later)
Testing completed (on m-c, etc.): landed on m-c 2/3, already on aurora
Risk to taking this patch (and alternatives if risky): low-risk, this code is pretty self-contained
String or UUID changes made by this patch: n/a

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Same question as in bug 834681, will wait for response.

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-beta/rev/db9742bcbfde

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-release/rev/19bf07fba5b0