Remove plugincheck links from Firefox

RESOLVED FIXED in Firefox 50

Status

()

Core
Plug-ins
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(relnote-firefox 50+, firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(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
Flags: needinfo?(benjamin)
(Assignee)

Comment 2

2 years ago
This will ride the trains, probably FF50.
Flags: needinfo?(benjamin)
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 3

a year ago
Created 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.

Review commit: https://reviewboard.mozilla.org/r/61812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61812/
Attachment #8767202 - Flags: review?(mconley)
Attachment #8767202 - Flags: review?(gijskruitbosch+bugs)

Updated

a year ago
Attachment #8767202 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 4

a year ago
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 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.
Attachment #8767202 - Flags: review?(mconley) → review+
(Assignee)

Comment 6

a year ago
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.
(Assignee)

Comment 7

a year ago
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
Flags: needinfo?(l10n)

Comment 8

a year ago
The packager stuff is out of my league. glandium?
Flags: needinfo?(l10n) → needinfo?(mh+mozilla)

Comment 9

a year ago
Pushed by bsmedberg@mozilla.com:
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
(Assignee)

Comment 10

a year ago
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.
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...
Flags: needinfo?(mh+mozilla)
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)
Flags: needinfo?(bugspam.Callek)

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7a95ad12750
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
> 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.
Flags: needinfo?(bugspam.Callek)
Good news is that the l10n nightlies are green today.

Updated

a year ago
Blocks: 1317865
Added to Fx50 release notes.
relnote-firefox: --- → 50+
You need to log in before you can comment on or make changes to this bug.