If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Detect add-on uninstallation

VERIFIED FIXED in Firefox 24

Status

Firefox Health Report
Client: Android
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
Firefox 25
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Seems like we don't do this correctly.
(Assignee)

Comment 1

4 years ago
Part 1:

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

Comment 2

4 years ago
Created attachment 784604 [details] [diff] [review]
Part 1: PIC change (git). v1
Attachment #784604 - Flags: review?(nalexander)
(Assignee)

Comment 3

4 years ago
Created attachment 784610 [details] [diff] [review]
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.
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+
(Assignee)

Comment 5

4 years ago
(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+
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/a0e2092ffbf7
https://hg.mozilla.org/integration/fx-team/rev/3f3c050840b2
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox24: --- → ?
Target Milestone: --- → Firefox 25
(Assignee)

Updated

4 years ago
Blocks: 880109

Updated

4 years ago
tracking-firefox24: ? → +
https://hg.mozilla.org/mozilla-central/rev/a0e2092ffbf7
https://hg.mozilla.org/mozilla-central/rev/3f3c050840b2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

4 years ago
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?
(Assignee)

Updated

4 years ago
Attachment #784610 - Flags: approval-mozilla-aurora?

Updated

4 years ago
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+

Updated

4 years ago
Keywords: qawanted, verifyme
Part 2 is going to need some fixing up for Aurora.
status-firefox23: affected → wontfix
status-firefox25: --- → fixed
Flags: needinfo?(rnewman)
Keywords: branch-patch-needed
(Assignee)

Comment 12

4 years ago
(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)
(Assignee)

Updated

4 years ago
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/cb0e7325a085
https://hg.mozilla.org/releases/mozilla-beta/rev/80d0d5dcb0bb
status-firefox24: affected → fixed
Keywords: branch-patch-needed
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
status-firefox24: fixed → verified
status-firefox26: --- → verified

Updated

4 years ago
Keywords: qawanted, verifyme
(Assignee)

Comment 17

4 years ago
Nice, thanks Aaron!
You need to log in before you can comment on or make changes to this bug.