831533 - about:plugins should show the enabled/disabled/blocklisted state of plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Chris Peterson [:cpeterson]]

about:plugins does not list "inactive" (i.e. blocklisted or user-disabled) plugins. Since about:plugins is used to diagnose problems, it would be more useful if all installed plugins were listed, including their enabled/disabled/blocklisted status.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Once we have a patch and we can play with it we should also update our Mozmill test.

Comment 2

[Justin Dolske [:Dolske]]

I'd rather WONTFIX this and remove about:plugins entirely. The addons manager should be used for this (adding info there, if needed).

Comment 3

[Georg Fritzsche [:gfritzsche]]

It's rather useful for triaging/testing/developing as it just plainly dumps the plugin information from the plugin host while the addon manager may do additional work (e.g. caching, beautifying names, ...).

Comment 4

[Bill Gianopoulos [:WG9s]]

(In reply to Justin Dolske [:Dolske] from comment #2)
> I'd rather WONTFIX this and remove about:plugins entirely. The addons
> manager should be used for this (adding info there, if needed).

I am posting a strong objection to such change.

Comment 5

[Chris Peterson [:cpeterson]]

Assigning bug to [e-mail]

Comment 6

[Chris Peterson [:cpeterson]]

Sunny, to implement this feature, you will need to:

1. Remove the isActive check from plugins.html so the for() loop will list all plugins, not just the enabled ones:

https://hg.mozilla.org/mozilla-central/file/b52c02f77cf5/toolkit/content/plugins.html#l117

2. You need to add new status field to the HTML for each plugin. The code to add the field will look a lot like the code for the "Version: " field. I suggest sticking the new field after Version and before Plugin Description:

https://hg.mozilla.org/mozilla-central/file/b52c02f77cf5/toolkit/content/plugins.html#l136

I don't know whether this field should be called "Status:", "State: ", or something else. The name does not matter for now; bsmedberg can suggest an appropriate name after we get the feature working. :)

3. plugin.isActive is defined in PluginProvider.jsm:

https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#208

You can see that isActive = (blocklisted || disabled), so it is not useful for determining *why* a plugin is inactive. But you can use plugin.userDisabled and plugin.blocklistState to get more details:

https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#210
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#222

4. The valid blocklistStates are defiend in nsIBlocklistService.idl:

https://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIBlocklistService.idl#15

I think we might want to list both the enabled/disabled state and the blocklist state. So the status code might look something like:

  status = plugin.isActive ? "Enabled" : "Disabled";
  if (plugin.blocklistState !== Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
    var Ci = Components.interfaces;
    status += " (";
    switch (plugin.blocklistState) {
      case Ci.nsIBlocklistService.STATE_SOFTBLOCKED:
        status += "Soft-blocked";
        break;
      case Ci.nsIBlocklistService.STATE_BLOCKED:
        status += "Blocked";
        break;
      case Ci.nsIBlocklistService.STATE_OUTDATED:
        status += "Outdated";
        break;
      case Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE:
        status += "Vulnerable; Update available";
        break;
      case Ci.nsIBlocklistService.STATE_VULNERABLE_NO_UPDATE:
        status += "Vulnerable; No update available";
        break;
      case Ci.nsIBlocklistService.STATE_NOT_BLOCKED:
      default: // shouldn't happen
        status += plugin.blocklistState;
        break;
      }
      status += ")";
    }

Unfortunately, since we are adding new strings to this web page, they might need to be localized for different languages. But we can implement that after we get the featuring working in one language. :)

Comment 7

[Benjamin Smedberg]

"State: ". And I wouldn't localize it, I would just include the exact strings "STATE_SOFTBLOCKED" etc.

Comment 8

[Chris Peterson [:cpeterson]]

hskupin: will the MozMill tests need to be updated if about:plugins includes both enabled and disabled plugins and a new "State:" field?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to Chris Peterson (:cpeterson) from comment #8)
> hskupin: will the MozMill tests need to be updated if about:plugins includes
> both enabled and disabled plugins and a new "State:" field?

Yes, we will have to update the test. That's why I put it into the whiteboard field. Once a patch is ready and we have a tryserver build we can easily prepare the test and check it in for the next nightly after this patch's landing.

Comment 10

[Sunny [:darkowlzz]]

Attachment: New field 'State' added which shows enabled/disabled/blocklisted state of plugins. This file depends on the changes in plugins.properties.

Comment 11

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins.

Comment 12

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 704252 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Looks good to me apart from the whitespace.

::: toolkit/content/plugins.html
@@ +176,5 @@
> +          status += ")";
> +        }
> +        stateDd.appendChild(document.createTextNode(status));
> +        dl.appendChild(stateDd);
> +        

You should remove the trailing whitespace here.

Comment 14

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 704257 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Looks good to me, thanks.

Comment 16

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Comment 17

[Chris Peterson [:cpeterson]]

Comment on attachment 704339 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Looking good! Here are a couple suggestions:

