Closed Bug 1010397 Opened 7 years ago Closed 3 years ago

Experiment manager may undo pending uninstalls

Categories

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

defect

Tracking

(firefox30 unaffected, firefox31 affected, firefox32 affected)

RESOLVED WONTFIX
Tracking Status
firefox30 --- unaffected
firefox31 --- affected
firefox32 --- affected

People

(Reporter: gfritzsche, Unassigned)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

* 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
Flags: firefox-backlog+
(Note that this only applies for hitting _evaluateExperiments() while the addon manager category view with the addon undo button is still open.)
This would also not be an issue once bug 612168 is fixed.
Assignee: nobody → georg.fritzsche
Added to iteration.
Status: NEW → ASSIGNED
Whiteboard: p=5 [qa+] → p=5 s=it-32c-31a-30b.2 [qa+]
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
Assignee: georg.fritzsche → nobody
Status: ASSIGNED → NEW
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] → p=5 [qa+]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Whiteboard: p=5 [qa+] → p=5 s=it-32c-31a-30b.3 [qa+]
Duplicate of this bug: 1017744
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.
Attached patch WIP PatchSplinter Review
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.
Attachment #8423873 - Attachment is obsolete: true
Attachment #8435951 - Flags: feedback?(bmcbride)
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.
Attachment #8435951 - Flags: feedback?(bmcbride) → feedback+
Attachment #8435951 - Flags: feedback+ → feedback-
Whiteboard: p=5 s=it-32c-31a-30b.3 [qa+] → p=5 s=33.1 [qa+]
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"?
Flags: needinfo?(bmcbride)
(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.
Flags: needinfo?(bmcbride)
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Whiteboard: p=5 s=33.1 [qa+] → p=5 [qa+]
(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.
No longer blocks: telex
Priority: -- → P4
Whiteboard: p=5 [qa+] → [measurement:client]
Depends on: 733045
No longer depends on: 612168
Telemetry experiments were removed from Firefox per bug 1415284.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.