Closed Bug 1010397 Opened 7 years ago Closed 3 years ago
Experiment manager may undo pending uninstalls
* 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
(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
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+]
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.
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 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"?
(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.
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]
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.