1277905 - Remove plugincheck links from Firefox

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Benjamin Smedberg]

plugincheck is not an important part of our security strategy for Firefox any more, and is a continuing source of frustration for users when the blocklist doesn't match plugincheck.

We're going to remove all plugincheck links from the product, and rely on the blocklist to inform users when their plugins are out of date.

This means that all future plugin blocks should be issued with an infoURL, or users will be directed to the AMO blocklist page, which is rarely useful.

Comment 1

[Jennifer Bertsch [:jbertsch]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #0)
> plugincheck is not an important part of our security strategy for Firefox
> any more, and is a continuing source of frustration for users when the
> blocklist doesn't match plugincheck.
> 
> We're going to remove all plugincheck links from the product, and rely on
> the blocklist to inform users when their plugins are out of date.
> 
> This means that all future plugin blocks should be issued with an infoURL,
> or users will be directed to the AMO blocklist page, which is rarely useful.

Hi bsmedberg-

Will the links be removed from old versions of the product or just the current version.

What is your timeline?

Thanks,
Jen

Comment 2

[Benjamin Smedberg]

This will ride the trains, probably FF50.

Comment 3

[Benjamin Smedberg]

Attachment: Bug 1277905 - Remove in-product links to plugincheck. Also remove support for startup prompting about outdated plugins, since that's annoying to users.

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

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8767202 [details]
Bug 1277905 - Remove in-product links to plugincheck. Also remove support for startup prompting about outdated plugins, since that's annoying to users.

https://reviewboard.mozilla.org/r/61812/#review58598

Tentative r+ assuming mconley is going to look at this as well - I'm not familiar with the code so an extra pair of eyes wouldn't go amiss. That said, looks OK to me minus the points below.

::: browser/modules/PluginContent.jsm:449
(Diff revision 1)
> +        {
> -        let updateLink = this.getPluginUI(plugin, "checkForUpdatesLink");
> +          let updateLink = this.getPluginUI(plugin, "checkForUpdatesLink");
> -        this.addLinkClickCallback(updateLink, "forwardCallback", "openPluginUpdatePage");
> +          let pluginTag = this._getPluginInfo(plugin).pluginTag;
> +          this.addLinkClickCallback(updateLink, "forwardCallback", "openPluginUpdatePage", pluginTag);
> +        }
>          /* FALLTHRU */

Nits:

- AFAICT this doesn't need to be in a block (that is, none of the variables here conflict), and the block makes the indent different from the other cases, and looks odd considering this is a fallthru case...
- slightly more DRY: `let {pluginTag} = this._getPluginInfo(plugin);`

::: browser/modules/PluginContent.jsm:604
(Diff revision 1)
>    },
>  
>    // Forward a link click callback to the chrome process.
> -  forwardCallback: function (name) {
> -    this.global.sendAsyncMessage("PluginContent:LinkClickCallback", { name: name });
> +  forwardCallback: function (name, pluginTag) {
> +    this.global.sendAsyncMessage("PluginContent:LinkClickCallback",
> +      { name: name, pluginTag: pluginTag });

Nit: {name, pluginTag}

::: dom/plugins/base/nsPluginHost.cpp:2253
(Diff revision 1)
>        // If the blocklist says this is an outdated plugin, warn about
>        // outdated plugins.

Nit: remove these 2 comment lines.

::: toolkit/locales/jar.mn:90
(Diff revision 1)
>    locale/@AB_CD@/global/webapps.properties              (%chrome/global/webapps.properties)
>    locale/@AB_CD@/global/wizard.dtd                      (%chrome/global/wizard.dtd)
>    locale/@AB_CD@/global/wizard.properties               (%chrome/global/wizard.properties)
>    locale/@AB_CD@/global/crashes.dtd                     (%crashreporter/crashes.dtd)
>    locale/@AB_CD@/global/crashes.properties              (%crashreporter/crashes.properties)
>  % locale global-region @AB_CD@ %locale/@AB_CD@/global-region/

AFAICT chrome://global-region/locale/ and chrome://global-region/content/ are now all dead, so please kill all of that as well.

::: toolkit/mozapps/extensions/content/extensions.js:3228
(Diff revision 1)
>            "details.notification.outdated",
>            [this._addon.name], 1
>          );
>          let warningLink = document.getElementById("detail-warning-link");
>          warningLink.value = gStrings.ext.GetStringFromName("details.notification.outdated.link");
> -        warningLink.href = Services.urlFormatter.formatURLPref("plugins.update.url");
> +        warningLink.href = this._addon.blocklistURL;

Based on the tests, it looks like this URL is not guaranteed to exist. Is that right, and in that case, shouldn't we hide this item if we don't have a URL for it? Same question for the similar changes in extensions.xml

::: toolkit/mozapps/extensions/test/browser/browser-common.ini
(Diff revision 1)
>  [browser_uninstalling.js]
>  [browser_install.js]
>  [browser_recentupdates.js]
>  [browser_manualupdates.js]
>  [browser_globalwarnings.js]
> -[browser_globalinformations.js]

Rather than removing this outright, shouldn't we test that this continues to work with the blocklist URL?

Comment 5

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

Comment on attachment 8767202 [details]
Bug 1277905 - Remove in-product links to plugincheck. Also remove support for startup prompting about outdated plugins, since that's annoying to users.

https://reviewboard.mozilla.org/r/61812/#review58912

Beyond what Gijs found, this looks okay to me.

Comment 6

[Benjamin Smedberg]

https://reviewboard.mozilla.org/r/61812/#review58598

> Nits:
> 
> - AFAICT this doesn't need to be in a block (that is, none of the variables here conflict), and the block makes the indent different from the other cases, and looks odd considering this is a fallthru case...
> - slightly more DRY: `let {pluginTag} = this._getPluginInfo(plugin);`

My c++-self may have gotten in the way here: in c++ you can't initialize a variable in the middle of a block when you can skip within the block (e.g. gotos or switches). But I suppose JS just initializes these to `undefined` in block scope. Fixed.

> Based on the tests, it looks like this URL is not guaranteed to exist. Is that right, and in that case, shouldn't we hide this item if we don't have a URL for it? Same question for the similar changes in extensions.xml

There is always a blocklist URI. In general there should also be an infoURL, and that is preferred, but there is at least always some URI that we can point users to.

> Rather than removing this outright, shouldn't we test that this continues to work with the blocklist URL?

This test covers the global notification that used to show up in the addon manager when there was an outdated plugin (linking to plugincheck). Now there is only a per-addon notification linking directly to the update site, and that has separate tests.

Comment 7

[Benjamin Smedberg]

Axel, the try build "l10n" of this is failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be99202dce2a

This is probably related to removing theglobal-region package, which is no longer used:

17:48:22     INFO -  /home/worker/workspace/build/src/obj-l10n/_virtualenv/bin/python /home/worker/workspace/build/src/toolkit/mozapps/installer/l10n-repack.py /home/worker/workspace/build/src/obj-l10n/dist/l10n-stage/firefox ../../dist/xpi-stage/locale-ar \
17:48:22     INFO -  		 \
17:48:24     INFO -  Error: Locale doesn't have a manifest entry for 'global-region'
17:48:25     INFO -  Error: Missing file: ../../dist/xpi-stage/locale-ar/chrome/ar/locale/ar/global-region/region.properties
17:48:26    ERROR -  Traceback (most recent call last):
17:48:26     INFO -    File "/home/worker/workspace/build/src/toolkit/mozapps/installer/l10n-repack.py", line 60, in <module>
17:48:26     INFO -      main()
17:48:26     INFO -    File "/home/worker/workspace/build/src/toolkit/mozapps/installer/l10n-repack.py", line 56, in main
17:48:26     INFO -      non_resources=args.non_resource, non_chrome=NON_CHROME)
17:48:26     INFO -    File "/home/worker/workspace/build/src/python/mozbuild/mozpack/packager/l10n.py", line 250, in repack
17:48:26     INFO -      _repack(app_finder, l10n_finder, copier, formatter, non_chrome)
17:48:26     INFO -    File "/usr/lib/python2.7/contextlib.py", line 24, in __exit__
17:48:26     INFO -      self.gen.next()
17:48:26     INFO -    File "/home/worker/workspace/build/src/python/mozbuild/mozpack/errors.py", line 131, in accumulate
17:48:26     INFO -      raise AccumulatedErrors()
17:48:26     INFO -  mozpack.errors.AccumulatedErrors
17:48:26    ERROR -  make[1]: *** [repackage-zip] Error 1
17:48:26     INFO -  make[1]: Leaving directory `/home/worker/workspace/build/src/obj-l10n/browser/locales'
17:48:26     INFO -  make: *** [repackage-zip-ar] Error 2
17:48:26    ERROR - Return code: 2
17:48:26    ERROR - make installers-ar failed

Comment 8

[Axel Hecht [:Pike]]

The packager stuff is out of my league. glandium?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a95ad12750
Remove in-product links to plugincheck. Also remove support for startup prompting about outdated plugins, since that's annoying to users. r=Gijs r=mconley

Comment 10

[Benjamin Smedberg]

After talking through with sheriff Kwierso, I believe that either one of these is true:

* the `l10n` job is failing because of leftover references to global-region which won't apply once this hits a main tree
OR
* the `l10n` job is failing because the locales still have this package even though the main tree doesn't

Either way this doesn't block landing as a tier-2 job, and we'll file a followup if necessary.

Comment 11

[Mike Hommey [:glandium]]

Facepalm. The l10n job is using the last nightly, not the build result from try. So yeah, when you change things in some ways, that can fail...

Comment 12

[Justin Wood (:Callek)]

self-ni, since I have some prose of near-future to mention in this bug (and link to a [possibly-to-file] bug about it)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7a95ad12750

Comment 14

[Justin Wood (:Callek)]

> self-ni, since I have some prose of near-future to mention in this bug (and
> link to a [possibly-to-file] bug about it)

Basically making l10n built off the corresponding en-US build for the try push is one of my intents for the near future, however I admit getting l10n working for a Taskcluster-based Nightly Graph is my main work for this quarter. And related support towards that project.

My overall plan is articulated in https://bugzilla.mozilla.org/show_bug.cgi?id=1286092#c3

I have a feeling the timeline there won't help in driving confidence here though. That said, if you edit https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/single_locale/try.py#4 on your push to try, you can get the L10n jobs pointing at the correct binary that your initial try push created.

Comment 15

[Axel Hecht [:Pike]]

Good news is that the l10n nightlies are green today.

Comment 16

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

Added to Fx50 release notes.