Closed Bug 836838 Opened 7 years ago Closed 7 years ago

Avoid race condition in distribution initialization

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox19 --- fixed
firefox20 + fixed
firefox21 + fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 2 obsolete files)

We should make sure Distribution.java is done copying the distribution files out of the APK before we do our distribution initialization in gecko.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #708737 - Flags: review?(bnicholson)
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.
Attachment #708737 - Attachment is obsolete: true
Attachment #708737 - Flags: review?(bnicholson)
Attached patch alternate patch (obsolete) — Splinter Review
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.
Attachment #708814 - Flags: review?(mark.finkle)
Attachment #708814 - Flags: review?(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.
(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.
Updated to address comments.
Attachment #708814 - Attachment is obsolete: true
Attachment #708814 - Flags: review?(mark.finkle)
Attachment #708814 - Flags: review?(bnicholson)
Attachment #708856 - Flags: review?(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.
Attachment #708856 - Flags: review?(bnicholson) → review+
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
Attachment #708856 - Flags: review?(mark.finkle)
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)
Attachment #708856 - Flags: review?(mark.finkle) → review+
Okay, one more try run before I land these 3 patches:
https://tbpl.mozilla.org/?tree=Try&rev=5165a08fb5fb
https://hg.mozilla.org/mozilla-central/rev/3ef8b1c93882
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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
Attachment #708856 - Flags: approval-mozilla-beta?
Same question as in bug 834681, will wait for response.
Attachment #708856 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #708856 - Flags: approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.