1038145 - Make use of the new infoURL item from the blocklist for plugins with an update

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

Currently when a plugin is marked out-of-date (vulnerable, needs an  update), the Firefox UI links the user to plugincheck.

In bug 873093 we now have the ability to add an infoURL to plugin blocklist entries so that we can link users directly to a better URL to update their plugin. We should prefer that URL to plugincheck.

If a block contains an infoURL, we should always prefer that to the default URL that we construct in-product, even for other blocklist types.

Jorge, can you please make the infoURL for all old-blocked Flash versions https://get.adobe.com/flashplayer/ in production?

Comment 1

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/annotate/f7aef4fc9d47/browser/base/content/browser-plugins.js#l893 is the code which needs to change. I propose a new API nsIBlocklistService.getPluginInfoURL which will return an empty/undefined value if there is no value.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #0)
> Jorge, can you please make the infoURL for all old-blocked Flash versions
> https://get.adobe.com/flashplayer/ in production?

Done. It can take up to an hour for the changes to apply.

Comment 3

[Chris More [:cmore]]

Does this have a target milestone yet? Just want to make sure the websites team is aware so if we see traffic drop off, we don't freak out. :)

Comment 4

[Benjamin Smedberg]

No, but it's on my shortlist of things that are easy and high-win.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145.patch

This patch modifies the nsIBlockService IDL and the relative JS implementation to add support for |getPluginInfoURL|, which returns the url from the |infoURL| tag in the blocklist file for the related plugin or null if the value is not available.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145.patch

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_tests.patch

This patch adds two XPCSHELL tests for the blocklist service to check that |getPluginInfoURL| returns the correct value when the infoURL tag is available (a string) and when it is not (null).

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8498673 [details] [diff] [review]
bug1038145.patch

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

This looks good, thanks!
Some comments below.

::: browser/base/content/browser-plugins.js
@@ +248,5 @@
>  
>        let url;
> +      // If a block contains an infoURL, we should always prefer that to the default
> +      // URL that we construct in-product, even for other blocklist types.
> +      let infoURL = Services.blocklist.getPluginInfoURL(pluginInfo.pluginTag);

