Closed Bug 1352204 Opened 7 years ago Closed 7 years ago

Only allow webextension and MPC=true (which=no shims) add-ons on Nightly

Categories

(Toolkit :: Add-ons Manager, enhancement, P2)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: shell, Assigned: aswan)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [targeting][triaged])

User Story

User Scenario: Eshan is tracking performance telemetry on the impact of quantum flow and browser tweaks, add-ons with shims performance are skewing the results.  

The best places for them see impact quickly are Nightly - but the population if you remove users with certain add-ons from consideration is too small for significance. 

The proposal from Ehsan, which seems valid based on the 57 goals, is to disable all add-ons that are not webextensions in Nightly and let that ride to Aurora (no further).

The user experience would be seeing an info bar explaining what happened - and that they have the option to re-enable, but this is why if they don't need it - it would help.  LEARN MORE.

Will require SUMO article explaining why (shell needs to file a bug)

Attachments

(3 files)

Bug 1352204 Fix test issues with non-MPC extensions
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1352204 Disallow non-MPC extensions on Nightly
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1352204 Update non-MPC extension notification
59 bytes, text/x-review-board-request
andy+bugzilla
: review+
Details 
The question to Felipe is: can the system add-on change all add-ons that are not webextensions to "disabled" in Nightly and Aurora, and pop a notification bar.

User Scenario: Eshan is tracking performance telemetry on the impact of quantum flow and browser tweaks.

The best places for them see impact quickly are Nightly and Aurora - neither of which are enormous populations.

e10s goes to everyone in Nightly (and aurora?) - including all add-on users.  Currently there is a chance that all add-ons (except webextensions) are interacting with technology that we don't want to factor into performance.  

In the small populations - the SDK and Shim performance issues are making it impossible to get statistically relevant perf telemetry.

The proposal from Eshan, which seems valid based on the 57 goals, is to disable all add-ons that are not webextensions in Nightly and let that ride to Aurora (no further).

The user experience would be seeing an info bar explaining what happened - and that they have the option to re-enable, but this is why if they don't need it - it would help.  LEARN MORE.

Will require SUMO article explaining why (shell needs to file a bug)
Flags: needinfo?(felipc)
The system add-on is not the best place to do that. This should be part of the code of the Add-ons Manager itself.

I don't know how to best do that, I suggest talking with rhelmer or kmag.
Flags: needinfo?(felipc)
Summary: Could the system add-on change all add-ons that are not webextensions to "disabled" in Nightly and Aurora → should we change "disabled" Legacy extensions in Nightly first (ride to aurora) - disable webextensions (can re-enable)
shell - scott.... wording is super hard to share how we feel - " to help us test quantum in next few months - your add-ons have been disabled in Nightly (can be re-enabled) - but confounds perf data"...
Summary: should we change "disabled" Legacy extensions in Nightly first (ride to aurora) - disable webextensions (can re-enable) → Could the system add-on change all add-ons that are not webextensions to "disabled" in Nightly and Aurora
My gut tells me that we'd take a significant press and influencer sentiment hit if we disable all legacy add-ons in Nightly. I also suspect that the users who don't leave Firefox altogether will just re-enable their add-ons, which doesn't solve the data quality problem.

We also risk a double-whammy for users who move to Aurora, only to have the change ride there, or for the channel to go away entirely.

Are there alternatives that might help this be more palatable? Ideas:

- Fix telemetry itself so we can target users without legacy add-ons
- Ask users to opt-in to disabling their add-ons, with an appeal like "Help us test Quantum Flow!"
- Limit the scope to only disabling legacy add-ons that are not marked as multiprocess capable
I was mistaken - the ask is webextensions AND mpc=true only... which is much less severe
Summary: Could the system add-on change all add-ons that are not webextensions to "disabled" in Nightly and Aurora → Could the system add-on change all add-ons that are not webextensions or MPC=True to "disabled" in Nightly and Aurora
Changed the title to reflect this a bit better. The bug to limit to add-ons being loaded into Firefox is bug 1336576, what we'd need to do is set that to true in Nightly and also extend this to MPC=True.

