Closed Bug 1038145 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
mozilla36
Iteration:
36.1

People

(Reporter: benjamin, Assigned: Dexter, Mentored)

References

Details

(Whiteboard: [lang=JS][diamond])

Attachments

(4 files, 11 obsolete files)

3.87 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
8.42 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
3.98 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
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?
Flags: needinfo?(jorge)
Flags: firefox-backlog+
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.
Mentor: georg.fritzsche
Whiteboard: [lang=JS][diamond]
(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.
Flags: needinfo?(jorge)
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. :)
Flags: needinfo?(benjamin)
No, but it's on my shortlist of things that are easy and high-win.
Flags: needinfo?(benjamin)
Assignee: nobody → alessio.placitelli
Attached patch bug1038145.patch (obsolete) — Splinter Review
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.
Attached patch bug1038145.patch (obsolete) — Splinter Review
Attachment #8498667 - Attachment is obsolete: true
Attached patch bug1038145_tests.patch (obsolete) — Splinter Review
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 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.
Attachment #8498673 - Flags: feedback+
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.
Attachment #8498685 - Flags: feedback+
Attached patch bug1038145_v2.patch (obsolete) — Splinter Review
Attachment #8498673 - Attachment is obsolete: true
Attached patch bug1038145_tests_v2.patch (obsolete) — Splinter Review
Attachment #8498685 - Attachment is obsolete: true
Attachment #8498925 - Flags: feedback?(georg.fritzsche)
Attachment #8498926 - Flags: feedback?(georg.fritzsche)
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.
Attachment #8498925 - Flags: feedback?(georg.fritzsche)
Attached patch bug1038145_v3.patch (obsolete) — Splinter Review
Thank you for your feedback! It is totally on me, I overlooked it! ;)
Attachment #8498925 - Attachment is obsolete: true
Attachment #8499461 - Flags: feedback?(georg.fritzsche)
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?
Attachment #8499461 - Flags: feedback?(georg.fritzsche) → review?(irving)
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.
Attachment #8499461 - Flags: review+
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.
Attachment #8498926 - Flags: review?(irving)
Attachment #8498926 - Flags: feedback?(georg.fritzsche)
Attachment #8498926 - Flags: feedback+
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.
Attachment #8498926 - Flags: review?(irving) → review+
Flags: qe-verify+
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.
Attachment #8499461 - Flags: review?(irving) → review-
Attached patch bug1038145_browser_tests.patch (obsolete) — Splinter Review
This adds a browser mochi test for the infoURL property in the blocklist file.
Attachment #8501758 - Flags: feedback?(georg.fritzsche)
This moves the test entry to xpcshell.ini and removes the bug ID from the xpcshell test xml file.
Attachment #8498926 - Attachment is obsolete: true
Attached patch bug1038145_v4.patch (obsolete) — Splinter Review
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|?
Attachment #8499461 - Attachment is obsolete: true
Flags: needinfo?(irving)
Attachment #8501768 - Flags: review?(irving)
Flags: needinfo?(irving)
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.
Attachment #8501758 - Flags: feedback?(georg.fritzsche) → feedback+
Thank you for your time reviewing this, Georg! This patch addresses the nits in the previous patch.
Attachment #8501758 - Attachment is obsolete: true
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.
Attachment #8503012 - Flags: review+
Thanks, fixed it.
Attachment #8503012 - Attachment is obsolete: true
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()).
Status: NEW → ASSIGNED
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.
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 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 ('
Attachment #8501768 - Flags: review?(irving) → review+
Thank you for your review Irving, I've taken care of the nits with this patch.
Attachment #8501768 - Attachment is obsolete: true
Attachment #8504558 - Flags: review+
I've updated the commit message with r=irving.
Attachment #8501765 - Attachment is obsolete: true
Attachment #8504715 - Flags: review+
Added the r= part in the commit message.
Attachment #8503110 - Attachment is obsolete: true
Attachment #8504967 - Flags: review+
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);
});
Attachment #8504053 - Flags: review?(mconley) → review+
Iteration: --- → 36.1
QA Contact: bogdan.maris
Depends on: 1083880
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.
Attachment #8504715 - Attachment is obsolete: true
Attachment #8501765 - Attachment is obsolete: false
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.
Flags: qe-verify+ → qe-verify-
(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
Flags: qe-verify- → qe-verify+
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
Status: RESOLVED → VERIFIED
Depends on: 1129287
You need to log in before you can comment on or make changes to this bug.