During user/normandy "end study" signal, the add-on context is destroyed before endStudy handling is able to run

[motin]

This is the underlying issue behind #194, but there are broader implications than missing out on firing survey URLs. Most importantly, add-on cleanup is skipped, leaving the client possibly with study-related preference/UX side effects post study.

[Steps to reproduce]:

1. Install a study add-on (such as example/small-study)
2. Go to about:addons and uninstall/disable the add-on

[Expected result]:

• Add-on endStudy handling is performed
• Add-on cleanup is performed

[Actual result]:

• No add-on endStudy handling is performed (including opening of survey urls, see during user uninstall, isn't firing survey #194)
• No add-on cleanup is performed

[Notes]:

• Standalone web extension experiments may allow for some (chrome-privileged) post-add-on cleanup to occur, but it is not obvious how we should handle "webextension-layer" cleanup

[motin]

@aswan, @rhelmer: Any suggestions on how we can allow for webextension-layer logic and cleanup to happen before the uninstallation occurs?

[aswan]

Any suggestions on how we can allow for webextension-layer logic and cleanup to happen before the uninstallation occurs?

In general, you cannot. If an extension is uninstalled while it is disabled (which can happen if the user disables and then uninstalls, or if the user uninstalls while in safe mode or smoething), then we cannot run code from inside the extension during uninstall.

What sort of cleanup do you need to run?

[motin]

What sort of cleanup do you need to run?

• Study-specific preference clean-ups
• Clearing study-specific local storage contents (probably done automatically when the extension is uninstalled)

[motin]

Also, the TAAR studies send an important final telemetry payload with the study-specific client state upon the cleanup event.

[MattGMoz]

@aswan @rhelmer this one is actually really devastating for the experiments program. Our inability to reliably debrief study participants means we are only getting half the story. Have we discussed alternative solutions?

[aswan]

There's an existing API that ought to cover the survey issue (I commented separately over on issue #194)
The general idea for webextensions is that they are like regular web content in that the browser itself takes care of any cleanup when an extension is unloaded (just like we don't depend on regular web pages doing things like unregistering event listeners themselves).
For instance, if studies have an experimental API for setting preferences and those preferences should be cleaned up when the study is uninstalled, the privileged part of the preference-setting API should do that in its onShutdown() method.

[motin]

@aswan In a bundled Web Extensions Experiment, the onShutdown handler is not (EDIT: reliably) available after add-on uninstall. Any chance we can have a browser.runtime.onUninstall.addListener, which get's metadata about the uninstall reason (user, normandy)?

[aswan]

I don't understand what you mean by "not available after add-on uninstall". That method will be called whenever an enabled extension is uninstalled. But like I said in my first comment on this issue, you're not going to be able to reliably catch uninstalls from a bundled experiment if the extension is disabled when it is uninstalled. The addon manager also doesn't have metadata about where an uninstall was initiated from. If you want something specific to happen when Normandy uninstalls an extension, can you put the logic for doing that directly into Normandy?

[gregglind]

@aswan, I will attempt to clarify.

ENDINGS

There are several cases of study addon ending that should have effects.

1. from inside the addon, a positive or negative natural endpoint, such as "expired", "ineligible", "user successfully tried the feature".

   1. webExtension should clean up
   2. ask user about the experience by opening tabs.
   3. correct ending reason and exit pings send via Telemetry

2. The user finds the study in about:addons and uninstalls it, or disables. To a study, we think both of those are clear signals of "user dislike". We also don't want studies to re-enable / re-install.

   1. webExtension should clean up
   2. ask user about the experience by opening tabs.
   3. correct ending reason and exit pings send via Telemetry

3. Normandy forces the study to uninstall, due to a potential error / bug in the addon, mis-targeting or other "failsafe" reason. From a research perspective, this is NOT a signal of user intent.

   1. webExtension should clean up
   2. ask user about the experie nce by opening tabs.
   3. correct ending reason and exit pings send via Telemetry <- Reason is incorrect

Issues around this:

• Cases 2 and 3 look identical to the addon. This makes it scary to ask a survey at ending at all

Possible solutions (based in my ignorance, and incomplete understanding of how Extensions.jsm actually works):

1. hide study addons from the about:addons page, so that you can't uninstall them this way. Perhaps filter on id, or some field in the manifest? This might look sneaky. (solves 2)
2. make study addons a different 'type' than regular addons. (could solve all 3, but at high cost)
   • tools for this would have to exist
   • might be hard to qa
   • we tried this with old experiments type. It was Not Awesome at the Time, but fixable.
   • Webdriver still expects xpi files, never mind new types.
3. augment whatever mechanism does the uninstall to handle these cases differently. Perhaps a flag to the 'addonInstall.uninstall()' method. (Differentiates 2/3)
4. force more the 'magic' (such as survey opening) in the WEE part. (Solves part of 2, 3)
   • creates rigid inflexibility for authors
   • even more 'unprivileged' code in the utils.
   • v4 => v5 we moved a lot of code out from the 'legacy' part into the unprivileged scope. Including the specific things around survey opening.
5. Find some mechanism of 'softer uninstall' like "addonInstall.requestGentleUninstall" which would signal the WE. This would solve all 3, IFF combined with 'hide the addons'

[gregglind]

(see #148 #48 #208)

[aswan]

I don't understand what the checkboxes (some checked and some unchecked) in @gregglind's comment mean. To the extent that I understand the issues here (and I'm finding hard to follow since there are a whole bunch of open issues on github, many of which reference multiple overlapping topics), I think this is all solvable without any of the large fundamental changes proposed above.

First, as Gregg mentioned I think we should not show Shield studies in about:addons. We should of course be very clear about this since we're not trying to do anything sneaky. Perhaps a banner or something informing users of about:studies would help. The idea though is that we happen to use extensions for a number of things in the browser. We have system extensions that users don't ever manage directly, about:addons for the things that they choose to install (ie from AMO) and about:studies for things that are installed as part of Shield/Normandy/Pioneer/whatever. This would also mean that Shield studies won't have to resort to hacks for things like avoiding the "Undo uninstall" interface in about:addons.

Now, as for sending final telemetry and showing surveys that depend on the manner in which the study ended, if Normandy is the place where the information about how a study ended is readily available, why not do all those things there rather than trying to get that information into the extension and fight all the gnarly lifecycle issues that arise when you try to make an extension run some code at the moment it is being uninstalled? Normandy can (if it doesn't already) use the AddonManager listener interface to find out when an extension that it installed is disabled or uninstalled. I guess that if one of those events occurs and it isn't happening as a result of something Normanday initiated itself, it can assume that the user is disabling/removing the extension themselves.

[gregglind]

(Checkboxes were meant to show things that correntada work as expected). Moving the problem over to about:studies still has the same problem of user disabling the study not letting the study cleanup. Normandy currently knows almost nothing about studies, but augmenting that might another option to explore. Perhaps Normandy could send warning to the study (via Global observer), and give the study a minute or 5 to shutdown, and to attempt to uninstall itself. Then hit it with the big hammer. None of his helps the “user uninstalled it” case, if the responses to that are happening in the addon, unless that action also has a brief “give me 30 seconds” timeout. That would move the complication to about:studies.

…

On Wed, Jul 11, 2018 at 2:13 PM aswan ***@***.***> wrote: I don't understand what the checkboxes (some checked and some unchecked) in @gregglind <https://github.com/gregglind>'s comment mean. To the extent that I understand the issues here (and I'm finding hard to follow since there are a whole bunch of open issues on github, many of which reference multiple overlapping topics), I think this is all solvable without any of the large fundamental changes proposed above. First, as Gregg mentioned I think we should not show Shield studies in about:addons. We should of course be very clear about this since we're not trying to do anything sneaky. Perhaps a banner or something informing users of about:studies would help. The idea though is that we happen to use extensions for a number of things in the browser. We have system extensions that users don't ever manage directly, about:addons for the things that they choose to install (ie from AMO) and about:studies for things that are installed as part of Shield/Normandy/Pioneer/whatever. This would also mean that Shield studies won't have to resort to hacks for things like avoiding the "Undo uninstall" interface in about:addons. Now, as for sending final telemetry and showing surveys that depend on the manner in which the study ended, if Normandy is the place where the information about how a study ended is readily available, why not do all those things there rather than trying to get that information into the extension and fight all the gnarly lifecycle issues that arise when you try to make an extension run some code at the moment it is being uninstalled? Normandy can (if it doesn't already) use the AddonManager listener interface to find out when an extension that it installed is disabled or uninstalled. I guess that if one of those events occurs and it isn't happening as a result of something Normanday initiated itself, it can assume that the user is disabling/removing the extension themselves. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#232 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAjwwcGA4wezXa4_MwXFH9Nng8l85Wks5uFk5ZgaJpZM4VC7ow> .

[gregglind]

Separately: what’s the proposed mechanism you like for “hide it from about:addons” They show up in telemetry (and get filtered by id by analysts). I suggest that same plan for this, barring nothing subtler.

…

On Wed, Jul 11, 2018 at 2:23 PM Gregg Lind ***@***.***> wrote: (Checkboxes were meant to show things that correntada work as expected). Moving the problem over to about:studies still has the same problem of user disabling the study not letting the study cleanup. Normandy currently knows almost nothing about studies, but augmenting that might another option to explore. Perhaps Normandy could send warning to the study (via Global observer), and give the study a minute or 5 to shutdown, and to attempt to uninstall itself. Then hit it with the big hammer. None of his helps the “user uninstalled it” case, if the responses to that are happening in the addon, unless that action also has a brief “give me 30 seconds” timeout. That would move the complication to about:studies. On Wed, Jul 11, 2018 at 2:13 PM aswan ***@***.***> wrote: > I don't understand what the checkboxes (some checked and some unchecked) > in @gregglind <https://github.com/gregglind>'s comment mean. To the > extent that I understand the issues here (and I'm finding hard to follow > since there are a whole bunch of open issues on github, many of which > reference multiple overlapping topics), I think this is all solvable > without any of the large fundamental changes proposed above. > > First, as Gregg mentioned I think we should not show Shield studies in > about:addons. We should of course be very clear about this since we're not > trying to do anything sneaky. Perhaps a banner or something informing users > of about:studies would help. The idea though is that we happen to use > extensions for a number of things in the browser. We have system extensions > that users don't ever manage directly, about:addons for the things that > they choose to install (ie from AMO) and about:studies for things that are > installed as part of Shield/Normandy/Pioneer/whatever. This would also mean > that Shield studies won't have to resort to hacks for things like avoiding > the "Undo uninstall" interface in about:addons. > > Now, as for sending final telemetry and showing surveys that depend on > the manner in which the study ended, if Normandy is the place where the > information about how a study ended is readily available, why not do all > those things there rather than trying to get that information into the > extension and fight all the gnarly lifecycle issues that arise when you try > to make an extension run some code at the moment it is being uninstalled? > Normandy can (if it doesn't already) use the AddonManager listener > interface to find out when an extension that it installed is disabled or > uninstalled. I guess that if one of those events occurs and it isn't > happening as a result of something Normanday initiated itself, it can > assume that the user is disabling/removing the extension themselves. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#232 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAKAjwwcGA4wezXa4_MwXFH9Nng8l85Wks5uFk5ZgaJpZM4VC7ow> > . >