1302846 - Add Test Pilot/Future Discovery Pane add-ons to e10s whitelist

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[Wil Clouser [:clouserw]]

This bug is a request to add the Test Pilot add-ons to the e10s rollout whitelist at http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm .  The Test Pilot add-on IDs:

> Test Pilot:            @testpilot-addon
> Tab Center:            [e-mail]
> Activity Stream:       @activity-streams
> Min Vid:               @min-vid
> Page Shot:             jid1-NeEaf3sAHdKHPA@jetpack

I'm told webextensions don't need to be on this list, but if I'm wrong, those IDs are:

> Tracking Protection:   [e-mail]
> No More 404s:          [e-mail]

The Test Pilot add-ons are marked compatible with e10s and have gone through our regular QA testing on release and nightly versions.  If there are additional requirements I'm happy to run our add-ons through them.

Test Pilot is doing a substantial marketing push on the 28th of this month (with the launch of three additional experiments) and we see significant user drop-off for add-on install prompts.  We haven't had to deal with restarts as well, but we expect it will have a weighty effect on our engagement numbers.  After discussion with some folks in the know (most of the CC list!), we concluded being on the whitelist was the best option.

My apologies for the late bug, we weren't following the e10s rollout plan closely enough.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I would like to get a patch up for review for this issue ASAP. Wil, is that patch going to come from your team?

Comment 2

[Kev Needham [:kev]]

Patch will need to come from Felipe, who manages the add-on, and Shell also needs to review, as she's managing the e10s/addons testing and rollout process. 

The whitelist is mis-named; currently in Fx48 all add-ons that install to an e10s-enabled version of Firefox (e.g. a profile that is not a RTL locale, no add-ons installed, and a11y disabled - roughly 60% of ADIs) will trigger a restart. With 49, we'll be introducing a small set of known, good add-ons for the purposes of testing whether e10s-compatible add-ons affect our stability and perf numbers appreciably. It's less of a whitelist, and more of a small group of add-ons we're testing e10s compatibility against, which will lead to leveling up to all MPC-compatible addons, including WebExtensions, in Fx 50.

Adding more extensions to the test list may add risk to the test, and I'd like Felipe, Shell, and dzeber to weigh in on those risks, and whether it might adversely affect (analysis of) results (given the number of addons we'd be adding, and the potential marketing campaign behind them, it's a real risk). We're using these numbers as a gating release for 50; if our known good add-ons perform as expected, we'll be enabling any addon that is multi-process compatible (MPX true) and webextensions to the e10s cohort.

Because this is a gating mechanism to a broader set of e10s-compatible add-ons, it's really important for us to be able to interpret results in 49.

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I think vlad might have just volunteered to write this in #fxa.

It looks like we need to specify minimum version numbers for each addon. The currently-released addon versions should work:

> Test Pilot:            @testpilot-addon
version 0.8.5

> Tab Center:            [e-mail]
version 1.24

> Activity Stream:       @activity-streams
version 1.1.3

> Min Vid:               @min-vid
version: 0.0.1

> Page Shot:             jid1-NeEaf3sAHdKHPA@jetpack
version: 0.0.1

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Eh, just assigning to :vladikoff, so we don't waste other folks' time writing the same patch.

:felipe, feel free to steal or just review whatever vlad comes up with :-)

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

current webextension versions:

> Tracking Protection:   [e-mail]
version 1.0.0

> No More 404s:          [e-mail]
version 1.5.6

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Ah, we also should add universal search, [e-mail], version 1.0.9

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

> Ah, we also should add universal search, [e-mail], version 1.0.9

Oops, nope. QA needs to take a pass before we whitelist the search addon.

Comment 8

[Vlad Filippov (:vladikoff)]

Attachment: Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

Review commit: https://reviewboard.mozilla.org/r/78818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78818/

Comment 9

[Andy McKay]

Kev, Shell is on PTO until after 49 goes out. 

My only real concern here is matching the data between beta and release and that we don't have this information on beta right now. I would probably say that's not going to buy us much and we should add these. But please correct me if I'm wrong.

Comment 10

[Vlad Filippov (:vladikoff)]

> "need to update browser/extensions/e10srollout/bootstrap.js to update the name"

