Closed Bug 922694 Opened 6 years ago Closed 6 years ago

Add partner ID / distribution ID and locales to FHR payload

Categories

(Firefox Health Report Graveyard :: Client: Android, defect)

All
Android
defect
Not set

Tracking

(firefox26+ fixed, firefox27 fixed, fennec26+)

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 + fixed
firefox27 --- fixed
fennec 26+ ---

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [qa+])

Attachments

(6 files, 6 obsolete files)

60.33 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
26.01 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
2.93 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
7.13 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
14.63 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
6.94 KB, patch
ckitching
: review+
Details | Diff | Splinter Review
Two/three new fields.
Bug 918465 is the bug for locales, but these should land as a single change.
Depends on: 918465
We want this in 26 for partner work. tracking 26?
that would be nice :)
tracking-fennec: --- → 26+
Attached patch Part 1: refactor. v1 (obsolete) — Splinter Review
This patch splits out distribution handling into an instance that records some (usually temporary) state, with Gecko messaging occurring at the static/wrapper level.

This will allow FHR to get access to the same computed data without launching Gecko, and makes the logic somewhat cleaner, giving us a point of extension to read more details from the distribution file.

I also reworked the logic to use the Scanner approach to file reads, added a 1K buffer for zip extraction (for jchen!) and otherwise simplified and tidied stuff.

I manually tested this with extra debug logging for the in-APK channel, and it (a) works, and (b) doesn't do any extra work on subsequent runs.

Next up is to hook up FHR.
I have not tested the /system/ packaging approach -- does someone have a rooted device to give that a shot?
Attachment #812902 - Flags: review?(margaret.leibovic)
Mostly for illustration at this point, 'cos I haven't tested it or written the consumer, but here's Part 2.
Attachment #812915 - Flags: review?(margaret.leibovic)
Additional code reuse.
Attachment #812915 - Attachment is obsolete: true
Attachment #812915 - Flags: review?(margaret.leibovic)
Attachment #812916 - Flags: review?(margaret.leibovic)
https://github.com/mozilla-services/android-sync/pull/363

-- part 3. Updated parts 1 & 2, and a part 4, following soon. Then tests, and making sure it actually works (which is not something I really want to do in an airport).
Flags: needinfo?(michael.l.comella)
Attached patch Part 3: from Git. (obsolete) — Splinter Review
Attached patch Part 4: WIP. v1 (obsolete) — Splinter Review
This builds, but isn't tested.
feedback on github PR.
Flags: needinfo?(michael.l.comella)
Comment on attachment 812902 [details] [diff] [review]
Part 1: refactor. v1

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

Looking good, I just have a few comments/questions to consider before landing.

I tested with a system distribution, and this works. Also tested with a system/APK combo, and the APK distribution won out (as it should).