::: toolkit/content/plugins.html
@@ +13,5 @@
>  
>    var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);
>    var pluginsbundle = strBundleService.createBundle("chrome://global/locale/plugins.properties");
>    var regionbundle = strBundleService.createBundle("chrome://global-region/locale/region.properties");
> +  var Ci = Components.interfaces;

Since you are introducing a new Ci variable for Components.interfaces, can you move `var Ci` above `var strBundleService` and replace `Components.interfaces.nsIStringBundleService` with `Ci.nsIStringBundleService`?

@@ +153,5 @@
> +        if (plugin.blocklistState !== Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
> +          status += " (";
> +          switch (plugin.blocklistState)  {
> +            case Ci.nsIBlocklistService.STATE_SOFTBLOCKED:
> +              status += "Soft-blocked";

In Comment 7, bsmedberg recommended using the STATE constants' names as the string. So STATE_SOFTBLOCKED would return the string "STATE_SOFTBLOCKED", etc. That way we don't need to localize the string to different languages and developers can search the code for that constant name.

After further reflection, a dictionary mapping STATE constants to their string names would be more "JavaScripty" than a big switch statement. So you could replace the `if blocklistState !== STATE_NOT_BLOCKED` check and switch code with something like:

  var stateNames = {
    // We don't want to display STATE_NOT_BLOCKED // Ci.nsIBlocklistervice.STATE_NOT_BLOCKED:
    Ci.nsIBlocklistService.STATE_SOFTBLOCKED: "STATE_SOFTBLOCKED",
    Ci.nsIBlocklistService.STATE_BLOCKED: "STATE_BLOCKED",
    Ci.nsIBlocklistService.STATE_OUTDATED: "STATE_OUTDATED",
    Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE: "STATE_VULNERABLE_UPDATE_AVAILABLE",
    Ci.nsIBlocklistService.STATE_VULNERABLE_NO_UPDATE: "STATE_VULNERABLE_NO_UPDATE"
  };
  if (plugin.blocklistState in stateNames) {
    status += " (" + stateNames[plugin.blocklistState] + ")";
  }

Comment 18

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made appropriate changes as mentioned in the feedback

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 705446 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

::: toolkit/content/plugins.html
@@ +145,5 @@
>  
> +        // "State: "
> +        var stateDd = document.createElement("dd");
> +        var state = document.createElement("span");
> +        state.setAttribute("class", "label");

Could we give all those entries above the table specific class names? Given that our Mozmill test will parse the page to check that it shows the correct content, it will be hard for us now to get this information. I would appreciate classes like e.g. 'active'.

Comment 20

[Chris Peterson [:cpeterson]]

Comment on attachment 705446 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Sunny, I tested your patch on my Firefox and it looks good! :)

After you address Henrik's comments, you should send your patch to "bsmedberg" for his r+. I'm not a module owner for this code, so an r+ from me is not official. <:)

Comment 21

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made changes as per the previous comments

Comment 22

[Benjamin Smedberg]

Comment on attachment 705855 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

The "stateNames" map won't work, because the keys are "Ci.nsIBlocklistService.STATE_SOFTBLOCKED" strings and not the integers they represent.

I think you should construct this map at the top of the file kinda like this:

var stateNames = {};
["STATE_SOFTBLOCKED",
 "STATE_BLOCKED",
 "STATE_OUTDATED",
 "STATE_VULNERABLE_UPDATE_AVAILABLE",
 "STATE_VULNERABLE_NO_UPDATE"].forEach(function(label) {
  stateNames[Ci.nsIBlocklistService[label]] = label;
});

Comment 23

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made suggested changes

Comment 24

[Benjamin Smedberg]

Comment on attachment 705949 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

I really think you should move `stateNames` out of the "aPlugins" loop, since it's constant and doesn't change for each plugin. r=me with that change

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 705949 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

::: toolkit/content/plugins.html
@@ +145,5 @@
>  
> +        // "State: "
> +        var stateDd = document.createElement("dd");
> +        var state = document.createElement("span");
> +        state.setAttribute("class", "enabled");

Shouldn't it be of class "label enabled" then? Or shall we better name it "label state"?  Reading it now the enabled would give a wrong assumption about that field.

Comment 26

[Sunny [:darkowlzz]]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 705949 [details] [diff] [review]
> Added 'state_label' property to plugins and a new 'State' field in
> about:plugins which shows enabled/disabled/blocklisted state of plugins
> 
> Review of attachment 705949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/plugins.html
> @@ +145,5 @@
> >  
> > +        // "State: "
> > +        var stateDd = document.createElement("dd");
> > +        var state = document.createElement("span");
> > +        state.setAttribute("class", "enabled");
> 
> Shouldn't it be of class "label enabled" then? Or shall we better name it
> "label state"?  Reading it now the enabled would give a wrong assumption
> about that field.

What is the right thing to do now?

Comment 27

[Benjamin Smedberg]

"state" is probably better than "enabled", but I don't care very strongly.

Comment 28

[Sunny [:darkowlzz]]

Attachment: Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made suggested changes

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/698a863a2771

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/698a863a2771

Comment 31

[Chris Peterson [:cpeterson]]

Thanks for your help, Sunny! :D

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Removing my name from in-qa-testsuite flag for a better query.