876473 - Connect generated health report to about:healthreport

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

On Android, we have an about:healthreport page waiting (Bug 857419) and we're fleshing out the generated report (Dependencies of Bug 858742).  This ticket will track connecting the generated report to about:healthreport.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 755489 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

So, this ended up entirely Fennec-side, and I think it's better for it.  There is one TODO to follow up on when Bug 828654 lands.  It's hard to say it's working since the FHR site doesn't provide much in the way of diagnostics.

Comment 3

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=6e64ed99a300

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 755489 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

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

::: mobile/android/base/health/BrowserHealthReporter.java
@@ +71,5 @@
> +        HealthReportDatabases databases = new HealthReportDatabases(context);
> +        try {
> +            // Closed by closing databases in finally block.
> +            HealthReportDatabaseStorage storage = databases.getDatabaseHelperForProfile(profile.getDir());
> +            HealthReportGenerator generator = new HealthReportGenerator(storage);

Don't do this. You'll open a new SQLiteDatabase pointing to the same DB.

Do what BrowserHealthRecorder does, and fetch it from the ContentProvider (using the CP as a lifecycle hack):

---
        this.client = EnvironmentBuilder.getContentProviderClient(context);
        if (this.client == null) {
            throw new IllegalStateException("Could not fetch Health Report content provider.");
        }

        this.storage = EnvironmentBuilder.getStorage(this.client, profilePath);
---

It would also make sense, then, to make this an inner class of BrowserHealthRecorder, or otherwise share that handle.

@@ +88,5 @@
> +     * This method performs IO, so call it from a background thread.
> +     */
> +    public JSONObject generateReport() throws JSONException {
> +        long since = System.currentTimeMillis() - MILLISECONDS_PER_SIX_MONTHS;
> +        long lastPingTime = since; // TODO: read this from SharedPreference owned by background uploader.

final SharedPreferences preferences = context.getSharedPreferences(AnnouncementsConstants.PREFS_BRANCH, GlobalConstants.SHARED_PREFERENCES_MODE);
preferences.getLong(AnnouncementsConstants.PREF_LAST_LAUNCH, -1);

::: mobile/android/chrome/content/aboutHealthReport.js
@@ +18,4 @@
>  // Name of Gecko Pref specifying report content location.
>  const PREF_REPORTURL = "datareporting.healthreport.about.reportUrl";
>  
> +function dump(s) {

Deliberately overriding dump()?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #4)
> Comment on attachment 755489 [details] [diff] [review]
> Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman
> 
> Review of attachment 755489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/health/BrowserHealthReporter.java
> @@ +71,5 @@
> > +        HealthReportDatabases databases = new HealthReportDatabases(context);
> > +        try {
> > +            // Closed by closing databases in finally block.
> > +            HealthReportDatabaseStorage storage = databases.getDatabaseHelperForProfile(profile.getDir());
> > +            HealthReportGenerator generator = new HealthReportGenerator(storage);
> 
> Don't do this. You'll open a new SQLiteDatabase pointing to the same DB.
> 
> Do what BrowserHealthRecorder does, and fetch it from the ContentProvider
> (using the CP as a lifecycle hack):
> 
> ---
>         this.client = EnvironmentBuilder.getContentProviderClient(context);
>         if (this.client == null) {
>             throw new IllegalStateException("Could not fetch Health Report
> content provider.");
>         }
> 
>         this.storage = EnvironmentBuilder.getStorage(this.client,
> profilePath);
> ---
> 
> It would also make sense, then, to make this an inner class of
> BrowserHealthRecorder, or otherwise share that handle.
> 
> @@ +88,5 @@
> > +     * This method performs IO, so call it from a background thread.
> > +     */
> > +    public JSONObject generateReport() throws JSONException {
> > +        long since = System.currentTimeMillis() - MILLISECONDS_PER_SIX_MONTHS;
> > +        long lastPingTime = since; // TODO: read this from SharedPreference owned by background uploader.
> 
> final SharedPreferences preferences =
> context.getSharedPreferences(AnnouncementsConstants.PREFS_BRANCH,
> GlobalConstants.SHARED_PREFERENCES_MODE);
> preferences.getLong(AnnouncementsConstants.PREF_LAST_LAUNCH, -1);
> 
> ::: mobile/android/chrome/content/aboutHealthReport.js
> @@ +18,4 @@
> >  // Name of Gecko Pref specifying report content location.
> >  const PREF_REPORTURL = "datareporting.healthreport.about.reportUrl";
> >  
> > +function dump(s) {
> 
> Deliberately overriding dump()?

I've never been clear on what's defined where.  This follows about:reader's lead.  Don't really care one way or the other -- do you prefer console.log, or nothing, or ADB only messages?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Richard Newman [:rnewman] from comment #4)
> > Comment on attachment 755489 [details] [diff] [review]
> > Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman
> > 
> > Review of attachment 755489 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/health/BrowserHealthReporter.java
> > @@ +71,5 @@
> > > +        HealthReportDatabases databases = new HealthReportDatabases(context);
> > > +        try {
> > > +            // Closed by closing databases in finally block.
> > > +            HealthReportDatabaseStorage storage = databases.getDatabaseHelperForProfile(profile.getDir());
> > > +            HealthReportGenerator generator = new HealthReportGenerator(storage);
> > 
> > Don't do this. You'll open a new SQLiteDatabase pointing to the same DB.
> > 
> > Do what BrowserHealthRecorder does, and fetch it from the ContentProvider
> > (using the CP as a lifecycle hack):
> > 
> > ---
> >         this.client = EnvironmentBuilder.getContentProviderClient(context);
> >         if (this.client == null) {
> >             throw new IllegalStateException("Could not fetch Health Report
> > content provider.");
> >         }
> > 
> >         this.storage = EnvironmentBuilder.getStorage(this.client,
> > profilePath);
> > ---
> > 
> > It would also make sense, then, to make this an inner class of
> > BrowserHealthRecorder, or otherwise share that handle.

So I didn't fold this into Recorder because I explicitly don't want to have to manage the life cycle: the only way to share the handle safely is to synchronize on the Recorder instance and handle all the different states.  Seems more complicated than necessary.

If I use getContentProviderClient and getStorage, is that cached enough to do each request?

Comment 7

[Nick Alexander :nalexander [he/him]]

> > +        long lastPingTime = since; // TODO: read this from SharedPreference owned by background uploader.
> 
> final SharedPreferences preferences =
> context.getSharedPreferences(AnnouncementsConstants.PREFS_BRANCH,
> GlobalConstants.SHARED_PREFERENCES_MODE);
> preferences.getLong(AnnouncementsConstants.PREF_LAST_LAUNCH, -1);

I thought lastPingTime was the last time we uploaded a document to the server, not anything about the last time the browser was launched.  A quick look at

http://mxr.mozilla.org/mozilla-central/source/services/healthreport/healthreporter.jsm#131

suggests I'm right.  Was your suggestion just an example?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #7)

> suggests I'm right.  Was your suggestion just an example?

Hurrr. I read lastLaunch, not lastPingTime!

(In reply to Nick Alexander :nalexander from comment #6)

> If I use getContentProviderClient and getStorage, is that cached enough to
> do each request?

So long as you hold on to the CPC, Android won't kill it. You can share the storage object safely. You'll get the same CPC (and thus the same CP and storage object) that BrowserHealthRecorder is using, and it might even stick around between activities.

It's strictly more cached than what you're doing in this patch (which is hard-closing everything each time you generate!), so yes, safe to do each request!

(In reply to Nick Alexander :nalexander from comment #5)

> > > +function dump(s) {
> > 
> > Deliberately overriding dump()?
> 
> I've never been clear on what's defined where.  This follows about:reader's
> lead.  Don't really care one way or the other -- do you prefer console.log,
> or nothing, or ADB only messages?

I was under the assumption that dump() was always defined, but perhaps not. If I had the option, I'd go for console.log, but nbd.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

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

Is this life-cycle right?  I'd really rather not persist the storage object if I don't have to.  I expect this to be called very infrequently, and not at all for most sessions.  Just realized there may be unneeded imports; I will trim imports before landing.

Comment 11

[Nick Alexander :nalexander [he/him]]

Just realized there may be unneeded imports; I
> will trim imports before landing.

Just one:

/Users/ncalexan/Mozilla/mcgit/mobile/android/base/health/BrowserHealthReporter.java:17:8: Unused import - org.mozilla.gecko.background.healthreport.HealthReportDatabases.

Comment 12

[Richard Newman [:rnewman]]

Btw, I should probably land this on top of my queue...

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

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

I will clean this up and add it to my queue for landing.

::: mobile/android/base/health/BrowserHealthReporter.java
@@ +47,5 @@
> +    // Health Report throughout the browser.  The Provider owns all underlying
> +    // Storage instances, so we don't need to explicitly close them.  Since the
> +    // Provider caches aggressively, we should even reference the same Storage
> +    // instances that BrowserHealthRecorder references and not waste resources.
> +    private ContentProviderClient client;

I would actually go so far as to fetch this within generateReport.

::: mobile/android/chrome/content/aboutHealthReport.js
@@ +23,5 @@
>      .getService(Ci.nsIAndroidBridge)
>      .handleGeckoMessage(JSON.stringify(message));
>  }
>  
> +// about:healthreport prefs are stored Firefox's default Android

stored in.

@@ +35,5 @@
>      let report = this._getReportURI();
>      iframe.src = report.spec;
>  
>      sharedPrefs.addObserver(PREF_UPLOAD_ENABLED, this, false);
> +    Services.obs.addObserver(this, "HealthReport:Response", false);

const.

@@ +98,5 @@
>  
>    injectData: function (type, content) {
>      let report = this._getReportURI();
>  
> +    // File URIs can't be used for targetOrigin, so we use "*" for

Should be "file:"

Comment 14

[Richard Newman [:rnewman]]

Marking as assigned to me; I'll switch back when landing.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #13)
> Comment on attachment 755638 [details] [diff] [review]
> Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman
> 
> Review of attachment 755638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I will clean this up and add it to my queue for landing.

Don't forget that unused import.

> ::: mobile/android/base/health/BrowserHealthReporter.java
> @@ +47,5 @@
> > +    // Health Report throughout the browser.  The Provider owns all underlying
> > +    // Storage instances, so we don't need to explicitly close them.  Since the
> > +    // Provider caches aggressively, we should even reference the same Storage
> > +    // instances that BrowserHealthRecorder references and not waste resources.
> > +    private ContentProviderClient client;
> 
> I would actually go so far as to fetch this within generateReport.

You know best.
 
> ::: mobile/android/chrome/content/aboutHealthReport.js
> @@ +23,5 @@
> >      .getService(Ci.nsIAndroidBridge)
> >      .handleGeckoMessage(JSON.stringify(message));
> >  }
> >  
> > +// about:healthreport prefs are stored Firefox's default Android
> 
> stored in.
> 
> @@ +35,5 @@
> >      let report = this._getReportURI();
> >      iframe.src = report.spec;
> >  
> >      sharedPrefs.addObserver(PREF_UPLOAD_ENABLED, this, false);
> > +    Services.obs.addObserver(this, "HealthReport:Response", false);
> 
> const.

Maybe take the names from the .java file, so mxr searches are as helpful as possible.

> @@ +98,5 @@
> >  
> >    injectData: function (type, content) {
> >      let report = this._getReportURI();
> >  
> > +    // File URIs can't be used for targetOrigin, so we use "*" for
> 
> Should be "file:"

Thanks for landing!

Comment 16

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b149bc52ebe

With these changes in place, the FHR page will load and receive a document, and will produce console error messages if you try, because the FHR page currently doesn't understand that document format (Bug 863440). It'll get there!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b149bc52ebe

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

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

User impact if declined: 
  No FHR.

Testing completed (on m-c, etc.): 
  Extensive manual testing on m-c.

Risk to taking this patch (and alternatives if risky): 
  Slim.

String or IDL/UUID changes made by this patch:
  None.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 755638 [details] [diff] [review]
Provide Java-generated Firefox Health Report to about:healthreport. r=rnewman

Approving as part of the body of work needed for FHR on FF23 Android.

Comment 20

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bedd1d32ec9e