1494781 - Include install error when reporting add-on study enroll errors

Status: VERIFIED FIXED

(Firefox :: Normandy Client, enhancement)

Description

[Michael Cooper [:mythmon]]

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.

Comment 1

[Michael Cooper [:mythmon]]

Attachment: Bug 1494781 - Include install error when reporting add-on study enroll errors r=aswan

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f88d27fbe44
Include install error when reporting add-on study enroll errors r=aswan

Comment 3

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6f88d27fbe44

Comment 4

[Adrian Florinescu [:aflorinescu]]

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?

Comment 5

[Michael Cooper [:mythmon]]

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

Comment 6

[Adrian Florinescu [:aflorinescu]]

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.

Comment 7

[Adrian Florinescu [:aflorinescu]]

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 [e-mail] for recipe 517: it is not installed.

Comment 8

[Adrian Florinescu [:aflorinescu]]

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?

Comment 9

[Michael Cooper [:mythmon]]

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.

Comment 10

[Adrian Florinescu [:aflorinescu]]

This bug should've been marked as verified based on comment 9. My fault leaving it hanging, updating it now.