753312 - The Preferences:Get message should take a UID of some sort

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Currently we have a bunch of different pieces of Java code that sends Preferences:Get to browser.js to read prefs. These can happen simultaneously, and there's no way for such code to definitively identify which resulting Preferences:Data message is theirs. We should add some sort of UID to the Preferences:Get message that is returned in the Preferences:Data message so that we can make this code more robust.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a GeckoPrefsHelper class and a requestId field to the JSON

I'm uploading the patch in pieces so that it's easier to review but I'll have to squash it all into one patch for landing, because the change to browser.js means that any existing Preferences:Get code needs to be updated simultaneously. So really these patches are not independent, and need to land together.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update GeckoScreenOrientationListener

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Update gfx/ code

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Update ProfileMigrator

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Update testPasswordEncrypt

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 6 - Update ui/ code

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 7 - Update GeckoPreferences

Also FYI I pushed a try build with all of these patches to https://tbpl.mozilla.org/?tree=Try&rev=99a111a1d7ce

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Oops, my local incremental build didn't pick up the compile error in the test code. Updated locally and re-pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=3209cb676030

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 661297 [details] [diff] [review]
Part 1 - Add a GeckoPrefsHelper class and a requestId field to the JSON

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

Assuming a pref has a single expected type, the PrefHandler interface's prefValue() overrides seem like overkill. If you just call back with the Object extracted from JSON, the callback implementer can check the type and handle the error condition itself. With PrefHandler's current interface, if the caller expects prefValue(String), they would need to implement awkward error handling code in the prefValue(int) and prefValue(String) overrides.

This change would also allow you to simplify GeckoPrefsHandler's bool/int/string type checks.

::: mobile/android/base/GeckoPrefsHelper.java
@@ +13,5 @@
> +
> +import android.util.Log;
> +
> +import java.util.Map;
> +import java.util.HashMap;

'H' (HashMap) before 'M' (Map).

@@ +22,5 @@
> +public final class GeckoPrefsHelper {
> +    private static final String LOGTAG = "GeckoPrefsHelper";
> +
> +    private static boolean sRegistered = false;
> +    private static Map<Integer, PrefHandler> sCallbacks = new HashMap<Integer, PrefHandler>();

You can make sCallbacks final.

@@ +65,5 @@
> +            }
> +            return;
> +        }
> +
> +        GeckoAppShell.sendEventToGecko(event);

If you move sendEventToGecko() inside the try {} block, you can remove the return from the catch {} block. That would also handle the case of cleaning up sCallbacks if sendEventToGecko() threw an exception.