::: mobile/android/base/Distribution.java
@@ +55,5 @@
>      /**
> +     * Use <code>Context.getPackageResourcePath</code> to find an implicit
> +     * package path.
> +     */
> +    public static void init(final Context context) {

As a follow-up, we could replace the Distirbution.init call in BrowserApp with this one, since that's what it's already doing.

@@ +65,5 @@
> +     * This method should only be called from a background thread.
> +     */
> +    public static JSONArray getBookmarks(final Context context) {
> +        Distribution dist = new Distribution(context);
> +        dist.doInit();

So, getBookmarks is called from BrowserProvider during DB creation. What thread does that run on? Is it the same background thread that we post to from the static init method up above? If two different threads call doInit in a racy way, can bad things happen?

You're not really changing the status quo here, but this would be a good time to think about edge cases we may not currently handle well.

@@ +66,5 @@
> +     */
> +    public static JSONArray getBookmarks(final Context context) {
> +        Distribution dist = new Distribution(context);
> +        dist.doInit();
> +        return dist.getBookmarks();

In dist.getBookmarks, you call doInit if it's necessary, so I don't think you need to include that call in this method.

@@ +84,5 @@
> +        this.packagePath = packagePath;
> +    }
> +
> +    public Distribution(final Context context) {
> +        this(context, context.getPackageResourcePath());

It's annoying that we need to have these two different constructors. This packagePath param really just exists so that we can pass a mock path from our robocop test. Maybe there's a way to only make a single entry point for this mock set-up.

@@ +92,5 @@
> +     * Don't call from the main thread.
> +     *
> +     * @return true if we've set a distribution.
> +     */
> +    public boolean doInit() {

Can this be private?

@@ +186,5 @@
>          zip.close();
>  
>          return distributionSet;
>      }
> +    

Whitespace.

@@ +237,2 @@
>          try {
> +            Scanner scanner = null;

I didn't know about Scanner. That's pretty handy.
Attachment #812902 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 812916 [details] [diff] [review]
Part 2: add accessor for distribution metadata. v2 (untested)

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

Looks good, assuming you find no problems when you actually start using this for something :)

::: mobile/android/base/Distribution.java
@@ +41,5 @@
> +        public final boolean valid;
> +        public final String id;
> +        public final String version;    // Example uses a float, but that's a crazy idea.
> +        public final String about;
> +        public final Map<String, String> localizedAbout;

I know what this means, but I think it would be good to add some documentation here about how localized versions of the "about" string can exist (also some documentation about what you're mapping to what in here).

@@ +54,5 @@
> +            while (keys.hasNext()) {
> +                String key = keys.next();
> +                if (key.startsWith("about.")) {
> +                    String locale = key.substring(6);
> +                    loc.put(locale, obj.optString(key, ""));

Won't there always be a value for this key, since you're iterating through the keys? (I also believe that optString defaults to returning an empty string if a value isn't found, so you shouldn't need to provide it as the default value.)
Attachment #812916 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #12)

> I know what this means, but I think it would be good to add some
> documentation here about how localized versions of the "about" string can
> exist (also some documentation about what you're mapping to what in here).

Good point.

> Won't there always be a value for this key, since you're iterating through
> the keys? 

There should be, yes; chalk this up to excessive caution.

What I ought to do instead is explicitly check for isNull here, 'cos .getString("somekeywithnullvalue") => "null", which is certainly not what we want.


> (I also believe that optString defaults to returning an empty
> string if a value isn't found, so you shouldn't need to provide it as the
> default value.)

Academic point, now, but: it does, but doing it this way (a) this avoids a method call (optString(String) calls through to optString(String, "")), and (b) it's self-documenting.
(In reply to :Margaret Leibovic from comment #11)

> So, getBookmarks is called from BrowserProvider during DB creation. What
> thread does that run on?

onCreate is called on the application main thread.

BrowserProvider shouldn't be doing *any* of the work it does in onCreate:

---
You should defer nontrivial initialization (such as opening, upgrading, and scanning databases) until the content provider is used (via query(Uri, String[], String, String[], String), insert(Uri, ContentValues), etc). Deferred initialization keeps application startup fast, avoids unnecessary work if the provider turns out not to be needed, and stops database errors (such as a full disk) from halting application launch. 
---

We'll be janking on first run, so we should file a follow-up to not screw around in onCreate regardless.

Given that this runs on the main thread, it should actually run before (or long after) any app code can spawn a background task to do Distribution stuff, and thus it's 'safe'. 

Note also that Distribution is no longer a singleton, so the only race here could be in copying files out of the zip file or talking to shared prefs, and the two instances will be doing the exact same work. Given that the contents will always be the same, that doesn't really concern me.

> In dist.getBookmarks, you call doInit if it's necessary, so I don't think
> you need to include that call in this method.

Good spot!
(In reply to Richard Newman [:rnewman] from comment #14)
> (In reply to :Margaret Leibovic from comment #11)
> 
> We'll be janking on first run, so we should file a follow-up to not screw
> around in onCreate regardless.

Spoke incorrectly. The onCreate I read is the DB helper for the provider, which is lazy.


> > So, getBookmarks is called from BrowserProvider during DB creation. What
> > thread does that run on?

Correct answer: the provider interaction that could race with this is all from about:home, which should run everything in AsyncTasks on the background thread. That'll include DB onCreate if needed.

If I'm wrong, then it's no worse than we have now.
Status update: I'm testing this now. Something is going awry in the background; unit tests and debug logging will find it.
This has been hand-tested. I'll be adding tests for the Git part tomorrow, and a QA plan for this part.

Feel free to treat this as a review if you wish.
Attachment #814228 - Attachment is obsolete: true
Attachment #815724 - Flags: feedback?(michael.l.comella)
Comment on attachment 815724 [details] [diff] [review]
Part 4: grab distribution ID and Accept-Locale pref in FHR. v1

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

The javadoc comment on BHRecorder.onEnvironmentChanged references removed method onBlocklistPrefChanged - update it.

Overall, looks pretty good.

::: mobile/android/base/GeckoApp.java
@@ +1288,5 @@
>                  final String profilePath = getProfile().getDir().getAbsolutePath();
>                  final EventDispatcher dispatcher = GeckoAppShell.getEventDispatcher();
>                  Log.i(LOGTAG, "Creating BrowserHealthRecorder.");
> +                final String osLocale = Locale.getDefault().toString();
> +                Log.d(LOGTAG, "Locale is " + osLocale);

nit: "OS locale is ..." or "Default locale is ..."

@@ +1293,5 @@
> +                mHealthRecorder = new BrowserHealthRecorder(GeckoApp.this,
> +                                                            profilePath,
> +                                                            dispatcher,
> +                                                            osLocale,
> +                                                            osLocale,    // Placeholder.

What is this a placeholder for? Until setAppLocale is called? I think it would help if this comment explained it.

Also, is this not null so the default app locale is initialized to the same value as the osLocale inside the database (if setAppLocale is not called)?

@@ +1558,5 @@
>  
>                  /*
> +                  XXXX see Bug 635342.
> +                  We want to disable this code if possible.  It is about 145ms in runtime.
> +                

Excess whitespace.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +508,4 @@
>              return;
>          }
> +
> +        // Otherwise, record the value.

nit: I assume this comment is supposed to line up with

> // We only record whether this is user-set

But it's at the wrong indentation level and, in my opinion, a bit confusing. I think the boolean prefs comment below alone would be sufficient.

@@ +512,5 @@
> +        // (We only handle boolean prefs right now.)
> +        try {
> +            boolean value = message.getBoolean("value");
> +
> +            if (AppConstants.TELEMETRY_PREF_NAME.equals(pref)) {

Like I mentioned in browser.js, perhaps this should be PREF_TELEMETRY_ENABLED.

@@ +607,5 @@
>          this.state = State.INITIALIZING;
>  
>          // If we can restore state from last time, great.
>          if (this.profileCache.restoreUnlessInitialized()) {
> +            this.profileCache.updateLocales(osLocale, appLocale);

updateLocales sets the 'needsWrite' flag but I do not believe 'writeToFile()' is called after this, since it is only called in 'completeInitialization', which is only called for the "Snapshot" request (which we wouldn't call), and onEnvironmentChanged, which the user may have no desire to call.

See https://mxr.mozilla.org/mozilla-central/search?string=writetofile&find=\.java%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

::: mobile/android/chrome/content/browser.js
@@ +5313,1 @@
>    TELEMETRY_PREF: 

I wonder if this shouldn't also be "PREF_TELEMETRY_ENABLED", like "PREF_BLOCKLIST_ENABLED".

@@ +5329,5 @@
>        console.log("Failed to initialize add-on status listener. FHR cannot report add-on state. " + ex);
>      }
>  
> +    console.log("Adding HealthReport:RequestSnapshot observer.");
> +    Services.obs.addObserver(this, "HealthReport:RequestSnapshot", false);

Is there any reason why "HealthReport:RequestSnapshot" is not a constant?

@@ +5369,5 @@
> +            sendMessageToJava({
> +              type: "Pref:Change",
> +              pref: aData,
> +              value: Services.prefs.getBoolPref(aData),
> +              isUserSet: Services.prefs.prefHasUserValue(aData),

nit: This JSON object could be constructed above the switch statement with the 'type', 'value', and 'isUserSet' keys, with the 'value' key set in each case statement, which would avoid a little code duplication.

@@ +5457,5 @@
>      this.notifyJava(aAddon);
>    },
>  
> +  sendSnapshotToJava: function () {
> +    console.log("GeckoXXX: Sending snapshot.");

Why "GeckoXXX"?
Attachment #815724 - Flags: feedback?(michael.l.comella) → feedback+
Attachment #814227 - Attachment is obsolete: true
Attachment #816956 - Flags: review+
Attachment #815724 - Attachment is obsolete: true
Attachment #816957 - Flags: review?(michael.l.comella)
Tested upgrade path by hand.
Attachment #816958 - Flags: review?(michael.l.comella)
Attachment #816958 - Flags: review?(michael.l.comella) → review+
Reuploading in case of bitrot.
Attachment #812916 - Attachment is obsolete: true
Attachment #817270 - Flags: review+
Reuploading in case of bitrot.
Attachment #812902 - Attachment is obsolete: true
Attachment #817272 - Flags: review+
Margaret, would you have time to check this patch stack against a system distribution? about:healthreport > raw data should show your distribution ID and version in the current environment.
Flags: needinfo?(margaret.leibovic)
Comment on attachment 816957 [details] [diff] [review]
Part 4: grab Accept-Locale pref in FHR. v1

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

r-, held up on the placeholder comment because I'm not sure I fully understand the interaction.

::: mobile/android/base/GeckoApp.java
@@ +1300,5 @@
> +                mHealthRecorder = new BrowserHealthRecorder(GeckoApp.this,
> +                                                            profilePath,
> +                                                            dispatcher,
> +                                                            osLocale,
> +                                                            osLocale,    // Placeholder.

Again, what is this a placeholder for? Until setAppLocale is called? Isn't it more of a default value then?

Also, is this not null so the default app locale is initialized to the same value as the osLocale inside the database (if setAppLocale is not called)?

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +510,5 @@
> +        // (We only handle boolean prefs right now.)
> +        try {
> +            boolean value = message.getBoolean("value");
> +
> +            if (AppConstants.TELEMETRY_PREF_NAME.equals(pref)) {

Did you not change this to `PREF_TELEMETRY_ENABLED` because it's used in too many other places?

::: mobile/android/chrome/content/browser.js
@@ +5313,5 @@
>  let HealthReportStatusListener = {
> +  PREF_ACCEPT_LANG: "intl.accept_languages",
> +  PREF_BLOCKLIST_ENABLED: "extensions.blocklist.enabled",
> +
> +  PREF_TELEMETRY_ENABLED: 

nit: end-of-line whitespace (though I'm not sure if it's necessary due to the preprocessing).
Attachment #816957 - Flags: review?(michael.l.comella) → review-
(In reply to Michael Comella (:mcomella) from comment #25)

> Again, what is this a placeholder for? Until setAppLocale is called? Isn't
> it more of a default value then?

See comment above:

+                // Replace the duplicate `osLocale` argument when we support switchable
+                // application locales.

It's a placeholder for a future, different value. Right now it's accurate, because the app locale is always the same as the OS locale. That means it's not a default -- it really is just holding the place for a future, different, value.


> Also, is this not null so the default app locale is initialized to the same
> value as the osLocale inside the database (if setAppLocale is not called)?

Yes -- because the app locale *is* the OS locale right now.
 

> Did you not change this to `PREF_TELEMETRY_ENABLED` because it's used in too
> many other places?

Yeah, I didn't want to touch AppConstants.
(In reply to Richard Newman [:rnewman] from comment #24)
> Margaret, would you have time to check this patch stack against a system
> distribution? about:healthreport > raw data should show your distribution ID
> and version in the current environment.

Yes, works with system distribution. I didn't see the distribution data in about:healthreport, but I did verify the distribution prefs were set correctly in about:config.
Flags: needinfo?(margaret.leibovic)
Note: Fennec as a whole doesn't support changing the distribution descriptor after first run. If that changes, we'll have to poke the environment when we spot the distro ID or version change.
14:02:41 < margaret> rnewman: okay, in about:healthreport is see "distribution": "mydistid:1.0"

Yay!
Comment on attachment 816957 [details] [diff] [review]
Part 4: grab Accept-Locale pref in FHR. v1

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

(In reply to Richard Newman [:rnewman] from comment #26)
> See comment above:
> 
> +                // Replace the duplicate `osLocale` argument when we
> support switchable
> +                // application locales.

Totally missed that - r+.
Attachment #816957 - Flags: review- → review+
Duplicate of this bug: 918465
Docs pushed:

Sending        _sources/healthreport/index.txt
Sending        _sources/howtos/run-sync.txt
Sending        healthreport/index.html
Sending        howtos/run-sync.html
Sending        objects.inv
Sending        searchindex.js
Transmitting file data ......
Committed revision 121258.

Committed revision 121259.

Committed revision 121260.
Whiteboard: [fixed in fx-team][qa+]
Once again, I really wish Robocop tests actually ran on any machine/phone combo that I have in my possession.
Got them working!

This explains it:

10-15 19:35:36.726 D/Robocop ( 3322): waiting for Distribution:Set:OK
10-15 19:35:36.746 I/GeckoDistribution( 3322): No distribution: not broadcasting Distribution:Set.

For some reason, sometimes our test runs when we've cached STATE_NONE.

That's probably because we have *just* started up:

10-15 19:35:32.141 I/GeckoDistribution( 3322): Getting file from distribution.
10-15 19:35:32.141 I/GeckoDistribution( 3322): No distribution: not broadcasting Distribution:Set.

and so the test is racing with the startup fetch.

Oh, tests.
That'll teach you.
Attachment #818182 - Flags: review?(chriskitching)
Comment on attachment 818182 [details] [diff] [review]
Part 6: avoid test racing. v1

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

This patch seems to not be broken in a way I can detect at 0300.

I propose two minor improvements, one of which is potentially pointless, the other is unrelated to this bug.

Seems to do the job. I have not actually run the offending test to see if it no longer explodes.

::: mobile/android/base/Distribution.java
@@ -119,2 @@
>      private final String packagePath;
> -    private final Context context;

It would not be impossible to remove this line from the diff and save one line of blame. (By ceasing to move it up two lines).

::: mobile/android/base/tests/testDistribution.java.in
@@ -102,5 @@
>          try {
>              // Call Distribution.init with the mock package.
>              ClassLoader classLoader = mActivity.getClassLoader();
>              Class distributionClass = classLoader.loadClass("org.mozilla.gecko.Distribution");
>              Class contextClass = classLoader.loadClass("android.content.Context");

Not your code, but if the Robocop tests are built with the android libraries in scope (They *must* be) then this line can, I believe, be replaced with:
Class contextClass = android.context.Context.class;

Or, with a suitable import, just Context.class

For a minor performance gain.

That, or I'm hallucinating.
Attachment #818182 - Flags: review?(chriskitching) → review+
Sorry for a comment only now, but why is accept-locale being used here? We usually use the useragent.locale pref to determine the locale of a build as people have UI to change accept-locale however they want in desktop. But I have no idea what the intention here is.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #41)
> Sorry for a comment only now, but why is accept-locale being used here? We
> usually use the useragent.locale pref to determine the locale of a build as
> people have UI to change accept-locale however they want in desktop. But I
> have no idea what the intention here is.

We want to capture three things:

* The locale of the device. Users can change this. It's fetched from the OS.
* The locale used for Firefox itself. They can't change this right now, but will soon. (Bug 917480) Again, this is a Java thing.
* Whether the user has made an election for accept-language -- that is, whether they want to view the web in a language other than the language they're using for Firefox. This is a less privacy-impacting way of seeing the impact of Bug 881510.

As far as I understand it, useragent.locale is the equivalent of the second point, but for XUL applications; it doesn't apply to Firefox on Android.

Does that make sense?

(We will probably need to refine the way in which our accept-language probe works as we implement Bug 881510, of course, but I don't think that's pertinent to your question.)
That makes sense.

Note: useragent.locale is random, so we actually stopped using it. We're using selectedLocale of the 'global' package instead for chrome:// apps.
needinfo'ing myself to assess for uplift.
Flags: needinfo?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #42)
> Does that make sense?

Yes, makes sense (though AFAIK we made useragent.locale as well as the selectedLocale stuff Axel mentions do the right thing even on Android so that stuff works cross-product).

Just FYI, I think we might need a little bit more process and scrutiny in the future on adding things to FHR given the area between user benefit, privacy and data analysis that all this is playing in - but from what I heard we don't have a lot set up there yet (I'm still pretty new to the area of FHR and still need to catch up on everything) and this has already landed anyhow.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #46)

> Yes, makes sense (though AFAIK we made useragent.locale as well as the
> selectedLocale stuff Axel mentions do the right thing even on Android so
> that stuff works cross-product).

So it does. Magic!

Can you give a pointer to how that works (ideally in Bug 917480)? I don't want us to break it when we start modifying the application Locale.

(Note that we still don't like using Gecko prefs for this kind of thing, because it's a pain in the ass to get to them from Java, where most of our code runs. Gecko is just a rendering engine, and it's super expensive to launch in order to read a value you need in most situations.)

> Just FYI, I think we might need a little bit more process and scrutiny in
> the future on adding things to FHR given the area between user benefit,
> privacy and data analysis that all this is playing in - but from what I
> heard we don't have a lot set up there yet (I'm still pretty new to the area
> of FHR and still need to catch up on everything) and this has already landed
> anyhow.

CCing mconnor so he can speak to the current process that this and other FHR-related changes have gone through. (Or feel free to sync up with him and jjensen in private.)
(In reply to Richard Newman [:rnewman] from comment #47)
> Can you give a pointer to how that works (ideally in Bug 917480)? I don't
> want us to break it when we start modifying the application Locale.

I'm not sure, Axel knows more about this - I only learned that this is true by talking to him myself. :)


> CCing mconnor so he can speak to the current process that this and other
> FHR-related changes have gone through. (Or feel free to sync up with him and
> jjensen in private.)

I have a meeting with jjensen set up anyhow an I will talk to more people in the FHR area in the next weeks. Let's take this off this bug, I didn't see anywhere here what process this would have gone through or not, and I was mostly curious about what's going on, including that. Would have been nice to see that documented here but Bugzilla is the wrong forum to discuss this.
Comment on attachment 817272 [details] [diff] [review]
Part 1: refactor. v2

No fallout on Nightly, so requesting uplift.

*** This approval request applies to all work done in this bug. ***

I'm too lazy to click seventy times in Bugzilla.

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

User impact if declined: 
  Unable to use locales or distribution information in delivering FHR benefits.

Testing completed (on m-c, etc.): 
  Baking in Nightlies for a while. Manually tested both upgrade and non-upgrade paths.

Risk to taking this patch (and alternatives if risky): 
  This is primarily additive and thorough, but this is more risky than my usual uplift requests: it alters an on-disk format (through a well-exercised mechanism, at least) and adds DB columns.

String or IDL/UUID changes made by this patch:
  None.
Attachment #817272 - Flags: approval-mozilla-aurora?
Comment on attachment 817272 [details] [diff] [review]
Part 1: refactor. v2

and I'm fine with not having to individually approve a ton of patches - rollup a+ is fine.
Attachment #817272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.