1010397 - Experiment manager may undo pending uninstalls

Status: RESOLVED WONTFIX

(Firefox Health Report Graveyard :: Client: Desktop, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

* have an active experiment
* click remove
* wait for (or trigger) an experiment update or evaluation

Actual:
* experiment manager resets the addon to the state it expects

Expected:
* experiment manager leaves the state of the addon alone

Comment 1

[Georg Fritzsche [:gfritzsche]]

(Note that this only applies for hitting _evaluateExperiments() while the addon manager category view with the addon undo button is still open.)

Comment 2

[Georg Fritzsche [:gfritzsche]]

This would also not be an issue once bug 612168 is fixed.

Comment 3

[Marco Mucci [:MarcoM]]

Added to iteration.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: WIP: Don't re-activate userDisabled experiment addon if it has a pending uninstall

I'm not realistically fixing this today, so i'm putting this here.

The idea was that we can just skip re-activating an experiment addon if it has pending uninstall (clicking remove in the addon manager UI ends up setting userDisabled=true and pendingOperations=PENDING_UNINSTALL on the addon, to allow for undo until bug 612168).

This fails at least testEnabledAfterRestart in browser/experiments/test/xpcshell/test_api.js, apparently because the active experiment addon has pendingOperations==AddonManager.PENDING_UNINSTALL.
I'm not sure yet why, if that is really neccesssary or how to tell this apart from the above state.

Relevant tests:
* browser/experiments/test/xpcshell
* toolkit/mozapps/extensions/test/browser/browser_experiment.js

Comment 6

[Benjamin Smedberg]

Bug 1017744 has slightly different STR:

* click remove
* close/reopen Firefox

Experiment is enabled/should be disabled. I think that this is the same basic bug, but that case should be included in testing/QA.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

https://tbpl.mozilla.org/?tree=Try&rev=b93e035b87f4

Do you know why we weren't setting addon.pendingUninstall = true; before? Note that this now causes a lot of test failures.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8435951 [details] [diff] [review]
WIP Patch

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

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1542,5 @@
>              this.setAttribute("wasDisabled", this.mAddon.userDisabled);
>  
>              // We must set userDisabled to true first, this will call
>              // _updateState which will clear any pending attribute set.
> +            this.mAddon.pendingUninstall = true;

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Do you know why we weren't setting addon.pendingUninstall = true; before?
> Note that this now causes a lot of test failures.

Er, because this doesn't actually do anything?

You're setting it on an AddonWrapper object, which doesn't have this property. For the pendingOperations
property to include PENDING_UNINSTALL, pendingUninstall=true needs to be set on the AddonInternal object, which isn't exposed outside of XPIProvider.

Comment 4 is wrong - clicking uninstall for a restartless add-on currently does *not* (and isn't expected to) make pendingOperations include PENDING_UNINSTALL. We need to fix bug 612168 in order to make that to happen.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Alright, i'm not sure what i got confused here about while trying to quickly finish this before leaving...
Sorry about that jaws.

Blair, do we have no way at all to tell from outside XPI provider that an addon item has .pending="uninstall"?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> Alright, i'm not sure what i got confused here about while trying to quickly
> finish this before leaving...
> Sorry about that jaws.
> 
> Blair, do we have no way at all to tell from outside XPI provider that an
> addon item has .pending="uninstall"?

Georg, you can look for <richlistitem penidng="uninstall"> in the #addon-list.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> Blair, do we have no way at all to tell from outside XPI provider that an
> addon item has .pending="uninstall"?

Addon.pendingOperations is a read-only bitfield, and will include AddonManager.PENDING_UNINSTALL.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Telemetry experiments were removed from Firefox per bug 1415284.