This will mean disabling an awful lot of Nightly users add-ons. Although those ports to WebExtensions are close in many cases, they haven't been uploaded to AMO yet. I remember a similar flow with e10s that I found quite frustrating, Nightly turned on e10s, I found it broken, I turned it off. A few releases later Nightly turned on e10s again, I turned it off. And repeat.
Summary: Could the system add-on change all add-ons that are not webextensions or MPC=True to "disabled" in Nightly and Aurora → Only allow add-ons without shims on Nightly
Summary: Only allow add-ons without shims on Nightly → Only allow add-ons webextension and MPC=true (which=no shims) add-ons on Nightly
Summary: Only allow add-ons webextension and MPC=true (which=no shims) add-ons on Nightly → Only allow webextension and MPC=true (which=no shims) add-ons on Nightly
Component: Extension Compatibility → Add-ons Manager
Product: Firefox → Toolkit
User Story: (updated)
need PM sign off and wording... shell owns completing scenario
Flags: needinfo?(sescalante)
See Also: → 1336576
User Story: (updated)
Blocks: QuantumFlow
Whiteboard: [targeting] → [targeting][qf:p1]
Going back and looking over the schedule and some associated emails I think I can understand what's going on here. Basically we'll have that in Firefox Desktop Nightly 56 we load MPC=true and WebExtensions only. In Firefox 57 we have WebExtensions only. 

Effectively, we've chopped out a bunch of add-ons from Nightly in 56. Given the number of top add-ons that are e10s compatible this doesn't seem too bad to me (by taking a look through arewee10syet.com).

Let's do this as part of the work for 1336576, which aswan will be starting on when he gets back from PTO next week.

Assuming kev signs off on this, as per comment 6.
Assignee: nobody → aswan
Depends on: 1336576
Flags: needinfo?(kev)
Whiteboard: [targeting][qf:p1] → [targeting][triaged]
Whiteboard: [targeting][triaged] → [targeting][triaged][qf:p1]
r=me given following path forward:

- disable all non-webextensions addons _except_ those that are a) MPC=true and b) do not use shims (we have a list)
- notify users that we've disabled addons so they know what's going on
- deploy on nightly for a month, collect data to see if add-ons that don't meet above criteria are skewing results
- if add-ons that use shims are skewing, push to auroroa
- no changes to beta or release at any time

Wording, as I understand it, is as follows:

- We're focusing on improving performance of Firefox, and need to ensure that how we measure those improvements are clean
- Nightly population, which we use to measure impact of perf fixes we land, is quite small, and we need to ensure data we're seeing isn't significantly impacted by external factors, including add-ons
- Shims don't work as well as hoped, which was always a possibility, and add-ons that use shims are likely skewing perf measurements
- Removing shims will help us ensure that changes we're making as part of qflow and other initiatives are making a positive difference
- We'll be disabling add-ons that aren't known to work with a) e10s and b) without shims to ensure we have clean data on improvements we're landing, nad making it better for everyone
Flags: needinfo?(kev)
This will be reversible, right? A user can re-enable an add-on that was disabled?
After conferring with Amy and Jorge, here is proposed copy for the yellow info bar:

Due to critical performance testing, we’ve disabled some of your add-ons. They can be re-enabled in the Add-ons Manager.  [Learn More]
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> This will be reversible, right? A user can re-enable an add-on that was
> disabled?

It should be, yes. e.g. just a one-time disable when shims are removed vs. a hardblock.
I'm concerned that if its user reversible, then telemetry will get polluted with shim data again as people turn it off. That would kind of defeat the purpose.
(In reply to Andy McKay [:andym] from comment #12)
> I'm concerned that if its user reversible, then telemetry will get polluted
> with shim data again as people turn it off. That would kind of defeat the
> purpose.

Is there any way that can actually be done? My understanding was the only way to make it permanent would be to blocklist, which would like be a little much.

The goal is to clean up the data as much as possible, and while I don't disagree the risk is there, understanding what our options are would help, and we could also enlist help from folks like Ben to identify profiles that might be suspect. Is there a patch we could add as part of this to identify webextensions in telemetry?
Basically, we can choose what gets loaded in add-ons manager and limit it there, the problem is that in trying to figure out the best way to implement this I see lots of problems and edge cases.

I'm meeting with a couple of people today to try and get a recommendation together on how we could implement this.
Depends on: 1356027
My plan here:

1. Add a new preference that disables loading extensions that don't explicitly declare MPC=true but don't change the default behavior yet (bug 1356027)
2. Get visual indicators into about:addons about disabled add-ons, instructions for re-enabling etc (perhaps bug 1354680, perhaps a new bug, not sure yet)
3. Flip the default setting for the pref on Nightly

Note that this should take care of shims for all extensions and CPOWs for most extensions, but there is a separate list of extensions that get access to CPOWs.  :billm is working on trimming down part of that list in bug 1355997 but even after this bug and that one are done, there will still be a few instances in which certain extensions can use CPOWs (and users will be able to flip the pref and use shims and CPOWs on Nightly...)
Can I add one more:

4) Please show a popup message (the yellow bar at the bottom of the screen) that lets the user know we've disabled add-ons (presumably if we haven't disabled any, there's no need to show this). This meets one of kevs requirements in comment 8.
(In reply to Andy McKay [:andym] from comment #16)
> Can I add one more:
> 
> 4) Please show a popup message (the yellow bar at the bottom of the screen)
> that lets the user know we've disabled add-ons (presumably if we haven't
> disabled any, there's no need to show this). This meets one of kevs
> requirements in comment 8.

