808219 - Firefox Health Report XPCOM service

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

Attachment: FHR service, v1

Forking bug 718067 to track the implementation of the XPCOM service that will provide the Firefox Health Report feature.

The patch is carried over from bug 718067. No new review is necessary as it hasn't been updated.

Comment 1

[Gregory Szorc [:gps]]

Posting original review from bug 718067.

(In reply to Richard Newman [:rnewman] from comment #126)
> Comment on attachment 676896 [details] [diff] [review]
> Part 5: HealthReporter service, v1
> 
> Review of attachment 676896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/healthreport/healthreporter.jsm
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +"use strict";
> > +
> > +this.EXPORTED_SYMBOLS = ["FirefoxHealthReporter"];
> 
> I'm probably not the person to make this determination, but is there a
> reason to call this FirefoxHealthReporter rather than HealthReporter?
> 
> @@ +16,5 @@
> > +Cu.import("resource://gre/modules/services/metrics/manager.jsm");
> > +Cu.import("resource://gre/modules/services/healthreport/policy.jsm");
> > +
> > +
> > +this.FirefoxHealthReporter = function FirefoxHealthReporter(branch) {
> 
> I'm guessing this should come with a pretty thorough comment explaining what
> it's for.
> 
> Here's my guess from reading the code: this is the desktop browser
> implementation of a coordinator, smushing together the roles of data
> aggregation (talking to the collector to grab results), serialization
> (turning results into JSON), and submission (Bagheera upload).
> 
> On Android we'd have a different implementation, one which would aggregate,
> serialize, and not upload.
> 
> Is that right?
> 
> @@ +51,5 @@
> > +    return CommonUtils.getDatePref(this._prefs, "lastPingTime", 0, this, 2012);
> > +  },
> > +
> > +  set lastPingDate(value) {
> > +    CommonUtils.setDatePref(this._prefs,"lastPingTime", value,
> 
> Unwrap, and space after comma.
> 
> @@ +160,5 @@
> > +    for (let [name, provider] of this._collector.collectionResults) {
> > +      o.providers[name] = provider;
> > +    }
> > +
> > +    return JSON.stringify(o);
> 
> It feels a little wrong to me for this entity to be so intimately aware of
> the format of collection results, and their serialization. Where's the
> separation for us to introduce other submitters that don't call into
> JavaScript? I guess this ties into my overarching question above.
> 
> I would rather see the policy handler chain together an aggregator (which is
> writing out a chunk of JSON on command) and an uploader (which is taking a
> 'recipe' -- a payload, a MIME type, and a destination -- and sending the
> content somewhere).
> 
> You might be envisioning the upload as something so trivial that it doesn't
> need its own component, and so this is just the Firefox version of the
> aggregator+uploader combined, but that does imply that we're doing a bunch
> of redundant 'pull' work if the upload fails, no?
> 
> @@ +167,5 @@
> > +  //-----------------------------
> > +  // HealthReportPolicy listeners
> > +  //-----------------------------
> > +
> > +  onRequestDataSubmission: function onRequestDataSubmission(request) {
> 
> This might be a little too expedient.
> 
> Doesn't this conflate the creation of a snapshot document and uploading it?
> On desktop it's best to conflate the two for minor efficiency reasons.
> 
> On mobile it's not -- the lifecycles of the component which can write the
> document (Fennec) and the component which can upload it (the background
> service) are not likely to overlap. On mobile we want to write out the
> document as soon as we can after our policy-determined period expires, and
> upload it whenever we have background data capability.
> 
> I wouldn't ordinarily object, but this is really an issue with the policy.
> Is this API call lenient enough to support this separation?

Comment 2

[Gregory Szorc [:gps]]

Attachment: FHR service, v2

Latest code. Still has some issues and horrible test coverage. Just throwing something out there.

Comment 3

[Gregory Szorc [:gps]]

Attachment: FHR service, v3

I'm pretty happy with this. The test coverage /could/ be improved. But, any additions at this point are somewhat redundant with existing unit tests in the lower-level modules. I'm open to suggestions on specific areas where we'd want redundancy.

rnewman gets main code review. gavin gets the sign-off on XPCOM service registration and whatever code he wants to look at.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 679794 [details] [diff] [review]
FHR service, v3

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

I'm pretty happy with this, but will give it another run through tonight.

::: services/healthreport/HealthReportService.js
@@ +56,5 @@
> +          notify: function notify() {
> +            timer.cancel();
> +            this._instantiate();
> +          }.bind(this),
> +        }, 10000, timer.TYPE_ONE_SHOT);

Lift out constant.

@@ +81,5 @@
> +    Cu.import("resource://services-common/log4moz.js", ns);
> +    Cu.import("resource://gre/modules/services/healthreport/healthreporter.jsm", ns);
> +
> +    // How many times will we rewrite this code before rolling it up into a
> +    // generic module?

Refer to the bug you filed.

@@ +89,5 @@
> +      "Services.Metrics",
> +      "Services.BagheeraClient",
> +    ];
> +
> +    let prefs = new Preferences("healthreport.logging.");

This, and the subsequent use of "healthreport.", should be (a) related, and (b) parameterized or constantized.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 679794 [details] [diff] [review]
FHR service, v3

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

Over to Gavin.

::: services/healthreport/HealthReportService.js
@@ +21,5 @@
> + */
> +this.HealthReportService = function HealthReportService() {
> +  this.wrappedJSObject = this;
> +
> +  this.prefs = new Preferences("healthreport.");

Parameterize.

Comment 6

[Gregory Szorc [:gps]]

Attachment: FHR service, v4

Added ability to request remote data deletion through public API. Also increased test coverage.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 680153 [details] [diff] [review]
FHR service, v4

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

::: services/healthreport/healthreporter.jsm
@@ +140,5 @@
> +    if (!value) {
> +      value = "";
> +    }
> +
> +    this._prefs.set("lastSubmitID", value);

What's wrong with 

  value || ""

? :)

Comment 8

[Gregory Szorc [:gps]]

Attachment: Part 2: Follow-up to facilitate UX features, v1

I needed to add a few features to facilitate the UI features.

1) .reporter is now a lazy getter. This fixes issues where someone else requests the instance before it has been loaded by the timer. e.g. the prefs pane is loaded within 10s of the application start.

