Closed Bug 1494781 Opened Last year Closed Last year

Include install error when reporting add-on study enroll errors

Categories

(Firefox :: Normandy Client, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox64 --- verified

People

(Reporter: mythmon, Assigned: mythmon)

Details

Attachments

(1 file)

When installing add-on studies, we report enroll errors, but don't pass along the reason for the failure that the add-on install object exposes. We should do this.
Assignee: nobody → mcooper
Status: NEW → ASSIGNED
Flags: qe-verify+
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f88d27fbe44
Include install error when reporting add-on study enroll errors r=aswan
https://hg.mozilla.org/mozilla-central/rev/6f88d27fbe44
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Looking at the code changes, there seems to be 3 cases for the opt-out studies failure:

ok1. "conflicting-addon-id" - when addon is already installed
       normandy 	enrollFailed 	addon_study 	test bug 1494781 	{"reason": "conflicting-addon-id"}
ok2.  "download-failure":  - when addon is corrupt
    normandy 	enrollFailed 	addon_study 	test bug 1494781 	{"reason": "download-failure", "detail": "ERROR_CORRUPT_FILE"} 

? 3.  "failed to install"


Verified as fixed the additional reasons for the install errors for the first two cases on Nightly 64.0a1 / 20181019005426 / Windows 8.1. Mike, any thoughts of what would be the usecase scenario to hit the 3rd case?
Flags: needinfo?(mcooper)
I'm not quite sure. I think something that could trigger that would be an add-on that is incompatible with the current Firefox, or if the profile directory isn't writable (though that may come up in case 2 instead.

I think that things that could trigger "failed-to-install" include an add-on incompatible with Firefox or an unwritable add-on  directory (in the profile?).

The code in the AddonManager that can trigger onInstallFailed (which leads to "failed-to-install") is here: https://searchfox.org/mozilla-central/rev/9cb3e241502a2d47e2d5057ca771324a446b6695/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1753-1839
Flags: needinfo?(mcooper)
For the case in which the extensions folder has access denied, we fail not nicely and nothing is logged into telemetry.(see full browser console logs here: https://pastebin.com/1mQSNhv8)

app.normandy.recipe-runner	INFO	Executing recipe "aflorinescu test bug 1494781" (action=opt-out-study)
addons.xpi	ERROR	Failed to create staging directory: Win error 5 during operation makeDir on file C:\Users\adrian.florinescu\AppData\Roaming\Mozilla\Firefox\Profiles\tz8vb8wg.s2\extensions\staged (Access is denied.

)((unknown module)) No traceback available

In about:studies it lists the study as being active, which shouldn't happen since the extension clearly failed to install. Seems to me that for this case, we are failing even before reaching the point to logging a telemetry event.
Flags: needinfo?(mcooper)
Adding to comment 6, if I remove the study from the about:studies page, I do get: app.normandy.action.AddonStudyAction	WARN	Could not uninstall addon vk-scrobbler@stsdc.org for recipe 517: it is not installed.
Additional update:

Tracked down a legacy extension and uploaded it to Stage. Setting up a study to use the legacy extension will get us as you guessed in case 2:

14377 	normandy 	enrollFailed 	addon_study 	aflorinescu test bug 1494781 	{"reason": "download-failure", "detail": "ERROR_CORRUPT_FILE"}

@mythmon So nothing I can think of related to when we will hit case 3(comment 4). If you have no other ideas, maybe it would worth closing this issue as verified  for case 1 and case 2 and maybe opening separate issues for case 3 and comment 6 & comment 7?
The goal here was mostly to log more data where it was available, not catch extra errors. Specifically, it added extra reporting about which of the various failure cases happened, but didn't add any new error handling. The problem you found in comment 6 is something we should probably fix, but I don't think it needs to be handled in this bug. I don't think that we can't figure out how to trigger case 3 is a problem. If it happens, I'm glad that the code tells us.

I'm happy marking this as verified.
Flags: needinfo?(mcooper)
This bug should've been marked as verified based on comment 9. My fault leaving it hanging, updating it now.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.