1564818 - Opt-out experiments are wrongly displayed as active in "about:studies" page after they are removed

Status: VERIFIED FIXED

(Firefox :: Normandy Client, defect, P1)

Description

[Remus Dranca [rdranca], Ecosystem QA]

Attachment: aboutStudiesRemove.gif

[Notes]:

• If the page is refreshed, the experiment will be displayed in the "Completed studies" section.

[Affected versions]:

• Firefox 67 and above

[Affected Platforms]:

• All Windows
• All Mac
• All Linux

[Prerequisites]:

• Have a recipe created for the experiment on the Normandy stage server (link to recipe).
• Have the latest Firefox Unbranded build installed.
• Have at least 3 add-ons installed from AMO.
• Have the following prefs set in “about-config” page:
  • The "xpinstall.signatures.dev-root" boolean pref with the "true" value.
  • The “app.normandy.dev_mode preference” to “true” to run recipes immediately on startup.
  • The “app.normandy.logging.level preference” to “0” to enable more logging.
  • The “security.content.signature.root_hash” preference to “DB:74:CE:58:E4:F9:D0:9E:E0:42:36:BE:6C:C5:C4:F6:6A:E7:74:7D:C0:21:42:7A:03:BC:2F:57:0C:8B:9B:90”.
  • The “services.settings.server” to “https://normandy.stage.mozaws.net/api/v1”.
  • The “extensions.shield-recipe-client.api_url” to “https://stage.normandy.nonprod.cloudops.mozgcp.net/api/v1”.

[Steps to reproduce]:

1. Open the browser with the profile from prerequisites.
2. Navigate to the "about:studies" page.
3. Click the "Remove" button for the “Preparatory Survey for NAAR” experiment and observe the behavior.

[Expected result]:

• The experiment is instantly moved to the "Completed studies" section.

[Actual result]:

• The experiment is still displayed in the "Active studies" section.

[Additional Notes]:

• This issue is also reproducible with the "JESTr Relaunch: Identifying Web Browsing Patterns in the Pioneer Population" experiment we tested.
• The following error message is displayed in the Browser's Console: "Error: Cannot stop study for recipe 781; it is already inactive.".
• Nothing happens if the "Remove" button is clicked multiple times, the experiment remains in the "Active studies" section.
• However, the notification bar is no longer displayed after clicking the "Remove" button and the correct telemetry pings are sent.
• We initially logged this issue on the NAAR repo but once we found an additional addon that has the same behavior we thought that this might be an issue caused by some change in the client.
• Attached a screen recording with the issue.

Comment 1

[Ciprian Muresan [:cmuresan], Ecosystem QA]

@mythmon, could this be caused by the recent changes made by the Normandy integration of Shield Study Add-on Utils feature?
I've tried to reproduce this issue with a couple of older addons that are already uploaded in the Delivery Console and in those cases the addons were removed and the page updated instantly.

Comment 2

[Michael Cooper [:mythmon]]

It is possible that this is related to the new feature, but there aren't any studies using this program yet, so if some add-ons are doing it and some aren't, it seems unlikely. I think there are probably a couple things going on here.

1. about:studies doesn't update automatically if it is open while a study is removed by another source.

• For some studies, that means that the add-on doesn't get disabled immediately when clicking the button since the add-on itself delays uninstallation by a moment.

2. If a study has already been removed when the button is clicked, about:studies should update immediately and mark it as removed, instead of doing nothing.

Does that match what you are seeing? Can you also take a look at the devtools console on the about:studies page directly?

Comment 3

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Attachment: aboutstudies console output.txt

@mythmon, yes I think that matches exactly what I'm seeing.

I've attached a .txt doc with the console messages that appear when I remove the addon in question. I've also observed that the .xpi remains in a trash folder inside the extensions folder even after the browser is restarted.

Comment 4

[Remus Dranca [rdranca], Ecosystem QA]

Cloning Fredrik Wollsén's comment from the "preparatory-survey-for-naar-study-addon" repo, regarding this issue:

Aha, it complains about setTimeout not being defined. This is code in a version of the utils library which we will deprecate soon. Transferring to the utils repo... (https://github.com/mozilla/shield-studies-addon-utils/issues/296)

Comment 5

[Michael Cooper [:mythmon]]

After getting more info from Fredrik, I have a clearer idea of what's going on here:

1. Studies that delay the uninstallation of add-on (such as Jestr) might interact poorly with about:addons. I need to talk to Fredrik about this particular delay, but the delay in general should be supported.
2. Studies that throw an exception while delaying add-on removal definitely interact poorly with about:addons.

As well as the things I talked about in comment 2, we should also check:

1. That about:studies properly interacts with delayed add-on removal. Probably it needs a spinner as well.
2. That a failed study-end hook doesn't bring the whole thing crashing down.

Comment 6

[Michael Cooper [:mythmon]]

Attachment: Bug 1564818 pt1 - Update about:studies after pressing "remove study", even if the remove failed r=Gijs

Comment 7

[Michael Cooper [:mythmon]]

Attachment: Bug 1564818 pt2 - Don't fail to uninstall studies if a callback throws an exception r=Gijs

Depends on D38929

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:mythmon, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/039b3972dceb
pt1 - Update about:studies after pressing "remove study", even if the remove failed r=Gijs
https://hg.mozilla.org/integration/autoland/rev/0192c6bb5f35
pt2 - Don't fail to uninstall studies if a callback throws an exception r=Gijs

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/039b3972dceb
https://hg.mozilla.org/mozilla-central/rev/0192c6bb5f35

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta uplift or can this fix ride with Fx70 to release?

Comment 12

[Michael Cooper [:mythmon]]

Comment on attachment 9079856 [details]
Bug 1564818 pt2 - Don't fail to uninstall studies if a callback throws an exception r=Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Some studies may fail to unenroll when instructed to do by either the user or Normandy server.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: STR from comment 0 of the bug should be able to verify this fix.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change affects the unenrollment system of Normandy, which is an important system. However, the part it changes is not often used yet.
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9079855 [details]
Bug 1564818 pt1 - Update about:studies after pressing "remove study", even if the remove failed r=Gijs

Fixes a bug which can cause studies to not properly un-enroll in some cases. Approved for 69.0b9.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/73211224425a
https://hg.mozilla.org/releases/mozilla-beta/rev/ae9514b36998

Comment 15

[Remus Dranca [rdranca], Ecosystem QA]

I have verified this issue and it's no longer reproducible on Firefox Nightly 70.0a1 (Build ID: 20190731215544) and Firefox Beta 69.0b9 (Build ID: 20190730004747), using the same recipe from prerequisites. Now, the experiment is instantly moved to the "Completed studies" section when the "Remove" button is clicked.