382367 - Add support for enabling and disabling individual plugins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, enhancement, P1)

Description

[Michael Wu [:mwu]]

We should allow individual plugins to be enabled or disabled, especially since uninstalling plugins to disable them is cumbersome and sometimes not possible.

Comment 1

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v1

I'm not exactly sure how this should be done. In particular,

- Should I make a nsIDOMPlugin2?
- Should nsPluginTag be reference counted now?
- Should we shutdown the plugin that was running?

The changes in the about:plugins are just for testing. If a plugin is disabled, the name is italicized. There is a button next to each plugin name to toggle the enabled/disabled status.

This patch also adds a readonly blacklisted attribute.. should probably remove it since I'm not entirely sure how blacklisting should be implemented at this point.

Comment 2

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v2

Comment 3

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v3

Should be okay, I guess. Working on a simple frontend to test this code now.

Comment 4

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v4

Comment 5

[Michael Wu [:mwu]]

Attachment: Simple extension for testing the new plugin api

firefox -chrome chrome://plugintest/content/test.xul

Comment 6

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v5

This one doesn't crash.

Comment 7

[timeless]

Comment on attachment 269309 [details] [diff] [review]
Add plugins enable/disable support, v5

>diff --git a/modules/plugin/base/public/nsIPluginHost.idl 
>+[scriptable, uuid(2af1c32d-38dd-4f72-b0ab-24697d836e61)]
> interface nsIPluginHost : nsIFactory
>+  [noscript] void loadPlugins();

why noscript?

>+  [noscript] void getPluginFactory(in string aMimeType, out nsIPlugin aPlugin);

this should be:

[noscript] nsIPlugin getPluginFactory(in string aMimeType);

>+  [noscript] void isPluginEnabledForType(in string aMimeType);

why noscript?

>+  [noscript] void isPluginEnabledForExtension(in string aExtension, in constCharStarRef aMimeType);

why noscript?

>+  [noscript] readonly attribute unsigned long pluginCount;

why noscript?

>   [noscript] void getPlugins(in unsigned long aPluginCount, out /*array*/ nsIDOMPlugin aPluginArray);

IMO this should be [notxpcom] because it's violating the xpcom rules. *grumble*.