Let me know what needs be updated there (if needed)

Comment 11

[Kev Needham [:kev]]

(In reply to Andy McKay [:andym] from comment #9)
> My only real concern here is matching the data between beta and release and
> that we don't have this information on beta right now. I would probably say
> that's not going to buy us much and we should add these. But please correct
> me if I'm wrong.

My concern is it adds to complexity of analysis, because we're essentially doubling the number of addons, and they haven't been part of the test process to date. There's no throttling of the test cohort, so it means that if we see differences in the control to the test, it'll make it that much more difficult todetermine whether it's a specific add-on vs. e10s on the whole with add-ons to back out, and if the results are negative it could also affect leveling up in 50 to include MPC-true and WebExtensions in the e10s by default group. 

There's also a marketing campaign planned for these, which may create a larger test cohort than planned, which has been a concern to date.

Timing on this isn't great, but I think everyone understands that. From what I understand the marketing campaign is planned for end of month, so we have a little tie if we want to push a system addon update separately (which I realize has it's own set of processes, and may not be possible for 49 launch date).

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

https://reviewboard.mozilla.org/r/78818/#review77478

You'll need to change the line

`"release" : "49a", // 10 tested add-ons + any WebExtension`

to the name of the new policy that will be used  ("49testpilota")

from browser/extensions/e10srollout/bootstrap.js

Please do that on a separate patch which should help with approvals

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:129
(Diff revision 1)
> +const setTestPilot49 = [
> +  ADDONS.TestPilot,
> +  ADDONS.TabCenter,
> +  ADDONS.ActivityStream,
> +  ADDONS.MinVid,
> +  ADDONS.PageShot
> +]

Can you add the webextension ones too, in case we need to remove the carte blanche to any webextension?

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:153
(Diff revision 1)
> +  "49testpilota": { addons: setTestPilot49, webextensions: true },
> +  "49testpilotb": { addons: setTestPilot49, webextensions: false },

you'll need to union this with the other set, "set49Release".

you can do [...set49Release, ...setTestPilot49]


And can you have another two policies ("a" and "b") where only the Test Pilot add-on itself is whitelisted?

Comment 13

[:Felipe Gomes (needinfo for replies!)]

My suggestion to reduce the risk here is the following:

Only ship with the Test Pilot add-on itself whitelisted. This means that the new install flow to Test Pilot can be used. Then installing the actual test add-ons (hmm confusing naming scheme, how are the called?) would still use the previous flow (with the doorhanger), but that way the main concern is gone and you can handle the flow better from inside the Test Pilot add-on.

Then, with more time to test each individual add-on, we can ship a GoFaster update mid-cycle to add the other ones to the whitelist.

Comment 14

[Vlad Filippov (:vladikoff)]

Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78818/diff/1-2/

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

https://reviewboard.mozilla.org/r/78818/#review77482

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm:139
(Diff revision 2)
> +]
> +
> +// Main Test Pilot add-on + test add-ons for Firefox 49
> +const setTestPilotAddons49 = [
> +  ADDONS.TestPilot,
> +  ADDONS.TabCenter,
> +  ADDONS.ActivityStream,
> +  ADDONS.MinVid,
> +  ADDONS.PageShot,
> +  ADDONS.TrackingProtection,
> +  ADDONS.NoMore404s
> +]

nit: I know the existing one is missing the ending semicolon, but since you're here can you add them?

(after the `]`)

Comment 16

[Vlad Filippov (:vladikoff)]

Attachment: Bug 1302846 part 2 - Enable Test Pilot add-ons in E10S rollout

Comment 17

[Vlad Filippov (:vladikoff)]

Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78818/diff/2-3/

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8791459 [details] [diff] [review]
Bug 1302846 part 2 - Enable Test Pilot add-ons in E10S rollout

Review of attachment 8791459 [details] [diff] [review]:
-----------------------------------------------------------------

the "beta" line just changed on inbound (from bug 1297755) and it should remain unchanged (i.e., keep it configured for 50 as set by bug 1297755).

A pre-emptive r+ for the changes in the "release" line to get the conversation going. Although let's see what's ended up decided about it, specially with comment 13 in mind.

Comment 19

[John Gruen [:jgruen]]

