Rework addon data format for Fennec FHR

RESOLVED INCOMPLETE

Status

RESOLVED INCOMPLETE
5 years ago
8 days ago

People

(Reporter: rnewman, Unassigned)

Tracking

Trunk
All
Android
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Summary: Rework plugin data format for Fennec FHR → Rework addon data format for Fennec FHR
Created attachment 8364514 [details] [diff] [review]
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.
Created attachment 8364536 [details]
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.
Attachment #8364536 - Flags: feedback?(rnewman)
(Reporter)

Updated

5 years ago
Depends on: 928575
(Reporter)

Updated

5 years ago
Attachment #8364514 - Flags: feedback?(rnewman)
Created attachment 8364573 [details] [diff] [review]
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.
(Reporter)

Comment 4

5 years ago
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.
Attachment #8364514 - Flags: feedback?(rnewman) → feedback-
(Reporter)

Comment 5

5 years ago
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.
Attachment #8364536 - Flags: feedback?(rnewman)
Created attachment 8365006 [details]
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)
[...]
Attachment #8364536 - Attachment is obsolete: true
Flags: needinfo?(rnewman)
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> I'm definitely not about:

... definitely not *sure* about
(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?
(Reporter)

Comment 9

5 years ago
(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.)
Flags: needinfo?(rnewman)
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.

Updated

4 years ago
Assignee: georg.fritzsche → nobody
Per https://bugzilla.mozilla.org/show_bug.cgi?id=1497137, this component is deprecated. Resolving this bug as incomplete, per :sdaswani.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 days ago
Resolution: --- → INCOMPLETE

Updated

8 days ago
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.