928574 - Rework addon data format for Fennec FHR

Status: RESOLVED INCOMPLETE

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Fennec overhaul for addon data

This should be the needed changes for Fennec. This needs to have bug 928575 reviewed first though and i still need to get the mozilla-services unit tests running properly.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: integration test log

To be sure this is after a fresh reinstall of my custom built Fennec and running it once.
I'm not sure which, or if even all, errors here are expected.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: WIP android-sync changes

For what it's worth, that's what we need changed in android-sync as far as i can tell. Not sure about the tests yet of course.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8364514 [details] [diff] [review]
Fennec overhaul for addon data

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

Overall comments:

* Same comments as Greg's for desktop.
* I trust that you know what you're doing in calling out to plugin tags.
* There is a potential perf slam here: now to compute the current set of add-ons, which happens during first run of Fennec, we have to stat an arbitrary number of files. Make sure you get profiles for this on Fennec. If it has an impact (e.g., we're not already stat'ing those files), you will need to find a different implementation.

f- for changing the 'signature' of a message without altering its consumers.

::: mobile/android/chrome/content/browser.js
@@ +5705,5 @@
>      return o;
>    },
>  
> +  jsonForPlugin: function (aPluginTag) {
> +    function getUserdisabled(tag) {

Nit: getUserDisabled

@@ +5707,5 @@
>  
> +  jsonForPlugin: function (aPluginTag) {
> +    function getUserdisabled(tag) {
> +      if (tag.disabled)
> +        return true;

Nit: braces around single-line conditionals.

@@ +5713,5 @@
> +        return "askToActivate";
> +      return false;
> +    }
> +
> +    function getModifiedDay(provider, tag) {

Same comments as Greg's in the desktop patch:

  return OS.File.stat(tag.fullpath).then((info) => provider._dateToDays(info.lastModificationDate));

But note that this returns a promise, so you need to handle things accordingly.

@@ +5725,5 @@
> +      name: aPluginTag.name,
> +      version: aPluginTag.version,
> +      description: aPluginTag.description,
> +      appDisabled: aPluginTag.blocklisted,
> +      userDisabled: getUserdisabled(aPluginTag),

userDisabled: tag.disabled || (tag.clicktoplay && "askToActivate"),

and kill the method.

@@ +5763,5 @@
>    },
>  
>    sendSnapshotToJava: function () {
>      AddonManager.getAllAddons(function (aAddons) {
> +        let jsonA = [];

Why are you making this change? (Presumably so you don't leak plugin IDs.)

This will require modifications in BrowserHealthRecorder, at the very least. I'd advise against this unless there's a good reason.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8364536 [details]
integration test log

[INFO] 4DD0000600000001_samsung_GT-I9100G :       junit.framework.AssertionFailedError
at org.mozilla.gecko.background.common.TestUtils.mkdir(TestUtils.java:147)

implies that either the application cache directory doesn't exist, or the test directory -- that includes the current time in milliseconds -- already exists.


[INFO] 4DD0000600000001_samsung_GT-I9100G :       android.database.sqlite.SQLiteException: unable to open database file
at android.database.sqlite.SQLiteDatabase.dbopen(Native Method)
at android.database.sqlite.SQLiteDatabase.<init>(SQLiteDatabase.java:1990)
at android.database.sqlite.SQLiteDatabase.openDatabase(SQLiteDatabase.java:905)
at android.database.sqlite.SQLiteDatabase.openOrCreateDatabase(SQLiteDatabase.java:995)

implies that the application directory doesn't exist, or isn't writable.


I suggest uninstalling Fennec and the "Firefox Sync" app on your phone, then install and launch your Fennec build, then try running `mvn integration-test` again.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Attachment: integration test log #2

I already uninstalled the Fennec build in previous tries, but now also uninstalled org.mozilla.gecko.test/org.mozilla.gecko per what i see in preprocess.ini.
Fewer errors now, most look like they should be from a missing live environment.

I'm definitely not about:

A few of these:
> java.lang.SecurityException: Permission Denial: opening provider org.mozilla.gecko.background.healthreport.HealthReportProvider from ProcessRecord{40b199a8 19795:org.mozilla.gecko/10140} (pid=19795, uid=10140) requires org.mozilla.fennec_gfritzsche.permissions.HEALTH_PROVIDER or org.mozilla.fennec_gfritzsche.permissions.HEALTH_PROVIDER
[...]

And DB errors:
> [INFO] 4DD0000600000001_samsung_GT-I9100G :       java.lang.NullPointerException
> at org.mozilla.gecko.background.healthreport.TestHealthReportDatabaseStorage.testEnvironmentsAndFields(TestHealthReportDatabaseStorage.java:172)
[...]

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> I'm definitely not about:

... definitely not *sure* about

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Richard Newman [:rnewman] from comment #4)
> * Same comments as Greg's for desktop.
> * I trust that you know what you're doing in calling out to plugin tags.
> * There is a potential perf slam here: now to compute the current set of
> add-ons, which happens during first run of Fennec, we have to stat an
> arbitrary number of files. Make sure you get profiles for this on Fennec. If
> it has an impact (e.g., we're not already stat'ing those files), you will
> need to find a different implementation.

We already hit this before, please see bug 928575, comment 6. Note that on Fennec releases, Flash is the only plugin we support.

> >    sendSnapshotToJava: function () {
> >      AddonManager.getAllAddons(function (aAddons) {
> > +        let jsonA = [];
> 
> Why are you making this change? (Presumably so you don't leak plugin IDs.)
> 
> This will require modifications in BrowserHealthRecorder, at the very least.
> I'd advise against this unless there's a good reason.

Per bug 922318 comment 2 et al we don't want to submit the plugin ids and add more data for plugins.
So, we need to change the data format anyway, the question is how exactly. Can you comment on bug 928575, comment 7 so we don't duplicate the discussion?

Comment 9

[Richard Newman [:rnewman]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)

> A few of these:
> > java.lang.SecurityException: Permission Denial: opening provider org.mozilla.gecko.background.healthreport.HealthReportProvider from ProcessRecord{40b199a8 19795:org.mozilla.gecko/10140} (pid=19795, uid=10140) requires org.mozilla.fennec_gfritzsche.permissions.HEALTH_PROVIDER or org.mozilla.fennec_gfritzsche.permissions.HEALTH_PROVIDER

What are the package names in  `package-name.txt` and `preprocess.ini` ? Are they org.mozilla.fennec_gfitzsche?

(See the re-packaged/own-package distinction in the README.)

Comment 10

[Georg Fritzsche [:gfritzsche]]

Yes, package-name.txt is org.mozilla.fennec_gfritzsche and and preprocess.ini has "ANDROID_PACKAGE_NAME = org.mozilla.fennec_%(USERNAME)s" uncommented per the README.

Comment 11

[Firefox Bug Husbandry Bot]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1497137, this component is deprecated. Resolving this bug as incomplete, per :sdaswani.