>diff --git a/modules/plugin/base/public/nsIPluginTag.idl b/modules/plugin/base/public/nsIPluginTag.idl
>+++ b/modules/plugin/base/public/nsIPluginTag.idl
>@@ -0,0 +1,49 @@
>+#include "nsISupports.idl"
>+
>+[scriptable, uuid(42f9c725-e13d-47d5-bf1d-f9b0a9ee5cf7)]
>+interface nsIPluginTag : nsISupports
>+{
>+  readonly attribute string  description;
>+  readonly attribute string  filename;
>+  readonly attribute string  name;

none of the above should be string, they should be AString or ACString

>+           attribute boolean disabled;
>+           attribute boolean blocklisted;

I object to this name. I'd rather "attribute boolean banned".

>diff --git a/modules/plugin/base/src/nsPluginHostImpl.cpp b/modules/plugin/base/src/nsPluginHostImpl.cpp

>+NS_IMETHODIMP
>+nsPluginTag::GetDescription(char** aDescription)
>+{
>+  *aDescription = new_str(mDescription);

new_str is not xpcom compatible. new_str uses c++ new, but xpcom requires you to use NS_Alloc/NS_strdup/...

thankfully if you take my advice and don't use 'string' you won't be able to make this mistake here.

>+nsPluginTag::SetDisabled(PRBool aDisabled)
>+  mPluginHost->UpdatePluginInfo();

I wonder if this can fail.

>+  return NS_OK;

> class nsPluginUnloadEvent : public nsRunnable {
>@@ -2706,34 +2773,34 @@ nsresult nsPluginHostImpl::ReloadPlugins
>   // shutdown plugins and kill the list if there are no running plugins
>-  nsPluginTag * prev = nsnull;
>-  nsPluginTag * next = nsnull;
>+  nsCOMPtr<nsPluginTag> prev;
>+  nsCOMPtr<nsPluginTag> next;

Um, plugintags have a very interesting leaky/crashy history any changes to this by someone who hasn't indicated reading this history scares me immensely.

and this isn't legal, you need to use nsRefPtr not nsCOMPtr.

> 
>-  for(nsPluginTag * p = mPlugins; p != nsnull;) {
>+  for(nsCOMPtr<nsPluginTag> p = mPlugins; p != nsnull;) {

>     next = p->mNext;
>     if(!IsRunningPlugin(p) && (!p->mEntryPoint || p->HasFlag(NS_PLUGIN_FLAG_OLDSCHOOL))) {
>       if(p == mPlugins)
>         mPlugins = next;
>       else
>         prev->mNext = next;
> 
>-      delete p;
>+      p->mNext = nsnull;
>       p = next;
>       continue;
>     }
> 
>     prev = p;
>     p = next;
>   }

>@@ -3209,33 +3276,33 @@ NS_IMETHODIMP nsPluginHostImpl::Destroy(
>   while (nsnull != mPlugins)
>   {
>+    nsCOMPtr<nsPluginTag> temp = mPlugins->mNext;

>   // Delete any remaining cached plugins list
>   while (mCachedPlugins)
>   {
>-    nsPluginTag *next = mCachedPlugins->mNext;
>-    delete mCachedPlugins;
>+    nsCOMPtr<nsPluginTag> next = mCachedPlugins->mNext;
>+    mCachedPlugins->mNext = nsnull;
>     mCachedPlugins = next;
>   }

This seems pointless. What's wrong with mCachedPlugins = nsnull; instead of this complicated while loop?

>   // Lets remove any of the temporary files that we created.

(not your fault, but i don't really care, it's buggy me) Let's

>@@ -4361,16 +4428,46 @@ nsPluginHostImpl::GetPlugins(PRUint32 aP
>+////////////////////////////////////////////////////////////////////////
>+NS_IMETHODIMP
>+nsPluginHostImpl::GetPluginTags(PRUint32* aPluginCount, nsIPluginTag*** aResults)
>+{
>+  LoadPlugins();
>+
>+  PRUint32 count = 0;
>+  nsCOMPtr<nsPluginTag> plugin = mPlugins;

could someone please remind me why plugins doesn't use an array (specifically nsCOMArray<>)?

>+  while (plugin != nsnull) {

don't explicitly compare against nsnull.

>+  *aResults = NS_STATIC_CAST(nsIPluginTag**,
>+                             nsMemory::Alloc(count * sizeof(**aResults)));
>+  if (!*aResults)
set *aPluginCount = 0 before returning here.
>+    return NS_ERROR_OUT_OF_MEMORY;

>@@ -4953,34 +5041,36 @@ #endif
>         // if this is a duplicate plugin, too place it back in the cache info list marking unwantedness

*grumble*. but OED allows it

>@@ -5369,16 +5460,26 @@ nsPluginHostImpl::LoadXPCOMPlugins(nsICo
> nsresult
>+nsPluginHostImpl::UpdatePluginInfo()
>+{
you are squishing rv for each of these, why?
>+  ReadPluginInfo();
>+  WritePluginInfo();
>+  ClearCachedPluginInfoList();
>+
>+  return NS_OK;

clearly some of them can fail!
>+nsresult
> nsPluginHostImpl::WritePluginInfo()
...
>   if (NS_FAILED(rv))
>     return rv;
>@@ -5629,17 +5730,17 @@ nsPluginHostImpl::ReadPluginInfo()
>       return rv;
>+void
>+nsPluginHostImpl::RemoveCachedPluginsInfo(const char *filename, nsPluginTag **result)
> {
>-  nsPluginTag **link = &mCachedPlugins;
>-  for (nsPluginTag *tag = *link; tag; link = &tag->mNext, tag = *link)
>+  nsCOMPtr<nsPluginTag> prev;
>+  nsCOMPtr<nsPluginTag> tag = mCachedPlugins;
>+  while (tag != nsnull)
>   {
>     if (!PL_strcmp(tag->mFileName, filename) ||
>         (tag->mFullPath && !PL_strcmp(tag->mFullPath, filename)))
>     {
>       // Found it. Remove it from our list
>+      NS_ADDREF(tag);

this isn't legal. or at least it absolutely should not be legal. did you compile this?

>diff --git a/modules/plugin/base/src/nsPluginHostImpl.h 

>+#define NS_PLUGIN_FLAG_BLOCKLISTED     0x0016    // plugin was blocklisted

was? you don't mean is? after all i just read code where you let javascript randomly change this bit.

>  * A linked-list of plugin information that is used for

*grumble*
>+  nsCOMPtr<nsPluginTag>   mNext;

*grumble* I'm grumbling here again even though I didn't grumble each time. nsRefPtr.

Comment 8

[Michael Wu [:mwu]]

(In reply to comment #7)
> (From update of attachment 269309 [details] [diff] [review])
> >diff --git a/modules/plugin/base/public/nsIPluginHost.idl 
> >+[scriptable, uuid(2af1c32d-38dd-4f72-b0ab-24697d836e61)]
> > interface nsIPluginHost : nsIFactory
> >+  [noscript] void loadPlugins();
> 
> why noscript?
> 
Because it wasn't scriptable before.

> >+  [noscript] void getPluginFactory(in string aMimeType, out nsIPlugin aPlugin);
> 
> this should be:
> 
> [noscript] nsIPlugin getPluginFactory(in string aMimeType);
> 
I would like to minimize unrelated changes in this bug.

> >+  [noscript] void isPluginEnabledForType(in string aMimeType);
> 
> why noscript?
> 
Because it wasn't scriptable before.

> >+  [noscript] void isPluginEnabledForExtension(in string aExtension, in constCharStarRef aMimeType);
> 
> why noscript?
> 
Because it wasn't scriptable before.

> >+  [noscript] readonly attribute unsigned long pluginCount;
> 
> why noscript?
> 
Because it wasn't scriptable before.

> >   [noscript] void getPlugins(in unsigned long aPluginCount, out /*array*/ nsIDOMPlugin aPluginArray);
> 
> IMO this should be [notxpcom] because it's violating the xpcom rules.
> *grumble*.
> 
Good to know.

> >diff --git a/modules/plugin/base/public/nsIPluginTag.idl b/modules/plugin/base/public/nsIPluginTag.idl
> >+++ b/modules/plugin/base/public/nsIPluginTag.idl
> >@@ -0,0 +1,49 @@
> >+#include "nsISupports.idl"
> >+
> >+[scriptable, uuid(42f9c725-e13d-47d5-bf1d-f9b0a9ee5cf7)]
> >+interface nsIPluginTag : nsISupports
> >+{
> >+  readonly attribute string  description;
> >+  readonly attribute string  filename;
> >+  readonly attribute string  name;
> 
> none of the above should be string, they should be AString or ACString
> 
Sure.

> >+           attribute boolean disabled;
> >+           attribute boolean blocklisted;
> 
> I object to this name. I'd rather "attribute boolean banned".
> 
blocklist is used elsewhere.. why change it now?

> >diff --git a/modules/plugin/base/src/nsPluginHostImpl.cpp b/modules/plugin/base/src/nsPluginHostImpl.cpp
> 
> >+NS_IMETHODIMP
> >+nsPluginTag::GetDescription(char** aDescription)
> >+{
> >+  *aDescription = new_str(mDescription);
> 
> new_str is not xpcom compatible. new_str uses c++ new, but xpcom requires you
> to use NS_Alloc/NS_strdup/...
> 
> thankfully if you take my advice and don't use 'string' you won't be able to
> make this mistake here.
> 
OK.

> >+nsPluginTag::SetDisabled(PRBool aDisabled)
> >+  mPluginHost->UpdatePluginInfo();
> 
> I wonder if this can fail.
> 
No users ever seem to care.

> >+  return NS_OK;
> 
> > class nsPluginUnloadEvent : public nsRunnable {
> >@@ -2706,34 +2773,34 @@ nsresult nsPluginHostImpl::ReloadPlugins
> >   // shutdown plugins and kill the list if there are no running plugins
> >-  nsPluginTag * prev = nsnull;
> >-  nsPluginTag * next = nsnull;
> >+  nsCOMPtr<nsPluginTag> prev;
> >+  nsCOMPtr<nsPluginTag> next;
> 
> Um, plugintags have a very interesting leaky/crashy history any changes to this
> by someone who hasn't indicated reading this history scares me immensely.
> 
Thanks, but this comment is not helpful for me.

> and this isn't legal, you need to use nsRefPtr not nsCOMPtr.
> 
Why is that?

> This seems pointless. What's wrong with mCachedPlugins = nsnull; instead of
> this complicated while loop?
> 
Sure. (now that nsPluginTag is refcounted..)

> >+  while (plugin != nsnull) {
> 
> don't explicitly compare against nsnull.
> 
It's done elsewhere in the file - just keeping things consistent.

> >+  *aResults = NS_STATIC_CAST(nsIPluginTag**,
> >+                             nsMemory::Alloc(count * sizeof(**aResults)));
> >+  if (!*aResults)
> set *aPluginCount = 0 before returning here.
OK.

> >@@ -5369,16 +5460,26 @@ nsPluginHostImpl::LoadXPCOMPlugins(nsICo
> > nsresult
> >+nsPluginHostImpl::UpdatePluginInfo()
> >+{
> you are squishing rv for each of these, why?
No users of these functions have ever checked the return values.

> >+  ReadPluginInfo();
> >+  WritePluginInfo();
> >+  ClearCachedPluginInfoList();
> >+
> >+  return NS_OK;
> 
> clearly some of them can fail!
And no existing users care. (still useful to ensure we can read the plugins first before writing to ensure we don't lose track of what plugins are unwanted)

> >+nsresult
> > nsPluginHostImpl::WritePluginInfo()
> ...
> >   if (NS_FAILED(rv))
> >     return rv;
> >@@ -5629,17 +5730,17 @@ nsPluginHostImpl::ReadPluginInfo()
> >       return rv;
> >+void
> >+nsPluginHostImpl::RemoveCachedPluginsInfo(const char *filename, nsPluginTag **result)
> > {
> >-  nsPluginTag **link = &mCachedPlugins;
> >-  for (nsPluginTag *tag = *link; tag; link = &tag->mNext, tag = *link)
> >+  nsCOMPtr<nsPluginTag> prev;
> >+  nsCOMPtr<nsPluginTag> tag = mCachedPlugins;
> >+  while (tag != nsnull)
> >   {
> >     if (!PL_strcmp(tag->mFileName, filename) ||
> >         (tag->mFullPath && !PL_strcmp(tag->mFullPath, filename)))
> >     {
> >       // Found it. Remove it from our list
> >+      NS_ADDREF(tag);
> 
> this isn't legal. or at least it absolutely should not be legal. did you
> compile this?
As a matter of fact, I did. Hell, it even runs.

> 
> >diff --git a/modules/plugin/base/src/nsPluginHostImpl.h 
> 
> >+#define NS_PLUGIN_FLAG_BLOCKLISTED     0x0016    // plugin was blocklisted
> 
> was? you don't mean is? after all i just read code where you let javascript
> randomly change this bit.
> 
This is somewhat intended, but I'm about to rip out the blocklisting stuff anyway since that isn't the point of this bug.

Comment 9

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v6

Blocklisting stuff ripped out, s/nsCOMPtr/nsRefPtr/, better plugin tag freeing, s/string/ACString/.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 269783 [details] [diff] [review]
Add plugins enable/disable support, v6

- In nsIPluginTag.idl:

+interface nsIPluginTag : nsISupports
+{
+  readonly attribute ACString description;
+  readonly attribute ACString filename;
+  readonly attribute ACString name;
+           attribute boolean  disabled;
+};

Would it make sense to add a version attribute there too, even if we don't have that information readily available yet? Some day I suspect we will, so we could avoid having to change this interface if we add that already and simply return empty strings instead for now. Either way is fine with me.

- In nsPluginTag::GetDescription(nsACString& aDescription):

aDescription.Assign(nsDependentCString(mDescription));

nsDependentCString is not safe to use with a null pointer, so you should check here as I don't see anything that guarantees that mDescription etc is non-null (i.e. you'll need to do the same in all methods that do the same thing). You could either do something like:

  aDescription.Truncate();

  if (mDescription) {
    aDescription.Assign(nsDependentCString(mDescritption));
  }

Other than that this looks good. Fix the above string issue and I'll r+sr this.

Comment 11

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v7

String handling made more paranoid. No version attribute added for now.

Comment 12

[Michael Wu [:mwu]]

Checking in modules/plugin/base/public/Makefile.in;
/cvsroot/mozilla/modules/plugin/base/public/Makefile.in,v  <--  Makefile.in
new revision: 1.41; previous revision: 1.40
done
Checking in modules/plugin/base/public/nsIPluginHost.idl;
/cvsroot/mozilla/modules/plugin/base/public/nsIPluginHost.idl,v  <--  nsIPluginHost.idl
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/modules/plugin/base/public/nsIPluginTag.idl,v
done
Checking in modules/plugin/base/public/nsIPluginTag.idl;
/cvsroot/mozilla/modules/plugin/base/public/nsIPluginTag.idl,v  <--  nsIPluginTag.idl
initial revision: 1.1
done
Checking in modules/plugin/base/src/nsPluginHostImpl.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <--  nsPluginHostImpl.cpp
new revision: 1.576; previous revision: 1.575
done
Checking in modules/plugin/base/src/nsPluginHostImpl.h;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.h,v  <--  nsPluginHostImpl.h
new revision: 1.104; previous revision: 1.103
done

Comment 13

[Michael Wu [:mwu]]

Backed out due to possible crashes. (bug 386160)

Comment 14

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v8

Don't increment the index if the plugin is being skipped in GetPlugins. Should fix the crash reported in bug 386160.

Comment 15

[timeless]

>> this should be:
>> [noscript] nsIPlugin getPluginFactory(in string aMimeType);
> I would like to minimize unrelated changes in this bug.

doing this doesn't change the C++ side, it's just an idl correctness change.

> Because it wasn't scriptable before.

not a great reason, but oh well.

>> I wonder if this can fail.
> No users ever seem to care.

code changes should improve correctness. buggy callers often chose not to care because the callee wasn't giving any useful failure indication. changes should improve code and enable callees to care.

> Thanks, but this comment is not helpful for me.

please see Bug 265936. Ideally whatever you do here will resolve that bug.

>> and this isn't legal, you need to use nsRefPtr not nsCOMPtr.
> Why is that?

nsCOMPtrs can only legally hold interfaces, you gave it a class. nsRefPtr exists to hold classes that have AddRef/Release.

> It's done elsewhere in the file - just keeping things consistent.

ok.

> > you are squishing rv for each of these, why?
> No users of these functions have ever checked the return values.

not a great reason. plugin code has been historically buggy and crashy and generally old. ideally changing it should involve making it less buggy. note that you own blame for lines you change. so the next time it crashes near these places, it's your fault :).

> And no existing users care. (still useful to ensure we can read the plugins
> first before writing to ensure we don't lose track of what plugins are
> unwanted)

right, so clearly users (including yourself) should care :).

> This is somewhat intended, but I'm about to rip out the blocklisting stuff
> anyway since that isn't the point of this bug.

ok, I was merely complaining about _WAS_ v. _IS_. But indeed, bugs should be limited in scope, and unrelated features which can each have their own regressions should indeed get distinct landings.

Comment 16

[Michael Wu [:mwu]]

(In reply to comment #15)
> >> this should be:
> >> [noscript] nsIPlugin getPluginFactory(in string aMimeType);
> > I would like to minimize unrelated changes in this bug.
> 
> doing this doesn't change the C++ side, it's just an idl correctness change.
> 
Yeah, looks like it. Will do, thanks.

> > Thanks, but this comment is not helpful for me.
> 
> please see Bug 265936. Ideally whatever you do here will resolve that bug.
> 
Well, nsPluginTag is now refcounted, so there should be no problems.

> > And no existing users care. (still useful to ensure we can read the plugins
> > first before writing to ensure we don't lose track of what plugins are
> > unwanted)
> 
> right, so clearly users (including yourself) should care :).
> 
It's still a bit better to proceed even if we couldn't read the old plugin info.. losing track of what plugins are unwanted isn't as bad as not saving the enabled/disabled status of plugins.

Comment 17

[Michael Wu [:mwu]]

Attachment: Add plugins enable/disable support, v9

Eliminate more unrelated changes, fix IDL for getpluginfactory.

Comment 18

[Michael Wu [:mwu]]

Checked in again.

Comment 19

[Boris Zbarsky [:bzbarsky]]

So this changed the handling of disabled java when checking for unwanted plugins.  I assume that was on purpose?

Comment 20

[Michael Wu [:mwu]]

(In reply to comment #19)
> So this changed the handling of disabled java when checking for unwanted
> plugins.  I assume that was on purpose?
> 
Yup, because disabled doesn't also mean unwanted now.

Comment 21

[Tony Chung [:tchung]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a9pre) Gecko/2007102504 Minefield/3.0a9pre

Comment 22

[Tony Chung [:tchung]]

Litmus Test Case ID: https://litmus.mozilla.org/show_test.cgi?id=4618