2) There are new APIs on reporter for querying and manipulating policy acceptance state. I don't believe in direct pref manipulation by 3rd party code because I feel that prefs should be private to the implementation (hidden implementation detail, loose coupling, etc). These APIs provide a proper interface.

I'll probably fold this into the main patch before checkin.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 680251 [details] [diff] [review]
Part 2: Follow-up to facilitate UX features, v1

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

::: services/healthreport/HealthReportService.js
@@ +62,5 @@
>          timer.initWithCallback({
>            notify: function notify() {
>              timer.cancel();
> +
> +            // Side-effect of access will instantiate if missing.

// Side effect: instantiates the reporter instance if not already accessed.

::: services/healthreport/healthreporter.jsm
@@ +270,5 @@
> +
> +  /**
> +   * Whether the data submission policy has been accepted.
> +   *
> +   * If this is true, the data will be submitted unless one of the kill

s/the data/health data

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 680153 [details] [diff] [review]
FHR service, v4

The policy/collector/service split seems very reasonable from a quick look-over. You and rnewman have thought about this more than I have so I don't feel a need to second-guess the design (and don't foresee having the time to really dive into which abstractions make sense).

Some very quick-skim nit-picky comments:
- some of the default prefs in healthreport-prefs.js seem to have the wrong type (e.g. those with values that are "0" as a string). Do all these prefs really need to have default values?
- I don't fully understand the policy.jsm changes yet, but you're introducing some "function doesn't always return a value" strict warnings, which looks wrong.

HealthReportComponents.manifest:
- Do the "application" specifiers serve any useful purpose? What apps are protected by them?

HealthReportService:
- I don't like the "wait 10 seconds random timer, but I guess I don't have a better suggestion. We need bug 692420. You shouldn't need to cancel a one-shot timer when it fires.
- is quit-application really the right topic (as opposed to quit-application-granted)? I'm not sure.

HealthReporter:
- why is _now needed, can't you just use Date.now()?

Comment 11

[Gregory Szorc [:gps]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> Some very quick-skim nit-picky comments:
> - some of the default prefs in healthreport-prefs.js seem to have the wrong
> type (e.g. those with values that are "0" as a string). Do all these prefs
> really need to have default values?

Do they all need default values? No. But, I like having my features to be explicit about the preferences they use and thus don't really care for hidden preferences.

"0" is be design. These hold milliseconds since UNIX epoch. Integer values overflow the pref since we store int prefs in 32 bits.

> - I don't fully understand the policy.jsm changes yet, but you're
> introducing some "function doesn't always return a value" strict warnings,
> which looks wrong.

I have "use strict" and am not seeing these warnings. And, I know I have test coverage of these functions. I want my xpcshell tests to fail if I have any "strict warnings." How do I enable that?
 
> HealthReportComponents.manifest:
> - Do the "application" specifiers serve any useful purpose? What apps are
> protected by them?

We probably don't want the service running for XULRunner. We will also initially roll out the service to only B2G (at least on 18).

The data providers (via category manager registration) will also require per-app specifiers since the provider set differs between application.
 
> HealthReportService:
> You shouldn't need to cancel a one-shot timer when it fires.

Will change.

> - is quit-application really the right topic (as opposed to
> quit-application-granted)? I'm not sure.

bsmedberg?

> - why is _now needed, can't you just use Date.now()?

Unit testing. It's much safer to muck about with a single object's instance variable than with the global Date object.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gregory Szorc [:gps] from comment #11)
> I have "use strict" and am not seeing these warnings. And, I know I have
> test coverage of these functions. I want my xpcshell tests to fail if I have
> any "strict warnings." How do I enable that?

There are warnings that show up when you have javascript.options.strict and check the error console - unrelated to the new JS "strict mode" stuff. I don't think we have test harness support for tracking them, and they can often have a low signal/noise ratio, so probably not the kind of thing we want to invest much in tracking globally.

Comment 13

[Gregory Szorc [:gps]]

You just told me of a new signal I can use to track the quality of the JS I write - of course I want the ability to capture it! I filed bug 811078 to set up the xpcshell test runner to do this universally. We'll see what happens.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/larch/rev/358b90b79ae3

Comment 15

[Gregory Szorc [:gps]]

Attachment: Part 3: Add timer reference, v1

We need to hold a reference to the nsITimer. Otherwise, it might be GC'd and never fire. In related news, an active timer can get garbage collected?!

Comment 16

[Richard Newman [:rnewman]]

Timer: http://hg.mozilla.org/projects/larch/rev/8344f18f87c9

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/larch/rev/664be25a2e2d

I self-reviewed a patch to package the default prefs .js file in the installer. This was causing some mochitest failures.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 680153 [details] [diff] [review]
FHR service, v4

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).

Comment 19

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f544baffff0

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f544baffff0

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 680153 [details] [diff] [review]
FHR service, v4

FHR for B2G ADU ping, won't be built/enabled for Mobile/Desktop.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d3cec2f1e95
https://hg.mozilla.org/releases/mozilla-beta/rev/ff1fb69423fd

(Did everything land here that was supposed to?)