514327 - Detect outdated plugins and offer upgrade path

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

It is common for users to have old versions of plugins installed. This situation can be improved by:

* Automatically detecting when a plugin is considered outdated
* Offer an simpler upgrade path

This would utilize the existing blocklist architecture.

See: https://wiki.mozilla.org/Firefox/Projects/Plugin_Update_Referrals

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Strings only patch

Here's a strings-only patch to meet the imminent string freeze for toolkit.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP Patch v1

Work-in-progress patch. In-browser notification isn't working yet. Also not implemented yet is opening the mozilla.com plugins page on startup.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 398613 [details] [diff] [review]
Strings only patch

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties
>--- a/browser/locales/en-US/chrome/browser/browser.properties
>+++ b/browser/locales/en-US/chrome/browser/browser.properties
>@@ -48,16 +48,17 @@ imageBlockedWarning=%S will now always b
> imageAllowedWarning=%S will now allow images from %S.
> undo=Undo
> undo.accessKey=U
> 
> # missing plugin installer
> missingpluginsMessage.title=Additional plugins are required to display all the media on this page.
> missingpluginsMessage.button.label=Install Missing Pluginsâ¦
> missingpluginsMessage.button.accesskey=I
>+outdatedpluginsMessage.title=Some plugins used by this page are out of date.

Do you not intend on adding a button to this for the user to be able to take action here? We need the strings for that button and access key.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 398613 [details] [diff] [review]
Strings only patch

uir=beltzner with one suggestion:

outdated.label should be "A newer, safer version is available."

AIUI, those strings are explaining why the offending add-on has been brought to the user's attention.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Strings only patch v2

