Closed Bug 900694 Opened 8 years ago Closed 8 years ago

Detect add-on uninstallation

Categories

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

All
Android
defect
Not set
normal

Tracking

(firefox23 wontfix, firefox24+ verified, firefox25 fixed, firefox26 verified)

VERIFIED FIXED
Firefox 25
Tracking Status
firefox23 --- wontfix
firefox24 + verified
firefox25 --- fixed
firefox26 --- verified

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(2 files)

Seems like we don't do this correctly.
Attachment #784604 - Flags: review?(nalexander)
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.
Attachment #784610 - Flags: review?(nalexander)
Attachment #784604 - Flags: review?(nalexander) → review+
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.
Attachment #784610 - Flags: review?(nalexander) → feedback+
(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 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.
Attachment #784610 - Flags: review+
Blocks: 880109
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.
Attachment #784604 - Flags: approval-mozilla-aurora?
Attachment #784610 - Flags: approval-mozilla-aurora?
Attachment #784604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #784610 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Part 2 is going to need some fixing up for Aurora.
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
Blocks: 901622
Comment on attachment 784604 [details] [diff] [review]
Part 1: PIC change (git). v1

[Triage Comment]
Switching approval to beta for sanity.
Attachment #784604 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
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.
Attachment #784610 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
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
Status: RESOLVED → VERIFIED
Nice, thanks Aaron!
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.