I would just default |url| to the value here and get rid of the ternaries below.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +1044,5 @@
> +   * @param   plugin
> +   *          The nsIPluginTag to get the blocklist state for.
> +   * @returns True if the blockEntry matches the plugin, false otherwise.
> +   */
> +  _getPluginMatch: function Blocklist_getPluginMatch(blockEntry, plugin) {

This doesnt return a matching plugin, it checks if there is a match - maybe something like |hasMatchingPluginName|?
This doesnt need access to members, so it doesnt need to be a method.

@@ +1050,5 @@
> +    for (let name in blockEntry.matches) {
> +      if (!(name in plugin) ||
> +          typeof(plugin[name]) != "string" ||
> +          !blockEntry.matches[name].test(plugin[name])) {
> +        matchFailed = true;

Just return |true| directly here and get rid of |matchFailed|.

@@ +1078,5 @@
>      }
>    },
>  
> +  /* See nsIBlocklistService */
> +  getPluginInfoURL: function Blocklist_getPluginInfoURL(plugin) {

Note that we dont need the second name for methods anymore thanks to improved debugging / stack printing.

::: xpcom/system/nsIBlocklistService.idl
@@ +111,4 @@
>  };
>  
>  /**
> + * nsIBlocklistPrompt is used, if available, by the default implementation of

We dont need to touch this line now.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8498685 [details] [diff] [review]
bug1038145_tests.patch

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

We should name the test files more descriptively here, e.g. test_pluginInfoURL.js / pluginInfoURL_block.xml

::: toolkit/mozapps/extensions/test/xpcshell/test_bug1038145.js
@@ +7,5 @@
> +const Ci = Components.interfaces;
> +
> +/**
> + * MockPlugin (taken from test_bug455906.js) mimics the behaviour
> + * of a plugin.

I dont think we need to reference the test this was copied from.

@@ +15,5 @@
> +  this.version = version;
> +  this.enabledState = enabledState;
> +}
> +
> +Object.defineProperty(MockPlugin.prototype, 'blocklisted', {

Just do |MockPlugin.prototype = { get blocklisted() { ...| ?

@@ +29,5 @@
> +  }
> +});
> +
> +// The mocked blocked plugin used to test the blocklist.
> +var PLUGINS = [

s/var/const/

@@ +37,5 @@
> +
> +/**
> + * An helper function to get a Blocklist Service instance.
> + */
> +function makeBlocklist() {

We can just use Services.blocklist from Services.jsm instead of this function.

@@ +49,5 @@
> + * The entry point of the unit tests, which is also responsible of
> + * copying the blocklist file to the profile folder.
> + */
> +function run_test() {
> +  do_print('Setting up the test.');

I dont think we need that.

@@ +74,5 @@
> +  let blocklist = makeBlocklist();
> +
> +  let pluginInfoURL = blocklist.getPluginInfoURL(PLUGINS[0]);
> +
> +  do_print('The infoURL from the blocklist is: ' + pluginInfoURL);

Not needed, we can see both arguments for the comparison below in the failure messages.
Also, i think we can use a few less blank lines in this and the following function :)

@@ +77,5 @@
> +
> +  do_print('The infoURL from the blocklist is: ' + pluginInfoURL);
> +
> +  Assert.strictEqual(blocklist.getPluginInfoURL(PLUGINS[0]),
> +    testInfoURL, 'Should be ' + testInfoURL);

Same here, a message like "plugin info urls should match" would be enough.

@@ +85,5 @@
> + * Test that the blocklist service correctly returns null
> + * if the infoURL tag is missing in the blocklist.xml file.
> + */
> +add_task(function* test_infoURL_missing() {
> +  do_print('Checking the infoURL from the blocklist...');

Dont need this line.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_v2.patch

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_tests_v2.patch

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8498925 [details] [diff] [review]
bug1038145_v2.patch

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

::: browser/base/content/browser-plugins.js
@@ +253,2 @@
>        if (pluginInfo.blocklistState == Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE) {
> +        if (url === null) {

|if (!url) {|

@@ +257,3 @@
>        }
>        else if (pluginInfo.blocklistState != Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
> +        if (url === null) {

|if (!url) {|

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +275,5 @@
> + * @param   plugin
> + *          The nsIPluginTag to get the blocklist state for.
> + * @returns True if the blockEntry matches the plugin, false otherwise.
> + */
> +function hasMatchingPluginName(blockEntry, plugin) {

Hm, so, this function doesnt seem to do what i expect it to (partially on me for missing this on the last pass).
Now it returns true if there is any mismatching or non-existing name on the plugin. This goes counter what the name and description say.
I think we should change the logic here to match them.

@@ +1066,5 @@
>      if (!this._isBlocklistLoaded())
>        this._loadBlocklist();
>  
>      for each (let blockEntry in this._pluginEntries) {
> +      let matchFailed = hasMatchingPluginName(blockEntry, plugin);

The above means you will need to change the name and logic handling here too.

@@ +1086,5 @@
> +    if (!this._isBlocklistLoaded())
> +      this._loadBlocklist();
> +
> +    for each (let blockEntry in this._pluginEntries) {
> +      let matchFailed = hasMatchingPluginName(blockEntry, plugin);

Same here.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_v3.patch

Thank you for your feedback! It is totally on me, I overlooked it! ;)

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8499461 [details] [diff] [review]
bug1038145_v3.patch

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

Thanks, this looks good now. Irving, can you review the toolkit part?

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8499461 [details] [diff] [review]
bug1038145_v3.patch

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

Also, the small browser-plugins change here should be fine with my sign-off.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8498926 [details] [diff] [review]
bug1038145_tests_v2.patch

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

::: toolkit/mozapps/extensions/test/xpcshell/data/pluginInfoURL_block.xml
@@ +3,5 @@
> +<blocklist xmlns="http://www.mozilla.org/2006/addons-blocklist">
> +  <emItems>
> +  </emItems>
> +  <pluginItems>
> +    <pluginItem blockID="test_bug1038145_plugin_wInfoURL">

I think we dont need the bug number in the blockID either.

Comment 17

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8498926 [details] [diff] [review]
bug1038145_tests_v2.patch

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

Nice test, thanks. r+ after you move the test file declaration to xpcshell.ini

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
@@ +220,5 @@
>  [test_no_addons.js]
>  [test_onPropertyChanged_appDisabled.js]
>  [test_permissions.js]
>  [test_permissions_prefs.js]
> +[test_pluginInfoURL.js]

The tests in xpcshell-shared.ini get run twice, with the 'unpack all add-ons' preference True and then False. Since this test isn't affected at all by whether XPI add-ons are installed unpacked, please put it in xpcshell.ini instead of xpcshell-shared.ini.

Comment 18

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8499461 [details] [diff] [review]
bug1038145_v3.patch

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

Needs a little bit of refactoring, but other than that it's fine.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +1064,5 @@
>      if (!this._isBlocklistLoaded())
>        this._loadBlocklist();
>  
>      for each (let blockEntry in this._pluginEntries) {
> +      let hasMatch = hasMatchingPluginName(blockEntry, plugin);

You do the same 'check for enabled; load blocklist; loop through the plugin entries to find the one that matches' here and in getPluginInfoURL(); factor it out into a method _getPluginEntry or similar name of your choosing.

@@ +1069,2 @@
>  
> +      if (hasMatch) {

Just call hasMatchingPluginName() in the 'if ()' condition, rather than declaring a variable and then testing it.

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_browser_tests.patch

This adds a browser mochi test for the infoURL property in the blocklist file.

Comment 20

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_tests_v3.patch

This moves the test entry to xpcshell.ini and removes the bug ID from the xpcshell test xml file.

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_v4.patch

Thank you both for your reviews and feedbacks! This patch deals with the refactoring suggested in the review from :irving.

I am just concerned about one thing: |getPluginBlocklistURL| now returns |null| instead of an empty string if |gBlocklistEnabled| is false. I have run the xpcshell and browser tests locally and it does not seem to be a problem. Should I keep the |if(!gBlocklistEnabled)| out of the newly introduced method and back into |getPluginBlocklistURL|?

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8501758 [details] [diff] [review]
bug1038145_browser_tests.patch

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

This looks good, thanks for figuring this out.
Just some nits below.

::: browser/base/content/test/plugins/browser_pluginnotification.js
@@ +825,5 @@
> +}
> +
> +function prepareTest26() {
> +  info("prepareTest26");
> +  var plugin = getTestPlugin();

|let|, not |var|.

@@ +843,5 @@
> +
> +  // Since the plugin notification is dismissed by default, reshow it.
> +  notification.reshow();
> +
> +  var pluginNode = gTestBrowser.contentDocument.getElementById("test");

|let|, not |var|.

@@ +845,5 @@
> +  notification.reshow();
> +
> +  var pluginNode = gTestBrowser.contentDocument.getElementById("test");
> +  ok(pluginNode, "Test 26, Found plugin in page");
> +  var objLoadingContent = pluginNode.QueryInterface(Ci.nsIObjectLoadingContent);

|let|, not |var|.

@@ +856,5 @@
> +  var infoUrl =
> +    gTestBrowser.contentDocument.getAnonymousElementByAttribute(
> +      PopupNotifications.panel.firstChild,
> +      "anonid",
> +      "click-to-play-plugins-notification-link");

|let|, not |var|. |infoURL| doesn't actually hold the URL value itself - |infoLink| would be more descriptive?
If you use |let doc = ...; let firstChild = ...; let infoLink = doc.get...|, this would probably become more readable.

Comment 23

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_browser_tests_v2.patch

Thank you for your time reviewing this, Georg! This patch addresses the nits in the previous patch.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8503012 [details] [diff] [review]
bug1038145_browser_tests_v2.patch

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

::: browser/base/content/test/plugins/browser_pluginnotification.js
@@ +859,5 @@
> +  let infoLink =
> +    doc.getAnonymousElementByAttribute(
> +      firstPanelChild,
> +      "anonid",
> +      "click-to-play-plugins-notification-link");

I don't think we need to spread this over 5 lines now, should be fine on 2 lines.

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_browser_tests_v3.patch

Thanks, fixed it.

Comment 26

[Georg Fritzsche [:gfritzsche]]

browser test try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=82f0574559dd

The test failure there shows that we are missing a resetBlocklist() call. We should just add that to the cleanup function (see registerCleanupFunction()).

Comment 27

[Georg Fritzsche [:gfritzsche]]

Attachment: Always properly cleanup the blocklist for browser_pluginnotifications.js

Comment 28

[Georg Fritzsche [:gfritzsche]]

Alright, sorry about the pitfalls here Dexter.
The above patch makes sure that browser_pluginnotification.js always cleans the blocklist up properly, so your new tests dont need to worry about cleanup.
If you put your newest patch versions up i can push to try together with that.

Comment 29

[Georg Fritzsche [:gfritzsche]]

No updates to the patches here were needed, so pushed to try together with the cleanup change:

remote: You can view the progress of your build at the following URL:
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=219b80f04584
remote: Alternatively, view them on TBPL (soon to be deprecated):
remote:   https://tbpl.mozilla.org/?tree=Try&rev=219b80f04584

Comment 30

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8501768 [details] [diff] [review]
bug1038145_v4.patch

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

r+ after the code format nits are fixed.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +1081,5 @@
> +
> +  /* See nsIBlocklistService */
> +  getPluginBlocklistURL: function Blocklist_getPluginBlocklistURL(plugin) {
> +    let blockEntry = this._getPluginBlockEntry(plugin);
> +    if(!blockEntry || !blockEntry.blockID) {

Add a space between 'if' and the opening paren (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code):

 if (!blockEntry...)

@@ +1091,5 @@
> +
> +  /* See nsIBlocklistService */
> +  getPluginInfoURL: function (plugin) {
> +    let blockEntry = this._getPluginBlockEntry(plugin);
> +    if(!blockEntry || !blockEntry.blockID) {

As above, 'if ('

Comment 31

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_v5.patch

Thank you for your review Irving, I've taken care of the nits with this patch.

Comment 32

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_browser_tests_v3.patch

I've updated the commit message with r=irving.

Comment 33

[Alessio Placitelli [:Dexter]]

Attachment: bug1038145_browser_tests_v3.patch

Added the r= part in the commit message.

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8504053 [details] [diff] [review]
Always properly cleanup the blocklist for browser_pluginnotifications.js

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

::: browser/base/content/test/plugins/browser_pluginnotification.js
@@ +62,5 @@
>      clearAllPluginPermissions();
>      Services.prefs.clearUserPref("extensions.blocklist.suppressUI");
> +    let deferred = Promise.defer();
> +    setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", deferred.resolve);
> +    return deferred.promise;

return new Promise(resolve => {
  setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", resolve);
});

Comment 35

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/31c202376ea8
https://hg.mozilla.org/integration/fx-team/rev/d79f7f939ab4
https://hg.mozilla.org/integration/fx-team/rev/579a8ed2fe3b
https://hg.mozilla.org/integration/fx-team/rev/d597c195be2d

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31c202376ea8
https://hg.mozilla.org/mozilla-central/rev/d79f7f939ab4
https://hg.mozilla.org/mozilla-central/rev/579a8ed2fe3b
https://hg.mozilla.org/mozilla-central/rev/d597c195be2d

Comment 37

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8504715 [details] [diff] [review]
bug1038145_browser_tests_v3.patch

The browser test patch was exported twice and obsoleted the xpcshell test patch.
As the only change was the commit message, adding r=irving, i landed from the previous version. Adjusting obsolete flags here to reflect that.

Comment 38

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Since this is covered by automatic tests I'm deprioritizing it for manual verification. 

If you feel like there is something not covered by the tests that would require attention from the manual QA team, then feel free to set back the qe-verify+ flag and specify what needs to be covered.

Comment 39

[Georg Fritzsche [:gfritzsche]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #38)
> Since this is covered by automatic tests I'm deprioritizing it for manual
> verification. 
> 
> If you feel like there is something not covered by the tests that would
> require attention from the manual QA team, then feel free to set back the
> qe-verify+ flag and specify what needs to be covered.

The automated test doesn't check that the info link looks and works properly (and makes sense for a user).
This change affects the info (check with Flash & Java):
http://benjamin.smedbergs.us/tests/ctptests/#single-blocked
http://benjamin.smedbergs.us/tests/ctptests/#single-outdated
http://benjamin.smedbergs.us/tests/ctptests/#single-unsafe
... and similar for multiple plugins (sanity check with Flash & Java):
http://benjamin.smedbergs.us/tests/ctptests/#multiple

Comment 40

[Bogdan Maris, Desktop Test Engineering]

I tested using latest Nightly across platforms, I got the same results regarding the OS, see etherpad [0] for more information. This issue seems fixed from QA point of view so I will go and mark this as verified fixed.

[0]: https://etherpad.mozilla.org/infoURL-Blocklist