172077 - Need a way for embedders to disable all plugins

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Simon Fraser [no longer active]]

We don't have a pref that turns off plugins for embedders (or in Mozilla). Nor
does it seem to be possible for an embedder to use nsIWebBrowserSetup to fully
control plugins, since they are only in control of the content-area
nsIWebBrowser, and don't get a chance to veto plugins for <iframe> contents.

Comment 1

[Simon Fraser [no longer active]]

Can we have some action on this bug please? It seems like a serious omission.

Comment 2

[Adam Lock]

What is the priority for this?

I can think of several ways it could be controlled, from a pref, to some kind of
inherited plugin behaviour, such as propogating settings such as mAllowPlugins
downwards on new creations. It sounds like there should be some general
preference anyway.

At present the docshell does nothing special with its mAllowPlugins setting. It
just exposes it via an attribute that others set and the
nsWebBrowserContentPolicy object queries to determine whether to load plugins
into a particular docshell or not.

http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp

Comment 3

[Simon Fraser [no longer active]]

Wouldn't it make sense for it to propagate that setting to subframes by default?
The embedder has not way to do that themselves, so the API is useless as it stands.

Comment 4

[Adam Lock]

Attachment: Patch

This patch tells docshells to inherit the permissions for the flags from their
parent in the call to setParent. I have put a check in to ensure the parent
docshell and the child are content before inheriting values. I don't know if
this is strictly necessary, but it seems safer.

Reviews?

Comment 5

[Simon Fraser [no longer active]]

Comment on attachment 103321 [details] [diff] [review]
Patch

+	 // Note: The item type of new docshells is determined before the
+	 //	  parent is set, so comparing the type to its parent
+	 //	  is safe.
+	 PRInt32 parentType = ~mItemType;
+	 if (NS_SUCCEEDED(mParent->GetItemType(&parentType)) &&
+	     parentType == mItemType)

This confuses me a little. Why initialize parentType to ~mItemType?
Is that to ensure that parentType != mItemType if mParent->GetItemType()
fails? Since you never do the comparison if it fails, then why bother?

Also the comments about the parent not being set indicate that mParent
can be null sometimes. Can we guarantee that it's never null?

Can 'content' docShells ever contain 'chrome' docshells? Does this
work in that case?

Other than those comments, this seems like the right direction.

Comment 6

[Adam Lock]

I do the ~mItemType thing since it is a convention adopted elsewhere. I don't
have to do it, but I thought it didn't hurt either.

The issue with the comment is this. The impl of nsDocShell::SetItemType checks
to ensure mParent is nsnull and returns NS_ERROR_UNEXPECTED if it isn't.
Therefore it is safe to compare the item types here because the item type is set
before the call to SetParent and cannot be changed afterwards. I just added a
note to indicate that.

I don't know if there is anything stopping chrome inside content, but the
embedder shouldn't be able to manage it from their APIs. For example, if you
load chrome:// URLs into a frame, they are still treated as content. I don't
believe there is anything to stop people creating chrome docshells by hand by
hitting the docshell directly - not something we publically support or encourage
embedders to do.

It would be possible that the embedder might create a chrome top-level docshell
with a content area, in which case perhaps we should allow content docshells to
inherit values chrome or content parents. If I did that I could just remove all
this type comparison code except for the outer nsIDocShellTreeItem::typeContent.
I'll do another simpler patch to demonstrate.

Comment 7

[Adam Lock]

Attachment: Simpler patch

This patch removes all the item type checking. Chrome or content will pick up
its parent docshell's settings.

Comment 8

[Adam Lock]

Simon does, the simpler patch work for you?

Comment 9

[Simon Fraser [no longer active]]

Note to self: need to test this.

Comment 10

[Adam Lock]

Simon, have you had a chance to test this patch yet? Thanks

Comment 11

[Simon Fraser [no longer active]]

Sorry, I'm no longer working on a project that requires this.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 103340 [details] [diff] [review]
Simpler patch

>+    if (mParent)

No need for this check -- do_QI is null-safe.

r=bzbarsky with that removed.

Comment 13

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch with change requested by the reviewer

Comment 14

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 151288 [details] [diff] [review]
updated patch with change requested by the reviewer

carrying forward bzbarsky's r+, and asking for sr

Comment 15

[Scott MacGregor]

Comment on attachment 151288 [details] [diff] [review]
updated patch with change requested by the reviewer

plussing since bz said this is ok :)

Comment 16

[timeless]

Comment on attachment 151288 [details] [diff] [review]
updated patch with change requested by the reviewer

i suspect i may have encountered this problem.

Comment 17

[timeless]

mozilla/docshell/base/nsDocShell.cpp 	1.604

Comment 18

[Mike Kaply [:mkaply]]

Comment on attachment 151288 [details] [diff] [review]
updated patch with change requested by the reviewer

a=mkaply

Comment 19

[timeless]

mozilla/docshell/base/nsDocShell.cpp 	1.584.2.4

Comment 20

[Mike Kaply [:mkaply]]

Can someone please verify where this was checked in

[ ] aviary
[ ] trunk
[ ] 1.7

Comment 21

[Mike Kaply [:mkaply]]

checked in everywhere - why isn't it marked FIXED?

Comment 22

[Kathleen :Brade]

resolving as fixed per comment 21 (checked in everywhere); please reopen if more
work is required.

Comment 23

[Daniel Veditz [:dveditz]]

verified this patch exists on the aviary and 1.7 branches.