Closed
Bug 382367
Opened 17 years ago
Closed 17 years ago
Add support for enabling and disabling individual plugins
Categories
(Core Graveyard :: Plug-ins, enhancement, P1)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.9alpha8
People
(Reporter: mwu, Assigned: mwu)
References
Details
Attachments
(2 files, 8 obsolete files)
1.65 KB,
application/x-xpinstall
|
Details | |
26.41 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
We should allow individual plugins to be enabled or disabled, especially since uninstalling plugins to disable them is cumbersome and sometimes not possible.
Assignee | ||
Comment 1•17 years ago
|
||
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.
Attachment #266554 -
Flags: review?(jst)
Updated•17 years ago
|
Target Milestone: --- → mozilla1.9beta1
Assignee | ||
Comment 2•17 years ago
|
||
Attachment #266554 -
Attachment is obsolete: true
Attachment #266554 -
Flags: review?(jst)
Assignee | ||
Comment 3•17 years ago
|
||
Should be okay, I guess. Working on a simple frontend to test this code now.
Attachment #269139 -
Attachment is obsolete: true
Attachment #269182 -
Flags: review?(jst)
Assignee | ||
Comment 4•17 years ago
|
||
Attachment #269182 -
Attachment is obsolete: true
Attachment #269260 -
Flags: review?(jst)
Attachment #269182 -
Flags: review?(jst)
Assignee | ||
Updated•17 years ago
|
Priority: -- → P1
Assignee | ||
Comment 5•17 years ago
|
||
firefox -chrome chrome://plugintest/content/test.xul
Assignee | ||
Comment 6•17 years ago
|
||
This one doesn't crash.
Attachment #269260 -
Attachment is obsolete: true
Attachment #269309 -
Flags: review?(jst)
Attachment #269260 -
Flags: review?(jst)
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.
Attachment #269309 -
Flags: review?(jst) → review-
Assignee | ||
Comment 8•17 years ago
|
||
(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.
Assignee | ||
Comment 9•17 years ago
|
||
Blocklisting stuff ripped out, s/nsCOMPtr/nsRefPtr/, better plugin tag freeing, s/string/ACString/.
Attachment #269309 -
Attachment is obsolete: true
Attachment #269783 -
Flags: review?(jst)
Comment 10•17 years ago
|
||
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.
Attachment #269783 -
Flags: superreview-
Attachment #269783 -
Flags: review?(jst)
Attachment #269783 -
Flags: review-
Assignee | ||
Comment 11•17 years ago
|
||
String handling made more paranoid. No version attribute added for now.
Attachment #269783 -
Attachment is obsolete: true
Attachment #269918 -
Flags: superreview?(jst)
Attachment #269918 -
Flags: review?(jst)
Updated•17 years ago
|
Attachment #269918 -
Flags: superreview?(jst)
Attachment #269918 -
Flags: superreview+
Attachment #269918 -
Flags: review?(jst)
Attachment #269918 -
Flags: review+
Assignee | ||
Comment 12•17 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta1 → mozilla1.9alpha6
Assignee | ||
Comment 13•17 years ago
|
||
Backed out due to possible crashes. (bug 386160)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•17 years ago
|
||
Don't increment the index if the plugin is being skipped in GetPlugins. Should fix the crash reported in bug 386160.
Attachment #269918 -
Attachment is obsolete: true
Attachment #270222 -
Flags: superreview?(jst)
Attachment #270222 -
Flags: review?(jst)
Assignee | ||
Updated•17 years ago
|
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Comment 15•17 years ago
|
||
>> 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.
Assignee | ||
Comment 16•17 years ago
|
||
(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.
Assignee | ||
Comment 17•17 years ago
|
||
Eliminate more unrelated changes, fix IDL for getpluginfactory.
Attachment #270222 -
Attachment is obsolete: true
Attachment #270357 -
Flags: superreview?(jst)
Attachment #270357 -
Flags: review?(jst)
Attachment #270222 -
Flags: superreview?(jst)
Attachment #270222 -
Flags: review?(jst)
Updated•17 years ago
|
Attachment #270357 -
Flags: superreview?(jst)
Attachment #270357 -
Flags: superreview+
Attachment #270357 -
Flags: review?(jst)
Attachment #270357 -
Flags: review+
Assignee | ||
Comment 18•17 years ago
|
||
Checked in again.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 19•17 years ago
|
||
So this changed the handling of disabled java when checking for unwanted plugins. I assume that was on purpose?
Assignee | ||
Comment 20•17 years ago
|
||
(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•17 years ago
|
||
Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; es-ES; rv:1.9a9pre) Gecko/2007102504 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
Comment 22•17 years ago
|
||
Litmus Test Case ID: https://litmus.mozilla.org/show_test.cgi?id=4618
Flags: in-litmus+
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•