Closed Bug 342333 Opened 18 years ago Closed 7 years ago

Safe mode should disable plug-ins

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: doronr, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(3 obsolete files)

Plug-ins should not be loaded when we are in safe mode so that safe mode is clear of any external evils :)
Attached patch Patch rev. 1 (obsolete) — Splinter Review
This works, but I would like to avoid introducing the "browser.safe_mode"
pref if possible.  I tried various other methods of propagating the value
of 'gSafeMode' to nsPluginHostImpl but failed...
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/xre/nsAppRunner.cpp&rev=1.156&root=/cvsroot&mark=493#493

Suggestions for something better than a pref would be appreciated...
We should hoist the nsIXULRuntime/nsIXULAppInfo interfaces to be available to gecko and necko.
you could add a readonly attribute boolean pluginsEnabled to nsIPluginHost.
(In reply to comment #2)
> We should hoist the nsIXULRuntime/nsIXULAppInfo interfaces to be
> available to gecko and necko.

I agree.  Is this just a Makefile change or do we need to copy the
CVS files to a new directory and remove branch tags or could I just
do a "cvs add/rm"?  What needs to be done?


(In reply to comment #3)
> you could add a readonly attribute boolean pluginsEnabled to nsIPluginHost.

The only consumer at this point would be browser.js, but nsIPluginHost isn't
scriptable.  Also, at the moment the patch is fairly low risk and does not
change any interfaces so it could potentially be considered for branches
since it would help us triage crashes better.  (But I see your point of
decoupling pluginsEnabled from safe mode.)
Attached patch Patch rev. 2 (obsolete) — Splinter Review
Here's a patch that assumes nsIXULRuntime/nsIXULAppInfo is available
to modules/plugin/base/src/nsPluginHostImpl.cpp
Attachment #259680 - Attachment is obsolete: true
> I agree.  Is this just a Makefile change or do we need to copy the
> CVS files to a new directory and remove branch tags or could I just
> do a "cvs add/rm"?  What needs to be done?

We do need to have the files in another directory, and I wouldn't worry about cvs copies. Just cvs add/rm them and put them in... xpcom/system?

--BDS
Attached patch Patch rev. 3 (obsolete) — Splinter Review
Assignee: nobody → mats.palmgren
Attachment #259799 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #259917 - Flags: review?(benjamin)
Shouldn't a choice whether to enable/disable plugins be added to the safe-mode initialization dialog ?
IMO, permanently disabling all plugins is a separate feature.
Probably best implemented as a pref.
Comment on attachment 259917 [details] [diff] [review]
Patch rev. 3

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>+#include "nsIXULAppInfo.h"
>+#include "nsIXULRuntime.h"

You don't need nsIXULAppInfo at all.

>+  nsCOMPtr<nsIXULAppInfo> appInfo = 
>+    do_GetService("@mozilla.org/xre/app-info;1");
>+  nsCOMPtr<nsIXULRuntime> xulRuntime = do_QueryInterface(appInfo);

You don't need separate getservice/QI. Use xulruntime = getservice() and skip appinfo altogether.

Otherwise, this looks fine. I'd like dbaron to sr the new patch.
Attachment #259917 - Flags: review?(benjamin) → review+
(In reply to comment #10)
> You don't need separate getservice/QI. Use xulruntime = getservice() 

I actually had it like that at first but I got warnings when testing the
patch in SeaMonkey:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Illegal value' when calling method: [nsIFactory::createInstance]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "<unknown>"  data: no]
************************************************************

It seems they have an app-info service that lacks nsIXULRuntime.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/components/xulappinfo/xulappinfo.js&rev=1.5&root=/cvsroot&mark=21-45
Attachment #259917 - Flags: superreview?(dbaron)
> It seems they have an app-info service that lacks nsIXULRuntime.

Sure. In that case the getservice should fail. That's expected and ok. I'm not sure why it's reporting that particular error to the JS console, but we shouldn't be bloating this code to work around that issue.
Ok, I'll change it to "xulRuntime = do_GetService" before checkin...
Comment on attachment 259917 [details] [diff] [review]
Patch rev. 3

sr=dbaron, although I'm a little suspicious of adding InSafeMode() checks in multiple places when what you mean is !PluginsEnabled() -- if you want to later change the conditions under which plugins are enabled, you have to go hunt for multiple places to change.  That said, I'm not sure where the "are plugins enabled" thing really belongs.  Maybe the document?

At least get a bug filed on fixing that if you don't want to fix it now (although I'd prefer if you fixed it now).
Attachment #259917 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 259917 [details] [diff] [review]
Patch rev. 3

I take that back -- I think given the comment I should mark it sr-.
Attachment #259917 - Flags: superreview+ → superreview-
(In reply to comment #14)
> change the conditions under which plugins are enabled, you have to go hunt for
> multiple places to change.  That said, I'm not sure where the "are plugins

And a second reason is that it may be hard to understand why there's an "in safe mode" check if somebody doesn't know about this particular feature.  Somebody might even think it's a mistake.
(In reply to comment #15)
> (From update of attachment 259917 [details] [diff] [review])
> I take that back -- I think given the comment I should mark it sr-.

As I said in comment 4:
I think the patch is low risk and that it would really help triaging
crash bugs, so the benefit/risk ratio might be high enough for branch
and that's why I didn't add a PluginsEnabled() to some interface.
I'll be happy to add it on trunk though.
David, see my last comment above (I forgot to CC you).
OK, so the above patch is ok for the branch if you land a better patch on the trunk first.
Probably not worth messing with this on branch...

Plugins can be individually disabled on trunk through Tools->Add-ons
so it's less critical to implement this for triage purposes IMO.
It should still be fixed of course, but my patch is likely obsolete by now...
Assignee: mats.palmgren → nobody
Status: ASSIGNED → NEW
Attachment #259917 - Attachment is obsolete: true
If the patch isn't going to land the information bar in the addons manager when in safe mode should be changed to "...all extensions are disabled" being that nothing else is disabled!
FWIW, plugins are still actually loaded even when disabled, and thus still have a chance to blow things up...
I've been taking a look at this since it looks like it should be fixed in order to fix bug 462334. Based on discussions with Mossop and biesi, I'm thinking along these lines:

- The pluginsDisabled attribute could go into nsIDocument as dbaron suggested. Not sure where it should be set though (wherever the document is instantiated I guess).

- The changes to pluginGlue.js don't look right since they won't run once the component has been registered. Maybe change missingPlugin.xml instead? It should use a different label and turn off the click handlers if plugins are disabled. This should mean that the change to browser.js isn't necessary since the PluginNotFound event will never fire anyway.

- We change nsObjectLoadingContent so that it doesn't try to load the plugin. Not sure where to put the check. Biesi suggested Instantiate, which could be right since apparently it only knows it's a plugin after OnStartRequest in some cases. It doesn't seem to be a big deal to me if it does the request even if the plugin is disabled.

- If we block plugin loading in nsObjectLoadingContent, however, I am far from convinced that FindPlugins will never be called. For example, won't nsPluginHostImpl::GetPluginCount still be called in some cases? nsPluginArray calls this, for example, though I'm not clear on what that is used for (nsNavigator::GetPlugins, but what is that for?).

- We probably want to handle plugins like extensions in safe mode. This means the list should still be displayed with an info banner saying they are disabled due to safe mode. Since we don't want to risk loading plugins, how about getting the info from pluginreg.dat? Basically I would suggest changing nsPluginHostImpl to use pluginreg.dat for FindPlugins when plugins are disabled. Hopefully there is some way for it to get at the document.

- Disabling of plugins would be triggered both by safe mode and by a plugins.disabled pref.
Blocks: 462334
(In reply to comment #24)
> - The changes to pluginGlue.js don't look right since they won't run once the
> component has been registered. Maybe change missingPlugin.xml instead? It
> should use a different label and turn off the click handlers if plugins are
> disabled. This should mean that the change to browser.js isn't necessary since
> the PluginNotFound event will never fire anyway.

missingPlugin.xml doesn't have chrome privileges I'm pretty sure, so that wouldn't quite work.

> - We change nsObjectLoadingContent so that it doesn't try to load the plugin.
> Not sure where to put the check. Biesi suggested Instantiate, which could be
> right since apparently it only knows it's a plugin after OnStartRequest in some
> cases. It doesn't seem to be a big deal to me if it does the request even if
> the plugin is disabled.

Yeah that's probably OK

> - If we block plugin loading in nsObjectLoadingContent, however, I am far from
> convinced that FindPlugins will never be called. For example, won't
> nsPluginHostImpl::GetPluginCount still be called in some cases? nsPluginArray
> calls this, for example, though I'm not clear on what that is used for
> (nsNavigator::GetPlugins, but what is that for?).

Why is it a problem if FindPlugins is called?

(nsNavigator::GetPlugins is for navigator.plugins (https://developer.mozilla.org/en/navigator.plugins))

> - We probably want to handle plugins like extensions in safe mode. This means
> the list should still be displayed with an info banner saying they are disabled
> due to safe mode. Since we don't want to risk loading plugins, how about
> getting the info from pluginreg.dat? Basically I would suggest changing
> nsPluginHostImpl to use pluginreg.dat for FindPlugins when plugins are
> disabled. Hopefully there is some way for it to get at the document.

Which "the document"?

I guess your suggestion would work (although on Windows you don't need to load a plugin to find its description etc). I would only do that when in safe mode though, not in all cases when plugins are disabled.
Flags: wanted1.9.2?
Blocks: safe-mode
Component: Plug-ins → Startup and Profile System
Flags: wanted1.9.2?
Product: Core → Toolkit
QA Contact: plugins → startup
Hardware: x86 → All
Component: Startup and Profile System → Plug-ins
Product: Toolkit → Core
QA Contact: startup → plugins
Realistically we're not going to fix this now, and plugins are OOP so less worrisome.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.