(In reply to :Felipe Gomes (needinfo me!) from comment #13)
> My suggestion to reduce the risk here is the following:
> 
> Only ship with the Test Pilot add-on itself whitelisted. This means that the
> new install flow to Test Pilot can be used. Then installing the actual test
> add-ons (hmm confusing naming scheme, how are the called?) would still use
> the previous flow (with the doorhanger), but that way the main concern is
> gone and you can handle the flow better from inside the Test Pilot add-on.
> 
> Then, with more time to test each individual add-on, we can ship a GoFaster

Requiring restart for Test Pilot experiments will break the on-boarding flow and require refactoring of Test Pilot. I'd rather get reviews in prior to shipping. Our team has dedicated QA if needed to help this process.
> update mid-cycle to add the other ones to the whitelist.

Comment 20

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Kev Needham [:kev] from comment #11)
> (In reply to Andy McKay [:andym] from comment #9)
> > My only real concern here is matching the data between beta and release and
> > that we don't have this information on beta right now. I would probably say
> > that's not going to buy us much and we should add these. But please correct
> > me if I'm wrong.
> 
> My concern is it adds to complexity of analysis, because we're essentially
> doubling the number of addons, and they haven't been part of the test
> process to date. There's no throttling of the test cohort, so it means that
> if we see differences in the control to the test, it'll make it that much
> more difficult todetermine whether it's a specific add-on vs. e10s on the
> whole with add-ons to back out, and if the results are negative it could
> also affect leveling up in 50 to include MPC-true and WebExtensions in the
> e10s by default group. 
> 
> There's also a marketing campaign planned for these, which may create a
> larger test cohort than planned, which has been a concern to date.
> 
> Timing on this isn't great, but I think everyone understands that. From what
> I understand the marketing campaign is planned for end of month, so we have
> a little tie if we want to push a system addon update separately (which I
> realize has it's own set of processes, and may not be possible for 49 launch
> date).

I think what we need to focus on is identifying what work needs to be done in order to be more comfortable with the risk, then work towards that. I like the idea of whitelisting test pilot itself and possibly Activity Stream initially and then working through a prioritized list of additional add-ons.

For the record Kev I don't see the risk as being actual ( I run test pilot and other experiments with e10s all the time ) but I do see the value in engaging in real validation testing.

Comment 21

[Dave Zeber [:dzeber]]

Can we apply Comment 13 to all the Test Pilot add-ons (including TP itself)?

If we add these to the whitelist halfway through the cycle, then we get some basis for comparison. For the first half of the cycle, the e10s-with-addons group would include the same add-ons as in Beta. Then, once the whitelist update gets pushed out, we can see whether there are any substantial changes to the stability metrics for that group. If necessary, we could also potentially do a before/after comparison analysis within the profile histories of profiles with TP.

Even if we don't do this, we should give a separate e10scohort tag to those profiles who have at least one of the Test Pilot add-ons installed. Ideally we could have separate tags for: no add-ons, TP addons only, original whitelist only, original whitelist + TP, all of whom would get e10s switched on. (I'm not sure if this tagging was already included in the patch). This will let us easily split out the groups for stability comparisons.

As it is, we don't really have a control group with the same characteristics (ie. add-ons) but with e10s staying off, so we'll be comparing between groups (ie. between e10s-with-addon and e10s-no-addons). If we can break out profiles that have TP add-ons, and if the groups are all big enough, I don't adding the TP add-ons will add significant risk to the analysis.

Comment 22

[John Gruen [:jgruen]]

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #20)
> (In reply to Kev Needham [:kev] from comment #11)
> > (In reply to Andy McKay [:andym] from comment #9)
> > > My only real concern here is matching the data between beta and release and
> > > that we don't have this information on beta right now. I would probably say
> > > that's not going to buy us much and we should add these. But please correct
> > > me if I'm wrong.
> > 
> > My concern is it adds to complexity of analysis, because we're essentially
> > doubling the number of addons, and they haven't been part of the test
> > process to date. There's no throttling of the test cohort, so it means that
> > if we see differences in the control to the test, it'll make it that much
> > more difficult todetermine whether it's a specific add-on vs. e10s on the
> > whole with add-ons to back out, and if the results are negative it could
> > also affect leveling up in 50 to include MPC-true and WebExtensions in the
> > e10s by default group. 
> > 
> > There's also a marketing campaign planned for these, which may create a
> > larger test cohort than planned, which has been a concern to date.
> > 
> > Timing on this isn't great, but I think everyone understands that. From what
> > I understand the marketing campaign is planned for end of month, so we have
> > a little tie if we want to push a system addon update separately (which I
> > realize has it's own set of processes, and may not be possible for 49 launch
> > date).
> 
> I think what we need to focus on is identifying what work needs to be done
> in order to be more comfortable with the risk, then work towards that. I
> like the idea of whitelisting test pilot itself and possibly Activity Stream
> initially and then working through a prioritized list of additional add-ons.

Test Pilot uses mozAddonManager to handle the installation and uninstallation of individual experiments. This means that we will need to whitelist all Test Pilot + all experiments at the same time, and not just Test Pilot + some, or just Test Pilot. 

Here's the disaster path I want to avoid:

* Test Pilot is whitelisted.
* e10s user installs test pilot add on
* b/c Test Pilot is whitelisted e10s remains on
* user goes to install an experiment in test pilot, install fails over silently (no restart doorhanger)

here's a video of what that looks like: https://www.dropbox.com/s/8y5ointszsefue4/e10s-restart-required-silent-fail.mov?dl=0
 
> For the record Kev I don't see the risk as being actual ( I run test pilot
> and other experiments with e10s all the time ) but I do see the value in
> engaging in real validation testing.

Comment 23

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to [:jgruen] from comment #22)
...
> Test Pilot uses mozAddonManager to handle the installation and
> uninstallation of individual experiments. This means that we will need to
> whitelist all Test Pilot + all experiments at the same time, and not just
> Test Pilot + some, or just Test Pilot. 
> 
> Here's the disaster path I want to avoid:
> 
> * Test Pilot is whitelisted.
> * e10s user installs test pilot add on
> * b/c Test Pilot is whitelisted e10s remains on
> * user goes to install an experiment in test pilot, install fails over
> silently (no restart doorhanger)

Thanks for clarifying, this is a potential disaster.

> here's a video of what that looks like:
> https://www.dropbox.com/s/8y5ointszsefue4/e10s-restart-required-silent-fail.
> mov?dl=0
>  
> > For the record Kev I don't see the risk as being actual ( I run test pilot
> > and other experiments with e10s all the time ) but I do see the value in
> > engaging in real validation testing.

Update: looks like we're engaging with Softvision to prioritize validation of the test pilot add-ons ASAP.

Comment 24

[John Gruen [:jgruen]]

:canuckistani Not a material change for the issue, but for the sake of precision...

Test Pilot uses the SDK AddonManager API to install experiments, not the mozAddonManager API

Comment 25

[Kev Needham [:kev]]

Felipe, is this possible?

(In reply to Dave Zeber [:dzeber] from comment #21)
> 
> Even if we don't do this, we should give a separate e10scohort tag to those
> profiles who have at least one of the Test Pilot add-ons installed. Ideally
> we could have separate tags for: no add-ons, TP addons only, original
> whitelist only, original whitelist + TP, all of whom would get e10s switched
> on. (I'm not sure if this tagging was already included in the patch). This
> will let us easily split out the groups for stability comparisons.

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Not at this point for 49 as that would require significant code changes.

But both telemetry and crash reports include the list of add-ons that the user was using, so that information can still be derived on these sources. Just not by looking at the cohort name.

Comment 27

[Dave Zeber [:dzeber]]

(In reply to :Felipe Gomes (needinfo me!) from comment #26)
> Not at this point for 49 as that would require significant code changes.

Fair enough. We can do this check on the server side, but we'll need to either modify the derived datasets we look at or run against the raw pings.

Comment 28

[John Gruen [:jgruen]]

(In reply to Dave Zeber [:dzeber] from comment #27)
> (In reply to :Felipe Gomes (needinfo me!) from comment #26)
> > Not at this point for 49 as that would require significant code changes.
> 
> Fair enough. We can do this check on the server side, but we'll need to
> either modify the derived datasets we look at or run against the raw pings.

Splitting out cohorts server side seems to obviate the risk Kev raised about confounding the e10s data by adding Test Pilot + Experiments. 

One note, before landing this patch, we will need to add Universal Search to the list of test pilot experiments being added.

The requisite info for universal search is:

> Universal Search:   [e-mail]
> Min Version: 1.0.10

Comment 29

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

wil, elan, kev, jgriffiths, jgruen and I met this morning. Updating this bug with a summary.

This bug remains a priority and wants to be resolved as soon as possible. In the meantime, the Test Pilot team is moving forward with a patch to mitigate the unfavorable UI in the current flow.

With respect to this bug: it looks like we're cleared for a checkin once comment 28 is applied (:vladikoff), and :shell has a chance to review and approve these additions to the list. It also looks like the cohort/analysis concern is cleared in comment 27 (:kev?).

Comment 30

[Chris H-C :chutten]

(In reply to Dave Zeber [:dzeber] from comment #27)
> (In reply to :Felipe Gomes (needinfo me!) from comment #26)
> > Not at this point for 49 as that would require significant code changes.
> 
> Fair enough. We can do this check on the server side, but we'll need to
> either modify the derived datasets we look at or run against the raw pings.

Do we expect this cohort to be large enough to warrant special attention? I'm thinking that it might be of too small a size to move the needle as part of a larger cohort, or to give us statistically-significant results on its own.

Comment 31

[Dave Zeber [:dzeber]]

(In reply to Chris H-C :chutten from comment #30)
> Do we expect this cohort to be large enough to warrant special attention?

Not necessarily - if we don't see any significant issues, we may not need to delve into this. The question is, if we do see a major increase in, say, crashes, is this due to: some change in 49? e10s getting turned on? e10s interacting with TP? Without a control group(s), we don't really have a basis to make these comparisons. However, if we do need to investigate, we can try separating TP users by checking add-ons in the raw pings

Comment 32

[Vlad Filippov (:vladikoff)]

Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78818/diff/3-4/

Comment 33

[Vlad Filippov (:vladikoff)]

(In reply to [:jgruen] from comment #28)
> (In reply to Dave Zeber [:dzeber] from comment #27)
> > (In reply to :Felipe Gomes (needinfo me!) from comment #26)
> > > Not at this point for 49 as that would require significant code changes.
> > 
> > Fair enough. We can do this check on the server side, but we'll need to
> > either modify the derived datasets we look at or run against the raw pings.
> 
> Splitting out cohorts server side seems to obviate the risk Kev raised about
> confounding the e10s data by adding Test Pilot + Experiments. 
> 
> One note, before landing this patch, we will need to add Universal Search to
> the list of test pilot experiments being added.
> 
> The requisite info for universal search is:
> 
> > Universal Search:   [e-mail]
> > Min Version: 1.0.10

Added 

Comment 34

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Clearing kev and shell's NI as this was approved over email.

NI :felipe for review on comment 32. Once done, we'll be requesting uplift straight to mozilla-release with the hopes of catching a dot release.

Comment 35

[:Felipe Gomes (needinfo for replies!)]

No need for a re-review there. It was just one addition to the list, and the r+ was carried forward.

Comment 36

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Comment on attachment 8791404 [details]
Bug 1302846 - Add Test Pilot add-ons to e10s whitelist

Approval Request Comment
[Feature/regressing bug #]:
See comment 22 for information. We made some mitigations in the UI to warn users of the restart, but a significant number of our users are exposed to this.

[User impact if declined]:
The requirement to restart the browser after install will affect many users who are hoping to get in and start experimenting with Test Pilot

[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 37

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Comment on attachment 8791459 [details] [diff] [review]
Bug 1302846 part 2 - Enable Test Pilot add-ons in E10S rollout

See comment 36 for reasons.

Comment 38

[:shell escalante]

NOTE:  we need to add a few more ID's to this list.  Sending to Felipe in next hour or so - to make change and request mconley reviews.  Jorge's providing info to Shell =]

Comment 39

[:shell escalante]

Here are the additions:

YouTube High Definition
https://addons.mozilla.org/firefox/addon/youtube-high-definition/
7b1bf0b6-a1b9-42b0-b75d-252036438bdc 48.3


Weather Underground
https://addons.mozilla.org/firefox/addon/weather-forecast-plus/
jid1-w3xH9kJhd3KJUp@jetpack 0.2.1


Dictionary Anywhere
https://addons.mozilla.org/firefox/addon/dictionary-anywhere/
jid0-fbHwsGfb6kJyq2hj65KnbGte3yT@jetpack 0.1.9


Gmail Notifier
https://addons.mozilla.org/firefox/addon/gmail-notifier-restartless/
jid0-GjwrPchS3Ugt7xydvqVK4DQk8Ls@jetpack 0.6.5

Comment 40

[:Felipe Gomes (needinfo for replies!)]

Changing bug title to also include the 4 add-ons planned to be presented in the Discovery Pane during 49.

Comment 41

[:Felipe Gomes (needinfo for replies!)]

Attachment: Add new requested add-ons to the list

This removes the "test pilot only" set because the Test Pilot team said that one wouldn't be useful, and adds the 4 new requested add-ons (from comment 39) into a new set

Comment 42

[:Felipe Gomes (needinfo for replies!)]

Attachment: Enable new set of add-ons in E10srollout

And this enables this configuration in the system add-on, bumping its version.

(This patch makes the patch "part 2" obselete.)

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8793910 [details] [diff] [review]
Add new requested add-ons to the list

Review of attachment 8793910 [details] [diff] [review]:
-----------------------------------------------------------------

Tentative r+, assuming the testpilot sets never got shipped to users (I don't recall that they did, but just making sure).

::: toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm
@@ -138,5 @@
> -]
> -
> -// Main Test Pilot add-on for Firefox 49
> -const setTestPilot49 = [
> -  ADDONS.TestPilot

Are we certain that this removal, plus the changes on line 196-197, don't violate http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/toolkit/mozapps/extensions/internal/E10SAddonsRollout.jsm#67 ? Like, none of this got sent out to users, right?

@@ +165,5 @@
>    ADDONS.UniversalSearch,
> +];
> +
> +const set49Expanded = [
> +  ...set49Release,

Might as well be consistent and either put the space before set49Release, or get rid of it on the next line. I'm not sure which one is better. But consistency trumps. :)

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8793911 [details] [diff] [review]
Enable new set of add-ons in E10srollout

Review of attachment 8793911 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/e10srollout/bootstrap.js
@@ +16,5 @@
>    "release" : 1.0,  // 100%
>  };
>  
>  const ADDON_ROLLOUT_POLICY = {
> +  "beta"    : "49expandeda", // 10 tested add-ons + Test Pilot add-ons + any WebExtension

Not 10 tested add-ons anymore - we just added a bunch more. Is it worth just dropping the number at this point?

::: browser/extensions/e10srollout/install.rdf.in
@@ +9,5 @@
>       xmlns:em="http://www.mozilla.org/2004/em-rdf#">
>  
>    <Description about="urn:mozilla:install-manifest">
>      <em:id>[e-mail]</em:id>
> +    <em:version>1.4</em:version>

:D

Comment 45

[:Felipe Gomes (needinfo for replies!)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #43)
> > -// Main Test Pilot add-on for Firefox 49
> > -const setTestPilot49 = [
> > -  ADDONS.TestPilot
> 
> Are we certain that this removal, plus the changes on line 196-197, don't
> violate
> http://searchfox.org/mozilla-central/rev/
> 8910ca900f826a9b714607fd23bfa1b37a191eca/toolkit/mozapps/extensions/internal/
> E10SAddonsRollout.jsm#67 ? Like, none of this got sent out to users, right?

Correct. Those sets were from the previous patch on this bug, which hasn't landed yet. If we land them all together this should be ok.


> 
> @@ +165,5 @@
> >    ADDONS.UniversalSearch,
> > +];
> > +
> > +const set49Expanded = [
> > +  ...set49Release,
> 
> Might as well be consistent and either put the space before set49Release, or
> get rid of it on the next line. I'm not sure which one is better. But
> consistency trumps. :)

Yeah I just noticed that after posting.. my eye twitched too. I'll fix it on landing

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #44)
> >  const ADDON_ROLLOUT_POLICY = {
> > +  "beta"    : "49expandeda", // 10 tested add-ons + Test Pilot add-ons + any WebExtension
> 
> Not 10 tested add-ons anymore - we just added a bunch more. Is it worth just
> dropping the number at this point?

Good call

Comment 46

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8793910 [details] [diff] [review]
Add new requested add-ons to the list

Approval Request Comment
[Feature/regressing bug #]: This adds 4 other add-ons that have been tested and are planned to be featured in the "Discovery Pane" of the Add-ons Manager.

Since we are updating the whitelist with the Test Pilot add-ons, might as well take the opportunity and do it here at the same time.

Comment 47

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8793911 [details] [diff] [review]
Enable new set of add-ons in E10srollout

Approval Request Comment
[Feature/regressing bug #]: This configures the system add-on to use the newly updated list

Comment 48

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Hi Lizz - wondering if you've had a chance to review this for uplift to release? We'd like to be in the position to catch a ride on 49.0.2 if/when it happens.

Comment 49

[Liz Henry (:lizzard) (relman/hg->git project)]

So far, no driver for a 49.0.2 has emerged. We have several potential ridealongs. We can take another look next week and re-evaluate. Here, it sounds like people trying out Test Pilot would have to restart to see the changes.  For a dot release, one thing we know is that we "orphan" a significant amount of users with each dot release. So, right now, I'm trying to avoid having to do one, and I'm not sure the restart users outweigh that consideration of users stuck on old versions of Firefox.

Comment 50

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

NI :ritu - assuming there is no dot release, can we please uplift this to 50?

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Cory Price [:ckprice] from comment #50)
> NI :ritu - assuming there is no dot release, can we please uplift this to 50?

Sure if that's the decision of the product team. Felipe, do you want to request an uplift to Aurora51 and Beta50 for this one?

Comment 52

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch for aurora and beta, author=vladikoff r=felipe

I made some simplifications to the patch due to it not being used in release, and squashed everything together to simplify the approval request

Approval Request Comment
[Feature/regressing bug #]: Add the current set of Test Pilot and Discovery Pane addons to the e10s rollout whitelist. This _might_ not be necessary if bug 1297755 rolls out to release, but that's still unclear, so it's better to have these lists ready to go. 
[User impact if declined]: Installing these add-ons will require a restart and will disable e10s
[Describe test coverage new/current, TreeHerder]: just adding things to the whitelist already in use
[Risks and why]: minimal, limited to the e10s whitelist
[String/UUID change made/needed]: none

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798579 [details] [diff] [review]
Patch for aurora and beta, author=vladikoff r=felipe

I am told these add-ons have been tested in e10s mode, Aurora51+, Beta50+

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a32205a1483

Comment 55

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e1133eb4dbe

Comment 56

[Liz Henry (:lizzard) (relman/hg->git project)]

We are planning a dot release now and could consider taking this as a ridealong. That way, users who try out the Test Pilot add-ons can also enable e10s and they won't have to restart. 

Do we have any info about how well this is working on beta?

Comment 57

[Paul Oiegas [:poiegas], PM for (Ecosystem QA, Services QA, WebCompat QA, Hubs QA teams)]

Hi Liz, 

I'm one of the embedded QAs for Test Pilot. We are testing 90% of the time Test Pilot add-on together with all the experiments on Nightly channel with e10s enabled. From July and until now, no major issues were observed between e10s and anything from Test Pilot.

However, if you guys want, we can double check this by using the latest Fx beta next week and provide some results. Just let us know so we can schedule the tests.

Comment 58

[:shell escalante]

Hi John,
Hi Cory,

There is a dot release that is being looked at shipping, is this still needed before Fx50?  I think test pilot was trying to hit a marketing date - and i'm not sure if that is still relevant.

Comment 59

[Liz Henry (:lizzard) (relman/hg->git project)]

From talking with shell and Cory, sounds like we would prefer to hold off until 50 at this point.

Comment 60

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #59)
> From talking with shell and Cory, sounds like we would prefer to hold off
> until 50 at this point.
Confirmed -- it's a nice to have on the Test Pilot side at this point, but we can wait a few more weeks.

Comment 61

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 62

[Andy McKay]

Wil, I think we can close this now?

Comment 63

[Wil Clouser [:clouserw]]

I think so.  Thanks everyone!