Closed
Bug 172077
Opened 22 years ago
Closed 20 years ago
Need a way for embedders to disable all plugins
Categories
(Core Graveyard :: Embedding: APIs, defect)
Core Graveyard
Embedding: APIs
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8alpha2
People
(Reporter: sfraser_bugs, Assigned: adamlock)
Details
(Keywords: fixed-aviary1.0, fixed1.7.5)
Attachments
(3 files)
2.08 KB,
patch
|
Details | Diff | Splinter Review | |
1.60 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.81 KB,
patch
|
chpe
:
review+
mscott
:
superreview+
mkaply
:
approval1.7.5+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
Can we have some action on this bug please? It seems like a serious omission.
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
Reporter | ||
Comment 3•22 years ago
|
||
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.
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?
Reporter | ||
Comment 5•22 years ago
|
||
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.
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.
This patch removes all the item type checking. Chrome or content will pick up its parent docshell's settings.
Reporter | ||
Comment 9•22 years ago
|
||
Note to self: need to test this.
Updated•22 years ago
|
QA Contact: depstein → ashishbhatt
Assignee | ||
Comment 10•22 years ago
|
||
Simon, have you had a chance to test this patch yet? Thanks
Attachment #103340 -
Flags: review?(sfraser)
Reporter | ||
Comment 11•21 years ago
|
||
Sorry, I'm no longer working on a project that requires this.
Attachment #103340 -
Flags: review?(sfraser) → review?(bz-vacation)
Comment 12•21 years ago
|
||
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.
Attachment #103340 -
Flags: review?(bz-vacation) → review+
Comment on attachment 151288 [details] [diff] [review] updated patch with change requested by the reviewer carrying forward bzbarsky's r+, and asking for sr
Attachment #151288 -
Flags: superreview?(mscott)
Attachment #151288 -
Flags: review+
Comment 15•20 years ago
|
||
Comment on attachment 151288 [details] [diff] [review] updated patch with change requested by the reviewer plussing since bz said this is ok :)
Attachment #151288 -
Flags: superreview?(mscott) → superreview+
Comment 16•20 years ago
|
||
Comment on attachment 151288 [details] [diff] [review] updated patch with change requested by the reviewer i suspect i may have encountered this problem.
Attachment #151288 -
Flags: approval1.7.1?
Comment 17•20 years ago
|
||
mozilla/docshell/base/nsDocShell.cpp 1.604
Comment 18•20 years ago
|
||
Comment on attachment 151288 [details] [diff] [review] updated patch with change requested by the reviewer a=mkaply
Attachment #151288 -
Flags: approval1.7.1? → approval1.7.1+
Comment 19•20 years ago
|
||
mozilla/docshell/base/nsDocShell.cpp 1.584.2.4
Target Milestone: mozilla1.5beta → mozilla1.8alpha2
Comment 20•20 years ago
|
||
Can someone please verify where this was checked in [ ] aviary [ ] trunk [ ] 1.7
Comment 21•20 years ago
|
||
checked in everywhere - why isn't it marked FIXED?
Comment 22•20 years ago
|
||
resolving as fixed per comment 21 (checked in everywhere); please reopen if more work is required.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 23•20 years ago
|
||
verified this patch exists on the aviary and 1.7 branches.
Keywords: fixed-aviary1.0,
fixed1.7.5
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•