@@ +69,5 @@
> +        GeckoAppShell.sendEventToGecko(event);
> +    }
> +
> +    private static void ensureRegistered() {
> +        if (!sRegistered) {

If you bail early with `if (sRegistered) return;` then you can unindent this entire method.

@@ +109,5 @@
> +
> +    public static void setPref(String pref, Object value) {
> +        if (pref == null || pref.length() == 0) {
> +            return;
> +        }

Are these parameter checks necessary? I assume these conditions indicate bugs in the caller's code, so crashing below or throwing an IllegalArgumentException would be appropriate.

@@ +132,5 @@
> +            Log.e(LOGTAG, "Error setting pref [" + pref + "]", e);
> +        }
> +    }
> +
> +    public static interface PrefHandler {

static is redundant for interfaces.

@@ +135,5 @@
> +
> +    public static interface PrefHandler {
> +        public void prefValue(String pref, boolean value);
> +        public void prefValue(String pref, int value);
> +        public void prefValue(String pref, String value);

public is redundant for interfaces.

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 661298 [details] [diff] [review]
Part 2 - Update GeckoScreenOrientationListener

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

LGTM

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 661299 [details] [diff] [review]
Part 3 - Update gfx/ code

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

LGTM

::: mobile/android/base/gfx/PluginLayer.java
@@ +68,5 @@
>  
> +    static void initPrefs() {
> +        GeckoPrefsHelper.getPref(PREF_PLUGIN_USE_PLACEHOLDER, new GeckoPrefsHelper.PrefHandlerBase() {
> +            @Override public void prefValue(String pref, int value) {
> +                sUsePlaceholder = (value == 1 ? true : false);

You can just use `sUsePlaceholder = (value == 1)`. The `? true : false` is redundant.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 661300 [details] [diff] [review]
Part 4 - Update ProfileMigrator

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

LGTM

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 661301 [details] [diff] [review]
Part 5 - Update testPasswordEncrypt

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

LGTM

::: mobile/android/base/GeckoPrefsHelper.java
@@ +83,5 @@
> +                            }
> +                        }
> +                        if (callback == null) {
> +                            Log.d(LOGTAG, "Preferences:Data message had an unknown requestId; ignoring");
> +                            return;

Is this condition a bug? Maybe the logcat message should be "louder" and include an exception stack trace? Logging the unknown requestId might be helpful when trying to debug preference message bugs by reading logs.

Log.d(LOGTAG, "Preferences:Data message had an unknown requestId " + requestId, new NoSuchElementException());

Comment 14

[Chris Peterson [:cpeterson]]

Comment on attachment 661303 [details] [diff] [review]
Part 7 - Update GeckoPreferences

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

LGTM

Comment 15

[Chris Peterson [:cpeterson]]

Comment on attachment 661302 [details] [diff] [review]
Part 6 - Update ui/ code

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

LGTM

::: mobile/android/base/ui/Axis.java
@@ +12,5 @@
>  
>  import android.util.Log;
>  
>  import java.util.Map;
> +import java.util.HashMap;

'H' (HashMap) before 'M' (Map).

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson (:cpeterson) from comment #9)
> Comment on attachment 661297 [details] [diff] [review]
> Part 1 - Add a GeckoPrefsHelper class and a requestId field to the JSON

Also, if someone misspells a message name, does GeckoPrefsHelper get an opportunity to remove the requestId from sCallbacks? I realize this is a bug in the caller. Is there a way to identify those bugs and then log a warning or throw an exception?

Also, blassey wants to get away from so many files and classes prefixed with "Gecko". You might consider renaming GeckoPrefsHelper.java to omit the "Gecko" prefix, unless you think the Gecko prefix makes senses because this class is invoking Gecko code.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Peterson (:cpeterson) from comment #9)
> Assuming a pref has a single expected type, the PrefHandler interface's
> prefValue() overrides seem like overkill. If you just call back with the
> Object extracted from JSON, the callback implementer can check the type and
> handle the error condition itself. With PrefHandler's current interface, if
> the caller expects prefValue(String), they would need to implement awkward
> error handling code in the prefValue(int) and prefValue(String) overrides.
> 
> This change would also allow you to simplify GeckoPrefsHandler's
> bool/int/string type checks.
> 

I agree that this would simplify GeckoPrefHelper's type checks, but at the expense of making the caller code slightly more complicated. At best the callback would take a JSONObject instead of the (String, String|int|bool) it currently does, and that seems wrong to me - the JSON is an implementation detail that the callers shouldn't need to know about. Also in the case where the callers need to handle multiple prefs' of different types they'll basically need to replicate the code that I put in GeckoPrefsHelper and we'll end up with multiple copies of it.

For now I'm leaving this as-is but if you feel strongly about it I can re-evaluate.

> > +import java.util.Map;
> > +import java.util.HashMap;
> 
> 'H' (HashMap) before 'M' (Map).

Doh! Fixed.

> > +    private static boolean sRegistered = false;
> > +    private static Map<Integer, PrefHandler> sCallbacks = new HashMap<Integer, PrefHandler>();
> 
> You can make sCallbacks final.
> 

Good catch, fixed.

> If you move sendEventToGecko() inside the try {} block, you can remove the
> return from the catch {} block. That would also handle the case of cleaning
> up sCallbacks if sendEventToGecko() threw an exception.
> 

Ok. When I wrote that I thought this way would be better in the case where sendEventToGecko threw an exception because if that happened there's no knowing whether or not the message actually got through. But on reflection it's better to take out the callback and have a warning later than to leak it, so I've changed that.

> If you bail early with `if (sRegistered) return;` then you can unindent this
> entire method.

Fixed.

> > +    public static void setPref(String pref, Object value) {
> > +        if (pref == null || pref.length() == 0) {
> > +            return;
> > +        }
> 
> Are these parameter checks necessary? I assume these conditions indicate
> bugs in the caller's code, so crashing below or throwing an
> IllegalArgumentException would be appropriate.
> 

They were in the GeckoPreferences.java code that lifted out into GeckoPrefsHelper, so I left them in by default. However it looks like GeckoPreferences.onPreferenceChange is the only place where it might be null so I've changed it to throw an IAE and added a guard in GeckoPreferences.onPreferenceChange.

> > +    public static interface PrefHandler {
> 
> static is redundant for interfaces.
> 
> > +        public void prefValue(String pref, String value);
> 
> public is redundant for interfaces.

Doesn't really hurt to have them in (explicit is better than implicit, I think) but I took them out.

(In reply to Chris Peterson (:cpeterson) from comment #11)
> 
> You can just use `sUsePlaceholder = (value == 1)`. The `? true : false` is
> redundant.

Fixed.

(In reply to Chris Peterson (:cpeterson) from comment #13)
> > +                        if (callback == null) {
> > +                            Log.d(LOGTAG, "Preferences:Data message had an unknown requestId; ignoring");
> > +                            return;
> 
> Is this condition a bug? Maybe the logcat message should be "louder" and
> include an exception stack trace? Logging the unknown requestId might be
> helpful when trying to debug preference message bugs by reading logs.
> 
> Log.d(LOGTAG, "Preferences:Data message had an unknown requestId " +
> requestId, new NoSuchElementException());

It's not really a bug because there might be code (such as this test class) which send Preferences:Get messages directly without going through the GeckoPrefsHelper. In that case, GeckoPrefsHelper will still get notified of the resulting Preferences:Data even though it won't have a callback for it. Logging a stack trace here is unlikely to be of much help because it will always be exactly the same - GeckoAppShell.handleGeckoMessage calls EventDispatcher.dispatchMessage, which calls GeckoPrefsHelper$1.handleMessage, or something like that. Also one of the possible triggers for the error here is that the Preferences:Data object doesn't have a requestId property at all, so it might not be possible to print it out as part of the error message either.

(In reply to Chris Peterson (:cpeterson) from comment #15)
> 'H' (HashMap) before 'M' (Map).

Fixed.

(In reply to Chris Peterson (:cpeterson) from comment #16)
> (In reply to Chris Peterson (:cpeterson) from comment #9)
> > Comment on attachment 661297 [details] [diff] [review]
> > Part 1 - Add a GeckoPrefsHelper class and a requestId field to the JSON
> 
> Also, if someone misspells a message name, does GeckoPrefsHelper get an
> opportunity to remove the requestId from sCallbacks? I realize this is a bug
> in the caller. Is there a way to identify those bugs and then log a warning
> or throw an exception?

I assume by "message name" you mean pref name? In that case GeckoPrefsHelper doesn't need to do anything special. The Preferences:Get will get sent to browser.js as usual, and browser.js will try to read the pref, fail, catch the error and log it to logcat. The error that shows up in logcat looks something like this:

09-14 14:40:35.905 E/GeckoConsole(25698): Error reading pref [services.sync.account]: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/browser.js :: getPreferences :: line 837"  data: no]

However browser.js does still send back a Preferences:Data with the requestId so GeckoPrefsHelper will get it and remove the callback. So the code should be robust enough to handle this scenario already.

> Also, blassey wants to get away from so many files and classes prefixed with
> "Gecko". You might consider renaming GeckoPrefsHelper.java to omit the
> "Gecko" prefix, unless you think the Gecko prefix makes senses because this
> class is invoking Gecko code.

Good point, I dislike the "Gecko" prefix too. I've renamed it to PrefsHelper instead. New patches coming up.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add a PrefsHelper class and a requestId field to the JSON (v2)

Updated as per comments, and renamed to PrefsHelper

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update GeckoScreenOrientationListener

Rebased, carrying r+

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Update gfx/ code

Rebased and updated as per comments, carrying r+

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Update ProfileMigrator

Rebased, carrying r+

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Update testPasswordEncrypt

Rebased, carrying r+

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 6 - Update ui/ code

Updated as per comments, rebased, carrying r+

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 7 - Update GeckoPreferences

Rebased this one, and added an extra TextUtils.isEmpty(prefName) check around the call to setPref. This goes with the new IllegalArgumentException that is thrown in PrefsHelper.setPref if the name is null or empty. Carrying r+ although feel free to minus this if you don't like it.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Latest try run is at https://tbpl.mozilla.org/?tree=Try&rev=bc7885cd9014

Comment 26

[Chris Peterson [:cpeterson]]

Comment on attachment 661529 [details] [diff] [review]
Part 1 - Add a PrefsHelper class and a requestId field to the JSON (v2)

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

LGTM

::: mobile/android/base/PrefsHelper.java
@@ +146,5 @@
> +        public void prefValue(String pref, int value) {
> +        }
> +
> +        public void prefValue(String pref, String value) {
> +        }

Callers will subclass PrefHandlerBase and override the prevValue() method for the pref type they expect. I think PrefHandlerBase's default implementation of these methods should log a warning about a pref having an unexpected type. If PrefHandlerBase's methods are directly invoked, I believe that would be a bug (or at least a surprise).

Comment 27

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Peterson (:cpeterson) from comment #26)
> Callers will subclass PrefHandlerBase and override the prevValue() method
> for the pref type they expect. I think PrefHandlerBase's default
> implementation of these methods should log a warning about a pref having an
> unexpected type. 

That sounds like a good idea, I'll put that in.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Landed with Log.w calls in the prefValue implementations in PrefHandlerBase (also verified that the log lines don't currently show up in logcat, as expected).

All seven patches were squashed into a single commit:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05ca0508f22

Comment 29

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/c05ca0508f22