900694 - Detect add-on uninstallation

Status: VERIFIED FIXED

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

Description

[Richard Newman [:rnewman]]

Seems like we don't do this correctly.

Comment 1

[Richard Newman [:rnewman]]

Part 1:

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

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 1: PIC change (git). v1

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 2: track add-on changes more accurately. v1

Turns out that:

* Pending add-on operations are canceled, rather than the operation being reversed. (No onEnabled/onDisabled pairs; you get onEnabled/onCancelled...)
* Uninstalls aren't represented in the data format, so we need to add a separate channel for that.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 784610 [details] [diff] [review]
Part 2: track add-on changes more accurately. v1

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

f+, but I'm not following the onEnvironmentTransition business.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +367,5 @@
> +        ThreadUtils.postToBackgroundThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                try {
> +                    onEnvironmentTransition(previousEnv, updatedEnv);

This doesn't appear on m-c; did you just land new stuff on m-i?  Or is this a patch error?

I was trying to follow this to make sure I understand how removing an add-on updated the environment hash.

::: mobile/android/chrome/content/browser.js
@@ +5358,5 @@
>  
>      return o;
>    },
>  
> +  notifyJava: function (aAddon, aNeedsRestart, aAction="Addons:Change") {

Huh, since Fx15, eh?  It worries me that this is not common in the code base; I wonder why.

Comment 5

[Richard Newman [:rnewman]]

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

> This doesn't appear on m-c; did you just land new stuff on m-i?  Or is this
> a patch error?

It's from Bug 880109, which interleaves between Part 1 and Part 2.

> > +  notifyJava: function (aAddon, aNeedsRestart, aAction="Addons:Change") {
> 
> Huh, since Fx15, eh?  It worries me that this is not common in the code
> base; I wonder why.

Slow adoption of new language features! So it goes.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 784610 [details] [diff] [review]
Part 2: track add-on changes more accurately. v1

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

per irc, there's some interleaving of patches here.

Comment 7

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/a0e2092ffbf7
https://hg.mozilla.org/integration/fx-team/rev/3f3c050840b2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0e2092ffbf7
https://hg.mozilla.org/mozilla-central/rev/3f3c050840b2

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 784604 [details] [diff] [review]
Part 1: PIC change (git). v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Initial landing of FHR.

User impact if declined: 
  Some mis-recording of data when add-ons are added or removed.

Testing completed (on m-c, etc.): 
  Tested locally, just hit m-c.

Risk to taking this patch (and alternatives if risky): 
  This particular patch is 100% trivial. The pair are low-risk; we just watch the Add-on Manager API correctly!

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

Comment 10

[bhavana bajaj [:bajaj]]

Comment on attachment 784610 [details] [diff] [review]
Part 2: track add-on changes more accurately. v1

Requesting QA verification by helping do some addhoc addon related testing to ensure we see the data in FHR as expected.

:rnewman/nalexander, please highlight any specific testcases that you may have in mind.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Part 2 is going to need some fixing up for Aurora.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Part 2 is going to need some fixing up for Aurora.

Yeah, it cross-depends on Bug 880109. Forgot to flag that for uplift.

(In reply to bhavana bajaj [:bajaj] from comment #10)

> Requesting QA verification by helping do some addhoc addon related testing
> to ensure we see the data in FHR as expected.
> 
> :rnewman/nalexander, please highlight any specific testcases that you may
> have in mind.

Bug 880109 provides some context here, but in general:

* Install a non-restartless add-on from AMO. I tested with Flash Video Downloader.
* Watching the logs, you'll see messages about the environment changing.
* Restart Fennec. Now toggle enable/disable/enable in the Add-on Manager. Without this fix, you'll only see messages going in one direction (disable, nothing, disable).
* Restart Fennec as prompted.
* Uninstall the add-on. Without this fix you'll see nothing. With this fix you'll immediately see an environment change.

In all of these cases, with Bug 880109 (which is the situation in which you'll be testing) you'll see a session transition with some more details if you're running at VERBOSE:

08-01 14:51:02.109 D/GeckoHealthRec( 9754): Recording session end: E
08-01 14:51:02.129 V/GeckoHealthRec( 9754): Recorded session entry for env 8, current is 7
08-01 14:51:02.129 D/GeckoSessInfo( 9754): Recording session done: 1375393855955
08-01 14:51:02.129 D/GeckoSessInfo( 9754): Recording runtime session transition: 1375393862144, 409745148
08-01 14:51:02.139 D/GeckoSessInfo( 9754): Recording start of session: 1375393862144

If you start without that add-on at, say, env 6, you should see:

  start:       env 6
  installed:   env 7
  disabled:    env 8
  enabled:     env 7
  uninstalled: env 6

with a session transition between each runtime operation.

Comment 13

[bhavana bajaj [:bajaj]]

Comment on attachment 784604 [details] [diff] [review]
Part 1: PIC change (git). v1

[Triage Comment]
Switching approval to beta for sanity.

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 784610 [details] [diff] [review]
Part 2: track add-on changes more accurately. v1

[Triage Comment]

Switching approval to beta given Fx24 is beta now.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cb0e7325a085
https://hg.mozilla.org/releases/mozilla-beta/rev/80d0d5dcb0bb

Comment 16

[Aaron Train [:aaronmt]]

Nightly (08/08) | Beta 24.0 (candidate #1, build #2) | HTC One (Android 4.2)

Installed Stylish

* D/GeckoHealthRec(15059): Recording session end: E
* V/GeckoHealthRec(15059): Recorded session entry for env 1, current is 2

Restart

* D/GeckoHealthRec(15059): Recording session end: P
* V/GeckoHealthRec(15059): Recorded session entry for env 2, current is 2

Report generated afterwards 

* I/GeckoLogger(15987): fennec :: GeckoHealthGen :: Generating FHR document from 1360423811948; last ping 1367500000000

Disabled

* D/GeckoHealthRec(15751): Recording session end: E
* V/GeckoHealthRec(15751): Recorded session entry for env 2, current is 3

Restart

* D/GeckoHealthRec(15751): Recording session end: P
* V/GeckoHealthRec(15751): Recorded session entry for env 3, current is 3
 
Uninstall

* D/GeckoHealthRec(15987): Recording session end: E
* V/GeckoHealthRec(15987): Recorded session entry for env 3, current is 1

Report generated afterwards

* I/GeckoLogger(16400): fennec :: GeckoHealthGen :: Generating FHR document from 1360423911479; last ping 1367500000000

Works for me

Comment 17

[Richard Newman [:rnewman]]

Nice, thanks Aaron!