(In reply to comment #3)
> Do you not intend on adding a button to this for the user to be able to take
> action here? We need the strings for that button and access key.

I was intending to use blockedpluginsMessage.searchButton.label ("Update Plugins...").

(In reply to comment #4)
> outdated.label should be "A newer, safer version is available."

That sounds much better - done.

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> Created an attachment (id=398694) [details]
> Strings only patch v2
> 
> (In reply to comment #3)
> > Do you not intend on adding a button to this for the user to be able to take
> > action here? We need the strings for that button and access key.
> 
> I was intending to use blockedpluginsMessage.searchButton.label ("Update
> Plugins...").

Don't reuse strings like that. Create a new one even if it is saying the same thing, the different context may mean it needs to be translated differently.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Strings only patch v3

(In reply to comment #6)
> Don't reuse strings like that. Create a new one even if it is saying the same
> thing, the different context may mean it needs to be translated differently.

Ok - Done.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Strings only patch v3 (fixed)

Grr, last patch was a "malformed" apparently. This one is the same, just not malformed.

Comment 9

[Dave Townsend [:mossop]]

Landed on trunk and branch:

http://hg.mozilla.org/mozilla-central/rev/9feb29bc2201
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7f7b1124292d

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 398751 [details] [diff] [review]
Strings only patch v3 (fixed)

r+a192=beltzner

Comment 12

[Alexander L. Slovesnik]

><!ENTITY  pluginupdatesfound.label "Newer versions of one or more of your plugins where found."> 
Should it be "Newer versions of one or more of your plugins were found"? 
s/where/were?

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> ><!ENTITY  pluginupdatesfound.label "Newer versions of one or more of your plugins where found."> 
> Should it be "Newer versions of one or more of your plugins were found"? 
> s/where/were?

Yeah, landed a typo fix:

http://hg.mozilla.org/mozilla-central/rev/630c06941556
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/14884b184ed6

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

This patch adds:

* Support for severity=1 in blocklist.xml to indicate an outdated plugin.
* "outdated" attribute to nsIPluginTag.
* 2 supporting methods to nsIPluginHost (1 generically useful, 1 specific to outdated plugins).
* Opens a tab on startup if there is outdated plugins, but not after a major software update (that has its own plugin check).
* A new page in the software update wizard to warn about outdated plugins.
* A warning and link beside each item of an outdated plugin in the plugins tab of the Extension Manager. Also, a button to go to the plugin page on mozilla.com regardless of whether there's any outdated plugins detected.
* Unit tests

What it doesn't have:

* Support for negate="true" in blocklist.xml as it is now unneeded (the plan is to mark specific versions as outdated, rather than specific versions as the newest). Also, it would break backwards compatibility.
* An alert for when the blocklist is updated and an outdated plugin is found. Reason: I only remembered this part yesterday, so missed the string freeze :(


Note: The URL to the plugin page on mozilla.com may not be correct and is currently a 404. We're ahead of webdev here; the page is scheduled to go live in early/mid October.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - in-browser notification

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - plugins tab of extension manager

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - plugins page of software update wizard

Looking again at the plugins page of the software update wizard, I'm not entirely happy with not having an "Ok" button. Thoughts?

Comment 18

[Christopher Blizzard (:blizzard)]

Are we able to add semantic information about these updates?  i.e. this is for security and you should update with a giant red stop sign (or some other symbol?)

I really like the drop down on the page.  I wonder if people will find that awesome or totally annoying.  I guess I wonder if there are problems updating plugins from time to time.

I might suggest a "remind me later" if we can do that?  Or are we asking too much in this short time frame?

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #18)
> Are we able to add semantic information about these updates?  i.e. this is for
> security and you should update with a giant red stop sign (or some other
> symbol?)

No, not beyond the 3 severity levels (outdated -> warn, softblock -> disable, block -> blocked). Presumably, if there were a security issue with an outdated plugin, it would be either blocked or softblocked.


> I really like the drop down on the page.  I wonder if people will find that
> awesome or totally annoying.  I guess I wonder if there are problems updating
> plugins from time to time.

I'm hoping "awesome" ;) At the moment, that notification bar will show every time an outdated plugin is used, and there's no way to turn that off (other than to update the plugin). That could be potentially be changed to only show once (and therefore also not show the plugin update page on startup because of that plugin). But I'd be worried about making it too easy to ignore.


> I might suggest a "remind me later" if we can do that?  Or are we asking too
> much in this short time frame?

As in, "remind me in a week" type of thing? I like that, but there's some architectural issues that make that type of thing far more difficult than it should be. There's also the issue of being in string-freeze for toolkit.

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 399938 [details] [diff] [review]
Patch v1

Going to skim over this a few times, will let you know when I'm done.

>diff --git a/content/base/src/nsObjectLoadingContent.h b/content/base/src/nsObjectLoadingContent.h

> enum PluginSupportState {
>   ePluginUnsupported,  // The plugin is not supported (not installed, say)
>   ePluginDisabled,     // The plugin has been explicitly disabled by the
>                        // user.
>   ePluginBlocklisted,  // The plugin is blocklisted and disabled
>+  ePluginOutdated,     // The plugin is concidered outdated, but not disabled

"considered"

>   ePluginOtherState    // Something else (e.g. not a plugin at all as far
>                        // as we can tell).

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>-      gPlugins[name][desc] = { filename    : plugin.filename,
>+      gPlugins[name][desc] = { name        : plugin.name,
>+                               description : plugin.description,
>+                               filename    : plugin.filename,
>                                version     : plugin.version,
>                                homepageURL : homepageURL,
>                                disabled    : plugin.disabled,
>                                blocklisted : plugin.blocklisted,
>                                plugins     : [] };

This change seems unnecessary.

>diff --git a/toolkit/mozapps/extensions/content/extensions.xml b/toolkit/mozapps/extensions/content/extensions.xml

>       <constructor>
>         <![CDATA[
>-          if (this.isBlocklisted || this.isSoftBlocklisted) {
>+          if (this.isBlocklisted || this.isSoftBlocklisted || this.isOutdated) {
>             try {
>               var blocklistMoreInfo = document.getAnonymousElementByAttribute(this, "anonid", "blocklistMoreInfo");
>               var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                                     .getService(Components.interfaces.nsIPrefBranch);
>               var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>                                         .getService(Components.interfaces.nsIURLFormatter);
>-              var url = formatter.formatURLPref("extensions.blocklist.detailsURL");
>+              var url = "";
>+              if (this.isOutdated)
>+                url = formatter.formatURLPref("extensions.blocklist.updatePluginsURL");
>+              else
>+                url = formatter.formatURLPref("extensions.blocklist.detailsURL");

Just put the var inside the if, it isn't scoped like other languages.

>       <constructor>
>         <![CDATA[
>-          if (this.isBlocklisted || this.isSoftBlocklisted) {
>+          if (this.isBlocklisted || this.isSoftBlocklisted || this.isOutdated) {
>             try {
>               var blocklistMoreInfo = document.getAnonymousElementByAttribute(this, "anonid", "blocklistMoreInfo");
>               var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>                                     .getService(Components.interfaces.nsIPrefBranch);
>               var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>                                         .getService(Components.interfaces.nsIURLFormatter);
>-              var url = formatter.formatURLPref("extensions.blocklist.detailsURL");
>+              var url = "";
>+              if (this.isOutdated)
>+                url = formatter.formatURLPref("extensions.blocklist.updatePluginsURL");
>+              else
>+                url = formatter.formatURLPref("extensions.blocklist.detailsURL");

Same again

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 399938 [details] [diff] [review]
Patch v1

I've skimmed over the extensions and update sections and they look ok, but there are a few changes necessary. You will also be needing to get review from a browser peer (maybe gavin if he isn't overloaded) and a plugin peer (probably josh) and eventually a super-review for the API changes (guessing jst).

You're going to need to support the case where extensions.blocklist.updatePluginsURL is not set and not offer to check for updates etc. in that case.

I think that the call to shouldWarnForOutdatedPlugins is going to cause a perf hit on startup as it forces a plugin scan when we wouldn't otherwise need one. I think if at all possible you should try to record in a boolean pref whether a warning is necessary and set that from the blocklist service when a new list is downloaded or something.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #20)
> >diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
> >-      gPlugins[name][desc] = { filename    : plugin.filename,
> >+      gPlugins[name][desc] = { name        : plugin.name,
> >+                               description : plugin.description,
> >+                               filename    : plugin.filename,
> >                                version     : plugin.version,
> >                                homepageURL : homepageURL,
> >                                disabled    : plugin.disabled,
> >                                blocklisted : plugin.blocklisted,
> >                                plugins     : [] };
> 
> This change seems unnecessary.

Ah yes, I forgot to mention that. That object is passed to nsIBlockListService.getPluginBlocklistState(), which expects a nsIPluginTag - the original code didn't provide all the properties of that interface. What I'll do instead, is pass the original nsIPluginTag, which is kept in that plugins array property.

Comment 23

[Dave Townsend [:mossop]]

Correct me if I'm wrong, but if we add a plugin as outdated that the user has installed then when the blocklist next updates they are going to see this UI https://bug455906.bugzilla.mozilla.org/attachment.cgi?id=345402 and be recommended to disable the plugin. However if they install a plugin that is outdated already then we won't disable it by default. This seems like a bit of an inconsistency.

Also the way this is at the moment it seems we only offer plugin updates to the user for plugins that are merely outdated, we don't offer the same for a plugin that is hard blocked which really is more important I think.

Comment 24

[IU]

I sure hope consideration is being given to the fact that not all updated plugins are stable or compatible.  A user may need to have an older plugin for an application to work.  I've run into this very scenario before, where I was trying to manage a Symantec firewall using a newer JRE.  It was not supported and I was forced to use an older JRE.

The user should have the ability to choose, once notified, and the decision should not be forced.

Comment 25

[WildcatRay]

(In reply to comment #24)
> I sure hope consideration is being given to the fact that not all updated
> plugins are stable or compatible.  A user may need to have an older plugin for
> an application to work.  I've run into this very scenario before, where I was
> trying to manage a Symantec firewall using a newer JRE.  It was not supported
> and I was forced to use an older JRE.
> 
> The user should have the ability to choose, once notified, and the decision
> should not be forced.

So long as the user can either override or not be forced to update the plugin update, then it should not be a problem.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #23)
> Correct me if I'm wrong, but if we add a plugin as outdated that the user has
> installed then when the blocklist next updates they are going to see this UI
> https://bug455906.bugzilla.mozilla.org/attachment.cgi?id=345402 and be
> recommended to disable the plugin. However if they install a plugin that is
> outdated already then we won't disable it by default. This seems like a bit of
> an inconsistency.

Oops. For outdated plugins, the user WON'T see that blocklist dialog. That'll be fixed in the next patch. Don't want to take away from the severity of that dialog - its meant to be scary, not a friendly reminder.

 
> Also the way this is at the moment it seems we only offer plugin updates to the
> user for plugins that are merely outdated, we don't offer the same for a plugin
> that is hard blocked which really is more important I think.

Good point. At the moment, for blocklisted plugins:

* The "More information" link goes to the blocklist page on mozilla.com, which gives information on why its blocked. I haven't checked, but I'm assuming that page will eventually have information on updating, or link to the plugin update page. I'm checking with WebDev on this.

* The "Update Plugins..." button launches the Plugin Finder Service. Which currently searches for any relevant plugin, not an updated one. So I think it makes sense for that button to take the user to the plugins update page.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #25)
> So long as the user can either override or not be forced to update the plugin
> update, then it should not be a problem.

Indeed - we're not forcing updates here.

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

This patch fixes all comments so far.

(In reply to comment #21)
> I think that the call to shouldWarnForOutdatedPlugins is going to cause a perf
> hit on startup as it forces a plugin scan when we wouldn't otherwise need one.
> I think if at all possible you should try to record in a boolean pref whether a
> warning is necessary and set that from the blocklist service when a new list is
> downloaded or something.

Agreed - done.

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 401364 [details] [diff] [review]
Patch v2

Couple of tweaks to be made and things to think about here but this is looking really good from my point of view. I'll ask gavin if he's ok with me reviewing the browser side too since I know that code fairly well as it is.

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+pref("extensions.blocklist.updatePluginsURL", "https://www.mozilla.com/%LOCALE%/plugins/");
>+pref("extensions.blocklist.outdatedPluginWarningPending", false);

While we're using the blocklist as a mechanism for this I think the preferences should be slightly different. How about plugins.update.url and plugins.update.notifyUser. These match the extension versions (sort of). Change the constants referring to them throughout to match.

>diff --git a/toolkit/mozapps/extensions/src/nsBlocklistService.js b/toolkit/mozapps/extensions/src/nsBlocklistService.js

>+const SEVERITY_OUTDATED               = 1;

Is 1 the correct value or maybe 0 would be right for something that is in the blocklist but we aren't really warning users about just suggesting they update, has this been discussed anywhere that you know of? I guess it would be nice if we'd ever formulated a policy for severities in the blocklist.

>       for (var i = 0; i < blockEntry.versions.length; i++) {
>         if (blockEntry.versions[i].includesItem(plugin.version, appVersion,
>                                                 toolkitVersion))
>+          // XXXunf what if gBlocklistLevel is set to the outdated level?
>+          if (blockEntry.versions[i].severity == SEVERITY_OUTDATED)
>+            return Ci.nsIBlocklistService.STATE_OUTDATED;

The way I see it, if users have lowered gBlocklistLevel too low then that should kick in in preference.

>       if (plugins[i].blocklisted) {
>         if (state == Ci.nsIBlocklistService.STATE_SOFTBLOCKED)
>           plugins[i].disabled = true;
>       }
>-      else if (!plugins[i].disabled && state != Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
>+      else if (!plugins[i].disabled &&
>+               state != Ci.nsIBlocklistService.STATE_NOT_BLOCKED &&
>+               state != Ci.nsIBlocklistService.STATE_OUTDATED) {
>         addonList.push({
>           name: plugins[i].name,
>           version: plugins[i].version,
>           icon: "chrome://mozapps/skin/plugins/pluginGeneric.png",
>           disable: false,
>           blocked: state == Ci.nsIBlocklistService.STATE_BLOCKED,
>           item: plugins[i]
>         });
>       }
>       plugins[i].blocklisted = state == Ci.nsIBlocklistService.STATE_BLOCKED;
>+      if (state == Ci.nsIBlocklistService.STATE_OUTDATED) {
>+        if (!plugins[i].outdated)
>+          gPref.setBoolPref(PREF_BLOCKLIST_PLUGIN_WARN, true);
>+        plugins[i].outdated = true;
>+      }

You can just fold this if block into the above one as the first else if and simplify the other condition. However you also need to set outdated to false if the blocklist no longer says it is so maybe that might clean up differently. Add a test for that case too please.

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug514327_1.js b/toolkit/mozapps/extensions/test/unit/test_bug514327_1.js

>+  // outdated
>+  do_check_true(blocklist.getPluginBlocklistState(PLUGINS[2], "1", "1.9") == nsIBLS.STATE_OUTDATED);
>+
>+  // not blocked
>+  do_check_true(blocklist.getPluginBlocklistState(PLUGINS[3], "1", "1.9") == nsIBLS.STATE_NOT_BLOCKED);
>+}
>\ No newline at end of file

Please add one.

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug514327_2.js b/toolkit/mozapps/extensions/test/unit/test_bug514327_2.js

>+  // should be marked as outdated in the plugin tag
>+  do_check_true(plugin.outdated);
>+  
>+  // should indicate that a warning should be shown
>+  do_check_true(prefs.getBoolPref("extensions.blocklist.outdatedPluginWarningPending"));
>+}
>\ No newline at end of file

And again.

>diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js

>+var gPluginsPage = {
>+  /**
>+   * Initialize
>+   */
>+  onPageShow: function() {
>+    var phs = CoC["@mozilla.org/plugin/host;1"].
>+                 getService(CoI.nsIPluginHost);
>+    var plugins = phs.getPluginTags({});
>+    var blocklist = CoC["@mozilla.org/extensions/blocklist;1"].
>+                      getService(CoI.nsIBlocklistService);
>+
>+    var hasOutdated = false;
>+    for (let i = 0; i < plugins.length; i++) {
>+      let pluginState = blocklist.getPluginBlocklistState(plugins[i]);
>+      if (pluginState == CoI.nsIBlocklistService.STATE_OUTDATED)
>+        hasOutdated = true;

Break out of the loop at this point.

>diff --git a/xpcom/system/nsIBlocklistService.idl b/xpcom/system/nsIBlocklistService.idl

>   const unsigned long STATE_BLOCKED     = 2;
>+  // Indicates that the item is concidered outdated, and there is a known
>+  // update available.

"considered"

Comment 30

[Dave Townsend [:mossop]]

Comment on attachment 401364 [details] [diff] [review]
Patch v2

Gavin's said he's ok with me reviewing the browser piece of this too, however he's not happy with us replacing the homepage with the update page. Instead just open a new tab with it after everything else has opened so remove the changes from nsBrowserContentHandler.js and add something in http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#295.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  if (gPrefService.getPrefType("extensions.blocklist.updatePluginsURL") != Ci.nsIPrefBranch.PREF_INVALID) {
>+    // only listen for the PluginOutdated event if we have a URL to take the user to
>+    gBrowser.addEventListener("PluginOutdated", gMissingPluginInstaller.newMissingPlugin, true, true);
>+  }

Going to contradict/clarify my earlier review comment. We can rely on the pref being set in browser code since we set it there, it's only in code shared with other apps that we need to be careful, so you don't need this test.


>+  else if (aEvent.type == "PluginOutdated") {
>+    var outdatedNotification = notificationBox.getNotificationWithValue("outdated-plugins");
>+    if (outdatedNotification)
>+      return;
>+    
>+    let iconURL = "chrome://mozapps/skin/plugins/pluginBlocked-16.png";

Let's add a new icon for this case, pluginOutdated-16.png. Just copy the generic plugin lego block for now.

Comment 31

[Tony Chung [:tchung]]

flagging for in-litmus?   we'll need a handful of litmus tests added for this feature.

Comment 32

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #29)
> >+const SEVERITY_OUTDATED               = 1;
> 
> Is 1 the correct value or maybe 0 would be right for something that is in the
> blocklist but we aren't really warning users about just suggesting they update,
> has this been discussed anywhere that you know of? I guess it would be nice if
> we'd ever formulated a policy for severities in the blocklist.

Not that I know of. I can't imagine having any level lower than outdated, so I'll change it to 0 for now, until someone comes up with a solid reason to do either.

Comment 33

[Josh Aas]

Comment on attachment 401364 [details] [diff] [review]
Patch v2

Is it necessary to have outdated state be a part of core plugins? Can't the plugins module simply provide the data necessary to determine if a plugin is outdated, the rest can be dealt without outside the module, in extension or browser code?

The reason we have things like blocklisted as states inside the plugin module is that the states affect the operation of the plugins directly - they aren't loaded for some reason, or they are loaded in a particular way. Being outdated does not affect the core functionality of plugins, that state is just used to determine if we should warn users. If it doesn't affect core plugin functionality then I'd rather not have the plugins module know about it.

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #29)
> However you also need to set outdated to false if
> the blocklist no longer says it is so maybe that might clean up differently.
> Add a test for that case too please.

Turns out this is currently untestable, thanks to the inability to force a reload of the blocklist, and the way the blocklist and plugin host interact.

(In reply to comment #33)
> (From update of attachment 401364 [details] [diff] [review])
> Is it necessary to have outdated state be a part of core plugins? Can't the
> plugins module simply provide the data necessary to determine if a plugin is
> outdated, the rest can be dealt without outside the module, in extension or
> browser code?

I'll see what I can do. Previously it did need to be there, but that may not be the case now.

Comment 35

[Dave Townsend [:mossop]]

(In reply to comment #34)
> (In reply to comment #29)
> > However you also need to set outdated to false if
> > the blocklist no longer says it is so maybe that might clean up differently.
> > Add a test for that case too please.
> 
> Turns out this is currently untestable, thanks to the inability to force a
> reload of the blocklist, and the way the blocklist and plugin host interact.

Wouldn't just adding an additional plugin to the array in test_bug514327_1.js that starts with outdated=true and has no info in the blocklist do the job?

Comment 36

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #35)
> Wouldn't just adding an additional plugin to the array in test_bug514327_1.js
> that starts with outdated=true and has no info in the blocklist do the job?

Yea, but that's not really testing anything - that dummy plugin tag would never get updated anyway, as getPluginBlocklistState() doesn't update plugin tags. Plugin tags are only updated when the blocklist updates, or when the plugin host first initializes. Would need to get a real plugin tag, which requires initializing the plugin host, but then it would never be updated from the blocklist (without forcing an update).

There's quite a bit of code in nsBlocklistService.js that can't be tested because there's no way to force an update/reload.

Anyway, as per comment 33, I've removed the outdated attribute from nsIPluginTag. So testing it that way isn't needed anymore. Next patch coming up very soon.

Comment 37

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v3

This fixes all outstanding reviewer comments, including removing nsIPluginTag.outdated. Also fixes some other misc issues the other patches had.

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 402044 [details] [diff] [review]
Patch v3

>+function outdatedPluginsInfo()

This seems to needlessly pollute the global scope, can you define it inside of newMissingPlugin?

>+  var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
>+                            .getService(Components.interfaces.nsIURLFormatter);

use Cc and Ci

>+  gBrowser.loadOneTab(url, null, null, null, false, false);

{inBackground: false} instead of null, null, null, false, false

Comment 39

[Dave Townsend [:mossop]]

(In reply to comment #38)
> (From update of attachment 402044 [details] [diff] [review])
> >+function outdatedPluginsInfo()
> 
> This seems to needlessly pollute the global scope, can you define it inside of
> newMissingPlugin?
> 
> >+  var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
> >+                            .getService(Components.interfaces.nsIURLFormatter);
> 
> use Cc and Ci

Do the same for blocklistInfo and pluginsMissing

Comment 40

[Dave Townsend [:mossop]]

Comment on attachment 402044 [details] [diff] [review]
Patch v3

This is mostly there. I'm going to chat with beltzner this afternoon over some questions I have about what the relative priorities of the notifications should be, I'll complete the review then.

Can you add an additional test that checks the test plugin is found and isn't outdated at first, then updates the blocklist and checks that the plugin is then outdated and the notify pref is set.

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>-  try {
>-    if (gPrefService.getBoolPref("plugins.hide_infobar_for_missing_plugin"))
>-      return;
>-  } catch (ex) {} // if the pref is missing, treat it as false, which shows the infobar
>-

let's keep this here to save the work, just use something along the lines of:

let prefName = aEvent.type == "PluginOutdated" ? "plugins.hide_infobar_for_outdated_plugin" : "plugins.hide_infobar_for_missing_plugin"

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js

>     case "plugins":
>       prefURL = PREF_EXTENSIONS_GETMOREPLUGINSURL;
>       types = [ [ ["plugin", "true", null] ] ];
>-      showCheckUpdatesAll = false;
>       break;

Only show check updates if there is an update url pref. You probably want to just cache that fact in Startup

>                                homepageURL : homepageURL,
>                                disabled    : plugin.disabled,
>                                blocklisted : plugin.blocklisted,
>                                plugins     : [] };
>     }
>     gPlugins[name][desc].plugins.push(plugin);
>   }
> 
>+  var hasWarningPage = gPref.getPrefType(PREF_PLUGINS_UPDATEURL) != nsIPrefBranch2.PREF_INVALID;
>+
>   var blocklist = Components.classes["@mozilla.org/extensions/blocklist;1"]
>-                            .getService(Components.interfaces.nsIBlocklistService);
>+                            .getService(nsIBlocklistService);
>   for (var pluginName in gPlugins) {
>     for (var pluginDesc in gPlugins[pluginName]) {
>       plugin = gPlugins[pluginName][pluginDesc];
>+      var pluginState = blocklist.getPluginBlocklistState(plugin.plugins[0]);

This will hurt my eyes slightly less if you put the blocklistState into the dummy object we create above, same as the disabled and blocklisted flags.

>diff --git a/toolkit/mozapps/extensions/src/nsBlocklistService.js b/toolkit/mozapps/extensions/src/nsBlocklistService.js

>+        if (state == Ci.nsIBlocklistService.STATE_OUTDATED) {
>+          gPref.setBoolPref(PREF_PLUGINS_NOTIFYUSER, true);
>+        } else {

Nit: the prevailing style is to start the else on a new line.

Comment 41

[Dave Townsend [:mossop]]

Comment on attachment 402044 [details] [diff] [review]
Patch v3

This patch is causing test_bug455906.js to fail

Comment 42

[Dave Townsend [:mossop]]

For the in-page notifications we should only display one per page, even if that page contains missing and blocked and outdated items. In terms of importance we should show outdated > missing > blocked. In terms of styling however the missing notification should be an info notification, outdated and blocked should be warnings.

Comment 43

[Josh Aas]

Comment on attachment 402044 [details] [diff] [review]
Patch v3

+#define NS_ERROR_PLUGIN_OUTDATED    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_PLUGINS,1003)

This is not necessary is it?

Comment 44

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - in-browser notifications

Updating the in-page notification screenshot - this shows all the other plugin-related notifications as well, as comparison.

Comment 46

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #43)
> (From update of attachment 402044 [details] [diff] [review])
> +#define NS_ERROR_PLUGIN_OUTDATED   
> NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_PLUGINS,1003)
> 
> This is not necessary is it?

Good catch - no, not needed any more. Removed.

Comment 47

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #41)
> (From update of attachment 402044 [details] [diff] [review])
> This patch is causing test_bug455906.js to fail

Ah, yes - the data files for that test were using severity="0", and assuming 0 had no meaning. Changing the data files to use severity="-1" and keep its old assumptions, and the test passes again.

Comment 48

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot - plugins page of software update wizard

Comment 49

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v4

Fixes all remaining reviewer comments.

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 403656 [details] [diff] [review]
Patch v4

>-      callback: blocklistInfo
>+      callback: gMissingPluginInstaller.showBlocklistInfo

We're trying to avoid methods where 'this' doesn't work as expected. The callbacks are only needed in newMissingPlugin, so can you define them locally?

E.g.

missingPluginInstaller.prototype.newMissingPlugin = function () {
  function blocklistInfo() {
    ...
  }

  ...
};

Comment 51

[Steffen Wilberg]

Comment on attachment 403654 [details]
 Screenshot - plugins page of software update wizard

You'll want to request ui-review from a specific person (beltzner or faaborg).

I think users are likely to just click the "whatever" button in the software update dialog if the warning is not more prominent there.

Maybe we should add the red plugin icon or a yellow warning triangle icon or something like that.

Or make the default action "Upgrade plugins...", and add a Cancel button.

Comment 52

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #50)
> We're trying to avoid methods where 'this' doesn't work as expected. The
> callbacks are only needed in newMissingPlugin, so can you define them locally?

Fair enough - done.

(In reply to comment #51)
> You'll want to request ui-review from a specific person (beltzner or faaborg).

Bah, I am... I must be selecting a bad autocomplete item.


> Or make the default action "Upgrade plugins..."

This is what happens.

Comment 53

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v4.1

Just the small change recommended by Dao in comment 50.

Comment 54

[Steffen Wilberg]

(In reply to comment #52)
> > Or make the default action "Upgrade plugins..."
> This is what happens.

So if outdated plugins are found, clicking the OK button in the software update dialog opens the link and closes the dialog
+   onWizardFinish: function() { openURL(this._url); }
whereas clicking the link opens that as well
+   var link = document.getElementById("pluginupdateslink");
+   link.setAttribute("href", this._url);
and just leaves the dialog open?

Shouldn't we label the button "Upgrade plugins...", and remove the redundant link? Or replace the link by a description saying something like "click OK to learn how to update your plugins"?

Comment 55

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #54)
> Shouldn't we label the button "Upgrade plugins...", and remove the redundant
> link? Or replace the link by a description saying something like "click OK to
> learn how to update your plugins"?

Its too late in the game to be changing strings (we're in string freeze). A cancel button could potentially be added though.

Comment 56

[Dave Townsend [:mossop]]

Comment on attachment 403913 [details] [diff] [review]
Patch v4.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   var notificationBox = gBrowser.getNotificationBox(browser);
> 
>-  // If there is already a missing plugin notification then do nothing
>-  if (notificationBox.getNotificationWithValue("missing-plugins"))
>-    return;
>+  // If there is already an outdated plugin notification then do nothing
>+  if (notificationBox.getNotificationWithValue("outdated-plugins"))
>+    return;
>+  // Should only display one of these warnings per page.
>+  // In order of priority, they are: outdated > missing > blocklisted

Move these last two lines above the previous comment as it then describes the general behaviour and then goes on to perform it.

>   else if (aEvent.type == "PluginNotFound") {
>-    // Cancel any notification about blocklisting
>+    if (missingNotification)
>+      return;
>+
>+    // Cancel any notification about blocklisting/outdated plugins
>     if (blockedNotification)
>       blockedNotification.close();

You're only cancelling the blocklisted notification, correct the comment to say that.

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug514327_3.js b/toolkit/mozapps/extensions/test/unit/test_bug514327_3.js

>+var WindowWatcher = {
>+  openWindow: function(parent, url, name, features, arguments) {
>+    // Should be called to list the newly blocklisted items
>+    do_check_eq(url, URI_EXTENSION_BLOCKLIST_DIALOG);
>+    // Should only include one item
>+    do_check_eq(arguments.wrappedJSObject.list.length, 1);
>+    // And that item should be the blocked plugin, not the outdated one
>+    var item = arguments.wrappedJSObject.list[0];
>+    do_check_true(item instanceof Ci.nsIPluginTag);

You'll need |item.item instanceof Ci.nsIPluginTag| if you want this test to pass...

Comment 57

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #56)
> You'll need |item.item instanceof Ci.nsIPluginTag| if you want this test to
> pass...

Hm, was sure I tested after that (last-minute) addition. Passing tests is... good.

Comment 58

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v4.2

Comment 59

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a80414164888

Comment 60

[Dão Gottwald [:dao]]

Bug 516603 should land before this on branch, but that's still waiting for approval...

Comment 61

[Johnathan Nightingale [:johnath]]

(In reply to comment #60)
> Bug 516603 should land before this on branch, but that's still waiting for
> approval...

Isn't bug 516603 just about cleaning up other callers? I understand wanting to get both in, but can you help me understand why the order matters?

Comment 62

[Dão Gottwald [:dao]]

It touches code that this bug touches too. We need to fix conflicts twice if we change the order.

Comment 63

[Johnathan Nightingale [:johnath]]

Done.

Comment 64

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7ab776c96c37
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/73cfa7816a2b

Comment 65

[Tony Chung [:tchung]]

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2b1) Gecko/20091019 Firefox/3.6b1

Verified fix on trunk and 1.9.2 branch for windows and mac.   Tested all the scenarios for the 3 different notification bars, manipulating the blocklist.xml and the different severities.   If interested, all testcases are listed at: https://wiki.mozilla.org/QA/Firefox3.6/TestPlan:Plugin_Update_Referrals#Test_Cases

Comment 66

[Johnny Stenback (:jst)]

For the record, this commit "caused" top crash bug 520639. And by caused, I mean made it made an existing problem show up way more, which led us to actually finding that problem.

Comment 67

[Vlad [QA]]

This issue is still reproducible for me on Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0 beta 5

The same behavior is reproducible also on:
Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:7.0a1) Gecko/20110608 Firefox/7.0a1 

The notification bar is missing if i have an older flash version installed and i watch a youtube video. Also, the "check for updates" button from the add-on manager doesn't search for new updates if i have an older version.

Comment 68

[Dave Townsend [:mossop]]

(In reply to comment #67)
> This issue is still reproducible for me on Mozilla/5.0 (Windows NT 5.1;
> rv:5.0) Gecko/20100101 Firefox/5.0 beta 5
> 
> The same behavior is reproducible also on:
> Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:7.0a1) Gecko/20110608
> Firefox/7.0a1 
> 
> The notification bar is missing if i have an older flash version installed
> and i watch a youtube video. Also, the "check for updates" button from the
> add-on manager doesn't search for new updates if i have an older version.

This is because the outdated plugin information still hasn't been added to the blocklist. not because this feature isn't present in the product.

Comment 69

[Dave Townsend [:mossop]]

Note that as a rule we don't re-open old bugs, file new bugs if you are seeing problems.