I had that lumped in with step 2, consider that to be "all the user-facing stuff we need to tell users about extensions that have been disabled".
Upon further consideration, I don't think bug 1354680 is the right place to handle that work, I'll file a new bug in a bit and we can bikeshed about exactly which style of notifications to show over there :-)
Ehsan reminds me, step 2.5 is to add the new pref to the telemetry environment.  (and possibly another boolean scalar to indicate if any non-MPC extensions are installed when that pref has been flipped)
Depends on: 1356462
Flags: needinfo?(sescalante)
Depends on: 1358620
I can create test builds for this, Krupa and Andy what platform(s) would you like builds for?
No longer depends on: 1336576
Flags: needinfo?(krupa.mozbugs)
Flags: needinfo?(amckay)
(In reply to Andrew Swan [:aswan] from comment #19)
> I can create test builds for this, Krupa and Andy what platform(s) would you
> like builds for?

I don't need any.
Flags: needinfo?(amckay)
(In reply to Andrew Swan [:aswan] from comment #19)
> I can create test builds for this, Krupa and Andy what platform(s) would you
> like builds for?

I'd like builds for win64, mac 64-bit and Linux pls.
looking at Comment 5 & 6 - can I just clarify - will bug 1336576 land at the same time to include webextensions in the "allowed to run in nightly" - or will patch land first and this disable webextensions?
(In reply to :shell escalante from comment #22)
> looking at Comment 5 & 6 - can I just clarify - will bug 1336576 land at the
> same time to include webextensions in the "allowed to run in nightly" - or
> will patch land first and this disable webextensions?

I don't understand the bit after the hyphen but there is no dependency between this and bug 1336576.  It's true that they do similar things but we have much more extensive UX requirements for 1336576 that I'll be working on over the next week or two.
(In reply to Andrew Swan [:aswan] from comment #24)
> Krupa: test builds are at:
> 
> macos:
> https://queue.taskcluster.net/v1/task/QVgkYn92ShyuoSO_TtTj2A/runs/0/
> artifacts/public/build/target.dmg
> windows:
> https://queue.taskcluster.net/v1/task/K4QvShb9Tw-RYugEO7dmkQ/runs/0/
> artifacts/public/build/firefox-55.0a1.en-US.win64.zip
> linux:
> https://queue.taskcluster.net/v1/task/DXVJ6bSbSkep3WHTtSYPgw/runs/0/
> artifacts/public/build/target.tar.bz2

on it, thanks aswan!
Depends on: 1359824
No longer depends on: 1359824
Depends on: 1360169
Depends on: 1360172
The banner that we show on first startup when the pref is flipped and there are affected extensions is:
Due to performance testing, we have disabled some of your add-ons. They can be re-enabled in the Add-ons Manager.

Andy just pointed out that they cannot actually be re-enabled in the Add-ons Manager.  A user will typically start there though, where they will see disabled extensions, then follow the "more information link" to a page that instructs them to flip a preference in about:config.

The current string has already landed but we can update it if we feel it is important (at the cost of wasting some translators' time if they have already started on the current string).  Scott, what say you?
Flags: needinfo?(sdevaney)
Depends on: 1360687
The attached patches (together with the bug 1360687 patches) are good on try (a couple of tier 2 failures and a couple of intermittents that passed on a re-trigger):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f10c749777e36ec94aa8e21215e198d77cfba8
Attachment #8863145 - Flags: review?(kmaglione+bmo)
Attachment #8863146 - Flags: review?(kmaglione+bmo)
How about this messaging adjustment:

"Due to performance testing, we have disabled some of your add-ons. They can be re-enabled in your browser settings. <Learn More link>"
Flags: needinfo?(sdevaney)
Comment on attachment 8863145 [details]
Bug 1352204 Fix test issues with non-MPC extensions

https://reviewboard.mozilla.org/r/134966/#review137914
Attachment #8863145 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8863146 [details]
Bug 1352204 Disallow non-MPC extensions on Nightly

https://reviewboard.mozilla.org/r/134968/#review137916

::: browser/app/profile/firefox.js:1515
(Diff revision 1)
>  pref("extensions.interposition.enabled", true);
>  pref("extensions.interposition.prefetching", true);
>  
> +// But don't allow non-MPC extensions by default on Nightly
> +#if defined(NIGHTLY_BUILD)
> +pref("extensions.allow-non-mpc-extensions", false);

Please also set this to true in an `#else`.
Attachment #8863146 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8863146 [details]
Bug 1352204 Disallow non-MPC extensions on Nightly

https://reviewboard.mozilla.org/r/134968/#review137916

> Please also set this to true in an `#else`.

There is already a default value of true in `modules/libpref/init/all.js`, if its important to set it here too can you explain the reasoning?
Comment on attachment 8863146 [details]
Bug 1352204 Disallow non-MPC extensions on Nightly

https://reviewboard.mozilla.org/r/134968/#review137916

> There is already a default value of true in `modules/libpref/init/all.js`, if its important to set it here too can you explain the reasoning?

If there isn't a value in the default branch, the preference doesn't show up in about:config, and people who want to change it for testing need to create it themselves.
Comment on attachment 8863146 [details]
Bug 1352204 Disallow non-MPC extensions on Nightly

https://reviewboard.mozilla.org/r/134968/#review137916

> If there isn't a value in the default branch, the preference doesn't show up in about:config, and people who want to change it for testing need to create it themselves.

But isn't the value in `all.js` enough to do that...?  Maybe I am missing something about the pref system myself...  I thought as long as there was some value in `all.js`, the pref would always appear in about:config.
Oh, sorry, I misread. Yeah, a value in all.js should be fine.
(In reply to sdevaney from comment #32)
> How about this messaging adjustment:
> 
> "Due to performance testing, we have disabled some of your add-ons. They can
> be re-enabled in your browser settings. <Learn More link>"

Hm, the notification currently has a link to about:addons, not to the page that explains how to re-enable them.  I think it would be ideal for users to go through that page and see specifically which extensions are affected before they change the pref.  Scott it out for the next few days.  Andy, you want to make the decision here?  We could go with Scott's wording but still have the button lead to about:addons...
Flags: needinfo?(amckay)
+1, that wording and bouncing to about:addons sounds fine to me.
Flags: needinfo?(amckay)
Attachment #8863530 - Flags: review?(amckay)
Comment on attachment 8863530 [details]
Bug 1352204 Update non-MPC extension notification

https://reviewboard.mozilla.org/r/135298/#review138214

lgtm
Comment on attachment 8863530 [details]
Bug 1352204 Update non-MPC extension notification

https://reviewboard.mozilla.org/r/135298/#review138216
Attachment #8863530 - Flags: review?(amckay) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46dd4a4338a1
Fix test issues with non-MPC extensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/cd801baf418d
Disallow non-MPC extensions on Nightly r=kmag
https://hg.mozilla.org/integration/autoland/rev/a6ae98895393
Update non-MPC extension notification r=andym
Here's the try run for the pushed changes.  The notification bar was tested manually.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=320af611c2b0580be5a39e88b95b103bbab4eb31
sorry to be late to this - but is anyone else worried about the wording sounding like the add-ons were disabled because they failed performance testing?  

"Due to performance testing, we have disabled some of your add-ons. They can be re-enabled in your browser settings. <Learn More link>"

We are just disabling these add-ons to limit factors to help us make and test performance improvements.
Flags: needinfo?(sdevaney)
Flags: needinfo?(kev)
Whoops, Talos wasn't covered in "-u all".  This looks like a simple matter of those tests loading non-MPC extensions, here's a good (linux64 opt) try run with the simple tweak:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85ff2c25ff9560c4d3b45355c5afeb39d186c95c
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8f8c11e7b95c -d aebffbddaafc: rebasing 393290:8f8c11e7b95c "Bug 1352204 Fix test issues with non-MPC extensions r=kmag"
merging dom/plugins/test/testaddon/install.rdf
merging layout/tools/reftest/reftest-preferences.js
merging testing/firefox-ui/tests/puppeteer/test_notifications.py
merging testing/marionette/harness/marionette_harness/tests/unit/mn-restartless-unsigned.xpi
merging testing/profiles/prefs_general.js
merging toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
merging toolkit/mozapps/extensions/test/xpcshell/head_addons.js
warning: /repos/mozreview-gecko/testing/marionette/harness/marionette_harness/tests/unit/mn-restartless-unsigned.xpi looks like a binary file.
warning: conflicts while merging testing/marionette/harness/marionette_harness/tests/unit/mn-restartless-unsigned.xpi! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39360ad5ebb2
Fix test issues with non-MPC extensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/92df9a474669
Disallow non-MPC extensions on Nightly r=kmag
https://hg.mozilla.org/integration/autoland/rev/5eb56bf75f48
Update non-MPC extension notification r=andym
Flags: needinfo?(aswan)
(In reply to :shell escalante from comment #49)
> sorry to be late to this - but is anyone else worried about the wording
> sounding like the add-ons were disabled because they failed performance
> testing?  
> 
> "Due to performance testing, we have disabled some of your add-ons. They can
> be re-enabled in your browser settings. <Learn More link>"
> 
> We are just disabling these add-ons to limit factors to help us make and
> test performance improvements.

Good point to ponder but I think given the space constraint, the current message will suffice. It will hopefully be apparent that to cleanly test browser functionality we had to strip away certain types of add-ons.
Flags: needinfo?(sdevaney)
Does this somehow ignore user.js?
I have this in my user.js but when I go to about:config, the value is still false:
user_pref("extensions.allow-non-mpc-extensions", true);	// https://bugzilla.mozilla.org/show_bug.cgi?id=1352204
Other settings I did via user.js work fine.
This bug is verified as fixed on Firefox 55.0a1 (2017-05-07) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.1 and here are the results:
 - non-mpc add-ons are not installed 
 - non-mpc add-ons are disabled during update and they are accompanied by a multiprocess yellow notification bar and a red message in Add-ons Manager: https://www.screencast.com/t/nFDzWJWoGu
 - all non-mpc add-ons are enabled while setting the multiprocess compatible pref to true
 - webextensions, mpc=tre add-ons, dictionaries, lang packs, complete themes are successfully installed regardless the "extensions.allow-non-mpc-extensions" pref value
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(krupa.mozbugs)
Is it possible for the developer to set Nightly to not send tracking data if setting `extensions.allow-non-mpc-extensions = true` in order to test certain addons, so as to not skew performance testing data?

(And sorry that I didn't follow the "more information" link before filing bug 1364807)
(In reply to Kevin Jones from comment #64)
> Is it possible for the developer to set Nightly to not send tracking data if
> setting `extensions.allow-non-mpc-extensions = true` in order to test
> certain addons, so as to not skew performance testing data?

Yes, you can turn off telemetry under Privacy & Security, Reports in the Options window, but please make sure to turn it back on when your testing is finished.  :-)
Flags: needinfo?(kev)
This update disabled most of my add-ons without my consent and removed the enable button for them. The message said it can be re-enabled in browser settings, but there were no such options, I had to search bugzilla for about:config flag to disable this.

I know Nightly is not meant for everyday use, but "we broke your add-ons for our testing purposes, have fun searching for a fix" was not exactly what I wanted to see when I accidentally launched it on my main profile.
(In reply to override.dll from comment #66)
> This update disabled most of my add-ons without my consent and removed the
> enable button for them. The message said it can be re-enabled in browser
> settings, but there were no such options, I had to search bugzilla for
> about:config flag to disable this.
> 
> I know Nightly is not meant for everyday use, but "we broke your add-ons for
> our testing purposes, have fun searching for a fix" was not exactly what I
> wanted to see when I accidentally launched it on my main profile.

What you're seeing is unrelated to this bug. Support for legacy extensions is being removed as of Firefox 47. That means release builds as well as nightlies.
(In reply to Kris Maglione [:kmag] from comment #67)
> (In reply to override.dll from comment #66)
> > This update disabled most of my add-ons without my consent and removed the
> > enable button for them. The message said it can be re-enabled in browser
> > settings, but there were no such options, I had to search bugzilla for
> > about:config flag to disable this.
> > 
> > I know Nightly is not meant for everyday use, but "we broke your add-ons for
> > our testing purposes, have fun searching for a fix" was not exactly what I
> > wanted to see when I accidentally launched it on my main profile.
> 
> What you're seeing is unrelated to this bug. Support for legacy extensions
> is being removed as of Firefox 47. That means release builds as well as
> nightlies.

extensions.legacy.enabled = true

This won't allow updates, but if your addons aren't broken for some other reason on 57 it should allow them to run.
I'm on 54.0a2, "legacy" add-ons are working and I don't plan on disabling them or updating Firefox anytime soon. But when I accidentally launched Nightly on main profile, it decided to disable my add-ons AND remove buttons for re-enabling them. If you plan on breaking stuff in 57+, can you at least break it in a way that doesn't also break anything pre-57? Like, show the "checking your add-ons for compatibility..." dialogue like you used to do.
Performance Impact: --- → P1
Whiteboard: [targeting][triaged][qf:p1] → [targeting][triaged]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: