Firefox Health Report XPCOM service

RESOLVED FIXED in Firefox 18

Status

Firefox Health Report
Client: Desktop
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed, firefox20 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 677916 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Blocks: 788894
(Assignee)

Updated

5 years ago
Depends on: 802914
(Assignee)

Updated

5 years ago
Depends on: 804491
(Assignee)

Comment 1

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 809089
(Assignee)

Comment 2

5 years ago
Created attachment 679445 [details] [diff] [review]
FHR service, v2

Latest code. Still has some issues and horrible test coverage. Just throwing something out there.
Assignee: nobody → gps
Attachment #677916 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 679794 [details] [diff] [review]
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.
Attachment #679445 - Attachment is obsolete: true
Attachment #679794 - Flags: review?(rnewman)
Attachment #679794 - Flags: review?(gavin.sharp)
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.
Attachment #679794 - Flags: feedback+
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.
Attachment #679794 - Flags: review?(rnewman)
Attachment #679794 - Flags: review+
Attachment #679794 - Flags: feedback+
(Assignee)

Updated

5 years ago
Depends on: 810132
(Assignee)

Comment 6

5 years ago
Created attachment 680153 [details] [diff] [review]
FHR service, v4

Added ability to request remote data deletion through public API. Also increased test coverage.
Attachment #679794 - Attachment is obsolete: true
Attachment #679794 - Flags: review?(gavin.sharp)
Attachment #680153 - Flags: review?(rnewman)
Attachment #680153 - Flags: review?(gavin.sharp)
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 || ""

? :)
Attachment #680153 - Flags: review?(rnewman) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 680251 [details] [diff] [review]
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.
Attachment #680251 - Flags: review?(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
Attachment #680251 - Flags: review?(rnewman) → review+
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()?
Attachment #680153 - Flags: review?(gavin.sharp) → feedback+
(Assignee)

Comment 11

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Flags: needinfo?(benjamin)
(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.
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/projects/larch/rev/358b90b79ae3
Flags: needinfo?(benjamin)
Whiteboard: [fixed-in-larch]
(Assignee)

Comment 15

5 years ago
Created attachment 681261 [details] [diff] [review]
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?!
Attachment #681261 - Flags: review?(rnewman)
Attachment #681261 - Flags: review?(rnewman) → review+
Timer: http://hg.mozilla.org/projects/larch/rev/8344f18f87c9
(Assignee)

Comment 17

5 years ago
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 on attachment 680153 [details] [diff] [review]
FHR service, v4

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).
Attachment #680153 - Flags: approval-mozilla-beta?
Attachment #680153 - Flags: approval-mozilla-aurora?
Attachment #680251 - Flags: approval-mozilla-beta?
Attachment #680251 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f544baffff0
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/6f544baffff0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 813844
Comment on attachment 680153 [details] [diff] [review]
FHR service, v4

FHR for B2G ADU ping, won't be built/enabled for Mobile/Desktop.
Attachment #680153 - Flags: approval-mozilla-beta?
Attachment #680153 - Flags: approval-mozilla-beta+
Attachment #680153 - Flags: approval-mozilla-aurora?
Attachment #680153 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Attachment #680251 - Flags: approval-mozilla-beta?
Attachment #680251 - Flags: approval-mozilla-beta+
Attachment #680251 - Flags: approval-mozilla-aurora?
Attachment #680251 - Flags: approval-mozilla-aurora+
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?)
status-firefox18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
Whiteboard: [fixed-in-larch]
No longer blocks: 788894

Updated

5 years ago
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla20 → Firefox 20
You need to log in before you can comment on or make changes to this bug.