Closed Bug 875400 Opened 8 years ago Closed 8 years ago

Incorporate add-ons into environment

Categories

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

ARM
Android
defect

Tracking

(firefox23+ fixed)

RESOLVED FIXED
Firefox 24
Tracking Status
firefox23 + fixed

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Depends on: 868449
Blocks: 868442
Depends on: 868447
Depends on: 875401
https://github.com/mozilla-services/android-sync/pull/312

Part 2 will be on the Fennec side.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
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?
Flags: needinfo?(bmcbride)
Priority: -- → P1
(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.
(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.
(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.
Flags: needinfo?(bmcbride)
Depends on: 877061
Attached patch Part 2: v1 (obsolete) — Splinter Review
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.)
Attachment #755535 - Flags: review?(mark.finkle)
For reference, part 1. Nick reviewed most of this out-of-band, but let's get a final pass.
Attachment #755539 - Flags: review?(nalexander)
Attached patch Part 2: v2Splinter Review
Just making sure this is up to date.
Attachment #755535 - Attachment is obsolete: true
Attachment #755535 - Flags: review?(mark.finkle)
Attachment #756166 - Flags: review?(mark.finkle)
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 on attachment 756166 [details] [diff] [review]
Part 2: v2

r+ and I think my review on the previous version still stands
Attachment #756166 - Flags: review?(mark.finkle) → review+
(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 on attachment 755539 [details] [diff] [review]
Part 1: from Git. v1

Reviewed on github, right?
Attachment #755539 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #13)

> Reviewed on github, right?

Yessir.
Duplicate of this bug: 868447
Duplicate of this bug: 865444
We'll track, but we wouldn't block the release on this bug.
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.
Attachment #755539 - Flags: approval-mozilla-aurora?
Attachment #756166 - Flags: approval-mozilla-aurora?
Attachment #755539 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #756166 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 884419
Blocks: 885042
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.