875400 - Incorporate add-ons into environment

Status: RESOLVED FIXED

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

Comment 1

[Richard Newman [:rnewman]]

https://github.com/mozilla-services/android-sync/pull/312

Part 2 will be on the Fennec side.

Comment 2

[Richard Newman [:rnewman]]

OK, might need some help here. I'm trying to track all add-on state changes as they happen. (Later I will be getting a dump at startup, but this is a good place to begin.)


I'm adding to browser.js:

let AddonStatusListener = Object.freeze({
  init: function () {
    dump("INITING ASL\n");
    AddonManager.addAddonListener(this);
  },

  ...

  onDisabled: function (aAddon) {
    dump("onDisabled.\n");
    this.notifyJava(aAddon);
  },

with an appropriate hook to call init.

init is being called:

05-24 19:29:46.555 E/GeckoConsole(23664): INITING ASL


But none of my callbacks are being called when I tap, e.g., 'disable' in Tools > Add-ons.

Blair, Dave, any idea why? What am I doing wrong?

Comment 3

[Dave Townsend [:mossop]]

(In reply to Richard Newman [:rnewman] from comment #2)
> OK, might need some help here. I'm trying to track all add-on state changes
> as they happen. (Later I will be getting a dump at startup, but this is a
> good place to begin.)
> 
> 
> I'm adding to browser.js:
> 
> let AddonStatusListener = Object.freeze({
>   init: function () {
>     dump("INITING ASL\n");
>     AddonManager.addAddonListener(this);
>   },
> 
>   ...
> 
>   onDisabled: function (aAddon) {
>     dump("onDisabled.\n");
>     this.notifyJava(aAddon);
>   },
> 
> with an appropriate hook to call init.
> 
> init is being called:
> 
> 05-24 19:29:46.555 E/GeckoConsole(23664): INITING ASL
> 
> 
> But none of my callbacks are being called when I tap, e.g., 'disable' in
> Tools > Add-ons.
> 
> Blair, Dave, any idea why? What am I doing wrong?

Note there are two events of interest, onDisabling and onDisabled. The former happens when the user has asked to disable the add-on, the latter only happens if the add-on can be disabled without a restart. My guess is you're disabling an add-on that requires a restart.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Dave Townsend (:Mossop) from comment #3)

Thanks for the weekend catch!

> Note there are two events of interest, onDisabling and onDisabled. The
> former happens when the user has asked to disable the add-on, the latter
> only happens if the add-on can be disabled without a restart.

Oh wow, that's good to know. So only one of each pair of events will ever be fired, not both? (I figured this was an onBefore/onAfter thing.)

Will `userDisabled` be true in `onDisabling`?

I'll try catching the other events, see what happens.

> My guess is you're disabling an add-on that requires a restart.

Actually no; the two I tried were QuitNow and the Wikipedia search engine. Neither prompted for restart.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Richard Newman [:rnewman] from comment #4)
> (In reply to Dave Townsend (:Mossop) from comment #3)
> 
> Thanks for the weekend catch!
> 
> > Note there are two events of interest, onDisabling and onDisabled. The
> > former happens when the user has asked to disable the add-on, the latter
> > only happens if the add-on can be disabled without a restart.
> 
> Oh wow, that's good to know. So only one of each pair of events will ever be
> fired, not both? (I figured this was an onBefore/onAfter thing.)

onDisabling will always be fired, onDisabled will only be fired if we can disable without a restart. We also pass a flag to onDisabling which tells you if that is going to happen.

> Will `userDisabled` be true in `onDisabling`?

IIRC yes.

> I'll try catching the other events, see what happens.
> 
> > My guess is you're disabling an add-on that requires a restart.
> 
> Actually no; the two I tried were QuitNow and the Wikipedia search engine.
> Neither prompted for restart.

Hmm, that's interesting then.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 2: v1

Particular areas for review and structure comments:

* This patch adds one message _to_ Gecko -- "Addons:FetchAll" -- and three messages _from_ Gecko:

1. Addons:All: the response to FetchAll.
2. Addons:Change: sent when an add-on has changed (enabled, installed, etc.).
3. Pref:Change: pref observation. Currently has no provision for pref type other than boolean. This is so we can see when you, e.g., disable blocklist or telemetry.

Please correct me if the style is wrong. These are registered and unregistered within BrowserHealthRecorder.

* We now have a HealthReportStatusListener inside browser.js. This is an AddonsListener and also a pref observer for the prefs that FHR cares about, and it's responsible for sending and receiving those new messages.

* Profile state is persisted between runs, and loaded asynchronously on startup. This is so we don't have to grab all of your add-ons on each launch. The assumption here is that by watching pref changes and add-on changes, and staying alive for the duration of the browser app, our on-disk cache will always be accurate. On first run we use Addons:FetchAll and pref requests to grab the things we need, but after that we just slurp a small JSON file.

* From watching the logs, all of this work happens in the background and interleaved with other stuff, and of course the Gecko responses delay conclusion of init until Gecko is fully set up. No StrictMode warnings or jank observed, but it warrants testing. (Though there's not too much we can do other than delay this work even further and cache events until it's done.)

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 1: from Git. v1

For reference, part 1. Nick reviewed most of this out-of-band, but let's get a final pass.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 2: v2

Just making sure this is up to date.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 755535 [details] [diff] [review]
Part 2: v1

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    Services.obs.addObserver(this, "Addons:FetchAll", false);

Move to HealthReportStatusListener.init

>+      case "Addons:FetchAll":
>+        HealthReportStatusListener.sendAllAddonsToJava();
>+        break;

Handle in HealthReportStatusListener

>+let HealthReportStatusListener = {

>+  init: function () {

>+    console.log("Add-on status listener initialized.");

Too chatty

>+  uninit: function () {

Might as well remove the "Addons:FetchAll" observer here too

>+  onPropertyChanged: function (aAddon, aProperties) {
>+    this.notifyJava(aAddon, aNeedsRestart);

There is no aNeedsRestart param

>+  notifyJava: function (aAddon, aNeedsRestart) {
>+  sendAllAddonsToJava: function () {

Might be nice to make Addons:All and Addons:Change be a unified Addons:Data, even if for changes it's an array of one add-on. The Gecko side uses the same code to make the JSON and the Java side could similarly reuse code for both "events". Maybe a followup.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 756166 [details] [diff] [review]
Part 2: v2

r+ and I think my review on the previous version still stands

Comment 11

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #9)

> Move to HealthReportStatusListener.init
> et al

Done. 

> Might be nice to make Addons:All and Addons:Change be a unified Addons:Data,
> even if for changes it's an array of one add-on. The Gecko side uses the
> same code to make the JSON and the Java side could similarly reuse code for
> both "events". Maybe a followup.

I was about to start doing this, and I thought about it before, but I decided that it would involve too much message inspection -- the sender would need to decide whether to put in some flag, and the receiver would need to look for it (because the handling differs between incremental and overwrite). Once y'hit that point, you might as well use a new message type anyway.

I also re-jigged how ignored add-ons are handled. New patch shortly.

Comment 12

[Richard Newman [:rnewman]]

Thanks for your patience with this, folks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3051ff11a5be
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e47042b767e

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 755539 [details] [diff] [review]
Part 1: from Git. v1

Reviewed on github, right?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3051ff11a5be
https://hg.mozilla.org/mozilla-central/rev/4e47042b767e

Comment 15

[Richard Newman [:rnewman]]

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

> Reviewed on github, right?

Yessir.

Comment 18

[Alex Keybl [:akeybl]]

We'll track, but we wouldn't block the release on this bug.

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 755539 [details] [diff] [review]
Part 1: from Git. v1

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

User impact if declined: 
  No FHR in 23.

Testing completed (on m-c, etc.): 
  Baking on m-c for some time. QA overview written up; waiting for QA to execute. Manual testing and automated unit tests.

Risk to taking this patch (and alternatives if risky): 
  None: new work.

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

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 756166 [details] [diff] [review]
Part 2: v2

Approving for uplift as part of the body of work for FHR's first revision on Android.

Comment 21

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/949ad423d76c
https://hg.mozilla.org/releases/mozilla-aurora/rev/e790d9247363