117707 - Disabling "Change status bar text" and/or "Cookie" prefs for JS breaks applications

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P1)

Description

[brightice]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; HP-UX 9000/785; en-US; rv:0.9.7) Gecko/20011227
BuildID:    2001122723

When having disabled "Change status bar text" for JS, the submit button on
http://wwwib.gad.de/gwmpb/ob/hbxg5735.htm will not work. Enabling the feature
does help.

Reproducible: Always
Steps to Reproduce:
1. Disable "Change status bar text" for JS
2. Load page, enter some values in the form fields
3. Try to submit, nothing happens

Actual Results:  nothing

Expected Results:  submit form data

Comment 1

[Phil Schwartau]

> Steps to Reproduce:
> 1. Disable "Change status bar text" for JS

[e-mail]: how are you disabling this? I can't see this
in Edit | Preferences anywhere -

Comment 2

[Phil Schwartau]

[e-mail]: do you mean View | Show/Hide | Status Bar ?
If I do this and hide the status bar, I do not see the bug -

Comment 3

[Martin Poirier]

It's under preferences|advanced|scripts&windows

you have to get a recent build to get this feature (was checked in on december 
30), see bug 75731

Comment 4

[Phil Schwartau]

Thanks, that's it!!! Must be a build from after December 30, and then:
Edit|Preferences|Advanced|Scripts&Windows|disable "Change status bar text"


However, I'm not sure I can see the bug. When I bring the page up, 
I see only two textboxes, for a login:

  Bitte geben Sie Ihre Kontonummer sowie die zugehörige PIN ein.
  Kontonummer: ___________
  PIN:         ___________


I can only enter random characters here, since I don't have an account.
When I click the submit button, I get a warning that my login is invalid.
This functions the same whether or not I have the status text disabled.

Reassigning to Browser-General until we can get further information.
This is not likely to be a bug in JS Engine.

[e-mail]:

1. Is there a test ID and password we can use at this site? 
2. Is the bug on the login page, or after the login page? 
3. Do you see any errors in Tasks > Tools > JavaScript Console?
   (be sure to clear it out of any previous errors first)

Comment 5

[Asa Dotzler [:asa]]

->doron.

Comment 6

[Martin Poirier]

BTW, in my comment #3 I meant bug 75371, silly me !
sorry for the spam, but i think clarifications were needed.

Comment 7

[brightice]

Phil,

if I do this ( disabling status bar changes for JavaScript, load the page, enter
account data, press button "Weiter" ) _nothing_ happens, while everything is
fine when the JavaScript is allowed to change the status bar.

The browser does not load anything, there is no progress bar, nothing.

1. There is no test ID you can use.
2. It's on the login page. (see my description above)
3. The only error I get is "Error: uncaught exception: Permission denied to set
property Window.status" as soon as I move the mouse pointer into the browser
window. But pressing the Weiter" button does not generate any message in the JS
console.

When it works, the status bar text is continously changed until the following
page is loaded.

Thanks,
 Matthias

Comment 8

[Doron Rosenberg (IBM)]

cc: mstoltz

Seems that the uncaught exception error is causing problems. Seems to be a more
general security capability issue, as this still happens if I just add the line
user.js rather than using my panel

Comment 9

[Fabian Guisset]

The problem is indeed that the site is relying on window.status not throwing an
exception. I'm really amazed such coding is possible (relying on being able to
change the status text to log in a bank, ugh), but anyway... 
I think putting a warning like "Some sites relying on this functionality may not
work if you disable it" is in order.
On the other hand, should we really throw an exception if the security manager
blocked the call?
Note also bug 112612 which says window.onError doesn't catch those exceptions.
Maybe getting rid of them altogether is the good solution.

Comment 10

[Johnny Stenback (:jst)]

Is our "Change status bar text" pref done by changing the security policy for
window.status to noAccess? If so, that's just BROKEN!

That will cause any code that touches window.status to throw an exception, and
that's totally not expected, or acceptable, it'll of course cause the script to
stop execution right there, if they didn't try/catch around setting
window.status. Nobody in their right mind would try/catch around setting
window.status, that needs fixing. If we want a pref that makes window.status not
settable, lets code that up in the setter of window.status so that it doesn't
throw an exception!

Who is in charge of the backend for the "Change status bar text" pref?

Comment 11

[Fabian Guisset]

That's exactly what it does (re: setting window.status to noAccess)
Mstoltz did the backend of the noAccess prefs I think

Comment 12

[Doron Rosenberg (IBM)]

Actually, since I wrote the panel, it is probably my fault fabian.  Mstoltz
 just wrote the capabilty code which I used.

Comment 13

[Johnny Stenback (:jst)]

Ok, then add a new preference, check for that preference
("dom.disable_window_status" or somesuch) and control that with the UI we
already have, and add a check for that pref in GlobalWindowImpl::SetStatus() and
just ignore the call if that pref is set (just like
GlobalWindowImpl::Open(nsIDOMWindow **) does.

Do we have other preferences that are implemented the same way?

Comment 14

[Boris Zbarsky [:bzbarsky]]

The entire Scripts and Windows panel is a front end to the capability 
policies....

The fact is, web authors _never_ wrap anything in try/catch (because not all 
browsers support them, for one thing).  As a result, any use of security 
policies for web content will lead to bugs such as this one... I see no reason 
to stop script execution just because a call to window.resizeTo() was blocked, 
for example....

Comment 15

[Johnny Stenback (:jst)]

Exactly. As a matter of fact, if you're writing code that's supposed to work in
4x as well as other browsers, you can not even use try/catch, they cause a parse
time error in 4x.

Comment 16

[Doron Rosenberg (IBM)]

Taking. The reason I did it the easy way (security policies) was that we wanted
it badley for 0.9.7.  

Comment 17

[Boris Zbarsky [:bzbarsky]]

Would it be possible to tweak security policies to disallow access but still not 
throw an exception?  Or would this need changes at all points where DOM objects 
are being accessed via script?

Comment 18

[Johnny Stenback (:jst)]

I donn't think we have what we'd need to disallow access to properties with the
security code w/p throwing an exception. I don't see how we could fix this w/o
touching the implmentation of the setters/getters/methods in question.

Comment 19

[Fabian Guisset]

Isn't it as simple as removing the following lines (nsScriptSecurityManager.cpp)

448 JS_SetPendingException(aJSContext,
449 STRING_TO_JSVAL(JS_NewStringCopyZ(aJSContext, errorMsg.get())));
450 //-- if (aProperty) we were called from somewhere other than xpconnect, so
we need to
451 //   tell xpconnect that an exception was thrown.
452 if (aProperty)
453 {
454 nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID());
455 if (xpc)
456 {
457 nsCOMPtr<nsIXPCNativeCallContext> xpcCallContext;
458 xpc->GetCurrentNativeCallContext(getter_AddRefs(xpcCallContext));
459 if (xpcCallContext)
460 xpcCallContext->SetExceptionWasThrown(PR_TRUE);
461 }
462 }

and add a few lines to log the error in the console as a message instead? (which
is easy)

Comment 20

[Johnny Stenback (:jst)]

No. Sure, that would make the exception go away, but script execution would
still be terminated, now silently, which is even worse than throwing an exception.

Comment 21

[Doron Rosenberg (IBM)]

got this fixed, doing it now for the rest of the prefs.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Doron, I assume you're taking out all that crap you were doing because the 
prefs could not just have preftype/prefname attributes and using such 
attributes instead?

Comment 23

[Doron Rosenberg (IBM)]

give me some credit, please :)

Though I shall probably need functions since I need !prefValue for the checkboxes. 

Comment 24

[Boris Zbarsky [:bzbarsky]]

Nah. Just call the prefs "allow_status_change" and the like.  Then default them
to true in all.js and default them to true in the C++ (for cases when the pref
service returns nothing at all).

Comment 25

[Doron Rosenberg (IBM)]

Ok, I have this working, but it blocks out chrome as well.

I noticed that jst patched the dom.disable_window_open
(http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsGlobalWindow.cpp&root=/cvsroot&subdir=mozilla/dom/src/base&command=DIFF_FRAMESET&rev1=1.468&rev2=1.469)
to not break in chrome.

Jst - should I just replicate your code for checking if the caller is chrome or
not for my patch (the c++ part is at http://nexgenmedia.net/mozilla/prefs.txt)

Comment 26

[Johnny Stenback (:jst)]

bz, doron, we don't want to default these prefs in all.js, that doesn't give us
much other than unnecessary bloat. Default them in C++, it can always be
overridden in all.js if needed.

As for the chrome issue, what's interesting here is to know if chrome or content
is trying to set the status text, not if the status text is being set on a
chrome or content window. To test whether the call comes from chrome or content
you need to check if the current context is a chrome one or not, to do this, do
something like this:

  nsCOMPtr<nsIDocShell> doc_shell;

  JSContext *cx = nsnull;
  nsCOMPtr<nsIThreadJSContextStack> stack(do_GetService(sJSStackContractID));
  if (stack)
    stack->Peek(&cx);

    if (cx) {
      nsCOMPtr<nsIScriptGlobalObject> sgo;
      nsJSUtils::GetDynamicScriptGlobal(cx, getter_AddRefs(sgo));

      if (sgo) {
        sgo->GetDocShell(getter_AddRefs(doc_shell));
      }
    }
  }

  if (doc_shell)
    PRInt32 caller_type = nsIDocShellTreeItem::typeChrome;

    get caller_type from doc_shell

    if (caller_type == content) {
      check pref

      return if not allowed to change...
    }
  }

Given that this is a non-trivial amount of code you probably want to split this
into a static helper method.

Comment 27

[timeless]

jst the rule last i read was that *all* prefs be listed in all.js so people 
would know that they can twiddle them.

has the rule changed?

Comment 28

[Johnny Stenback (:jst)]

Um, well that kinda blows, I had never heard that rule before. Can we then add
the prefs but comment them out to avoid bloating the pref code's hashtables n' all?

Comment 29

[Mitchell Stoltz (not reading bugmail)]

I should probably take this, or open a new bug on this. Johnny, you're
absolutely right, we should have "content controls" that simply no-op the call
they block, rather than throwing an exception. That's the difference between
security controls and simply filtering out annoying behavior; it's because we're
using the same backend for both that we encounter this problem. We could see
about adding the ability to configure caps to skip the call in question without
throwing an exception - shouldn't be too hard.

Comment 30

[Jesse Ruderman]

It would be nice if there could be warnings (not errors) in the JS console for 
blocked calls to window.open, window.resizeTo, etc.

Comment 31

[Johnny Stenback (:jst)]

I don't think there's a need to add code/hacks to caps/XPConnect that would
enable skipping of calls, in fact, I don't think it's doable w/o introducing
oddities for these not-too-often-seen special cases.

Making "denied" attempts at making some calls in these cases throw an
information item on the JS console sounds like a good idea to mee though.

Comment 32

[Jesse Ruderman]

While many sites will degrade more gracefully once this "no-op" fix goes in, 
broadcast.yahoo.com will probably still be blank when pop-ups or window-raising 
are disabled (bug 123186).

Comment 33

[Doron Rosenberg (IBM)]

Jesse - which is why I filed a bug about adding a warning to the panel about that. 

Comment 34

[Jesse Ruderman]

See also bug 122866, "Annoying-content controls should not stop script execution".

Comment 35

[Olav Vitters]

*** Bug 126381 has been marked as a duplicate of this bug. ***

Comment 36

[Doron Rosenberg (IBM)]

->1.0, I have it working in one place, still some issues in 2 others

Comment 37

[Per Ångström]

I have found a site that is affected by this bug: <http://www.lagercrantz.se/>.
If "Change status bar text" is disallowed, the dynamic menus won't display when
clicking the menu bar, otherwise they will.

Comment 38

[Jerome Kwok]

Attachment: window.status test

Both Test 1 and Test 2 should show an alert box. If Preference -> Scripts & 
Window -> Disable Change Status Bar Text, Test 2 will fail. All codes after the
window.status statement will be ignored.

Comment 39

[Boris Zbarsky [:bzbarsky]]

*** Bug 128696 has been marked as a duplicate of this bug. ***

Comment 40

[Henrik Gemal]

the simple cookie script test that should write 1234 but only writes 134:

<script>
document.writeln("1");
var dc = document.cookie;
document.writeln("2");
</script>
<script>
document.writeln("3");
var dc;
try { dc = document.cookie; } catch (err) {};
document.writeln("4");
</script>

Comment 41

[bugs]

I think this bug keeps reloading the page again and again on this page:

http://www.freeweb.hu/linuxdoc/hogyanok/hogyan/Konfiguracio-HOGYAN/Config.html

It is very annoying with the combined effects of bug 51139 (it takes several
seconds and reloads to stop it while holding down the Esc key).

(I think this is the right place to write, but I'll file a new bug if it isn't.)

Comment 42

[Phil Schwartau]

*** Bug 129153 has been marked as a duplicate of this bug. ***

Comment 43

[Doron Rosenberg (IBM)]

-> 0.9.9

I have this partially working, hoping to get this in for 0.9.9

Comment 44

[Doron Rosenberg (IBM)]

I'm a nut, meant 1.0

Comment 45

[Doron Rosenberg (IBM)]

Ok, I have this working for the Image.src blocking pref.

My question is, what should be done first (for performance reasons) - the check
if the caller is chrome://, or to check if the pref is set to true/false?

Comment 46

[Johnny Stenback (:jst)]

Pick one, post a patch and we'll have a look.

Comment 47

[Doron Rosenberg (IBM)]

Attachment: partial patch, done to block image.src setting

The example patch works for HTMLImage.src setting

Comment 48

[Johnny Stenback (:jst)]

Comment on attachment 73673 [details] [diff] [review]
partial patch, done to block image.src setting

>+static nsresult IsCallerChrome()

Don't pass PRBool values as nsresult's, make this method return a PRBool in
stead.

>+{
>+
>+  PRBool rv = PR_FALSE;
>+  nsCOMPtr<nsIDocShell> mDocShell;
>+
>+  JSContext *cx = nsnull;
>+  nsCOMPtr<nsIThreadJSContextStack> stack(do_GetService(sJSStackContractID));
>+
>+  if (stack){

If there's no context stack, treat the caller as chrome (i.e. return PR_TRUE).

>+    stack->Peek(&cx);
>+
>+    if (cx) {

If there's no context in the stack, treat the caller as chrome.

>+
>+  PRInt32 caller_type = nsIDocShellTreeItem::typeChrome;
>+  if (mDocShell){ 

If there's no docshell (which is unlikely), treat the caller as chrome.

[...]
> NS_IMETHODIMP
> nsHTMLImageElement::SetSrc(const nsAReadableString& aSrc)
> {
>+
>+ /* If document.enable_image_src_set is set to false, block image.src setting */
>+  nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
>+  
>+  if (prefs) 
>+  {
>+    PRBool allowImageSrcSet = PR_FALSE;
>+    prefs->GetBoolPref("document.enable_image_src_set", &allowImageSrcSet);

Flip this pref around, we shouldn't require that this pref is set to enable
setting image.src, we should let users add a pref that *disable* setting
image.src.

Other than that, this looks like a reasonable approach, but this patch needs
work.

Comment 49

[Doron Rosenberg (IBM)]

Attachment: next try

Comment 50

[Doron Rosenberg (IBM)]

Attachment: next try

Comment 51

[Phil Schwartau]

*** Bug 131610 has been marked as a duplicate of this bug. ***

Comment 52

[Doron Rosenberg (IBM)]

sorry about that, those 2 are the wrong patch.

Comment 53

[Doron Rosenberg (IBM)]

Attachment: correct patch for image.src setting blocking

The correct patch this time

Comment 54

[Ben Bucksch (:BenB)]

re comment 21:
Is the status bar instance fixed now? Which others are?

FYI: I'd like Beonex Communicator to ship with most of these capabilities
disabled (esp. status bar, resize/move, raise/lower).

Comment 55

[Doron Rosenberg (IBM)]

the patch is just an example - once approved, I will repeat it for all prefs in
the panel and create a final patch.

Comment 56

[Doron Rosenberg (IBM)]

bz just noted that we might want to put the IsCallerChrome() function somewhere
central (or perhaps make a function that returns the Caller type). Any
suggestions where?

Comment 57

[Fabian Guisset]

What do you think of adding it to nsContentUtils? I am surprised there is no
"easy" way to know whether we are in chrome or not.
Also, will this have any impact on applications that change the src of images in
tight loops or timeouts, or is it lost in the noise?
Last comment about the patch: add spaces around the equal sign in the second
line of the patch.

Comment 58

[Doron Rosenberg (IBM)]

I have to do this for dom/src/base/nsGlobalWindow.cpp, and using
nsContentUtils::GetDynamicScriptGlobal crashes, so have to use here the
originally mentioned nsJSUtils.

>Also, will this have any impact on applications that change the src of images in
>tight loops or timeouts, or is it lost in the noise?

Haven't tested yet, if you can provide a testcase, I can try it

>Last comment about the patch: add spaces around the equal sign in the second
>line of the patch.

Which line? the second line is a include

Comment 59

[Fabian Guisset]

Er oops... I meant here
+static const char *sJSStackContractID="@mozilla.org/js/xpc/ContextStack;1";
The testcase should be easy... 
var img = document.createElement("img");
img.src = "http://mozilla.org/images/mozilla-banner.gif";
document.body.appendChild(img);
var start = new Date();
for (var i = 0; i < 1000; i++) {
  img.src = "http://mozillazine.org/image/new/title.gif";
}
var end = new Date();
alert("Elapsed time: " + (end-start) + "ms");
  

Comment 60

[Doron Rosenberg (IBM)]

I better hide.

with my code: 4422ms
without     : 3579ms

Comment 61

[Doron Rosenberg (IBM)]

Doing the Chrome caller check first oseems to not change anything (this is with
he pref turned off)

Comment 62

[Doron Rosenberg (IBM)]

If I have the images cached, it is 1028 vs 998.  Might be my sucky connection
that was causing the trouble

Comment 63

[Doron Rosenberg (IBM)]

If you think that this is too much of a cost, we need to backout the panel for
1.0 btw.

Comment 64

[Doron Rosenberg (IBM)]

Attachment: what I currently have, the entire panel moved to c++

What I currently have, moves the entire panel's backend to c++. Still has some
useless code in it for the capability prefs.

Comment 65

[Doron Rosenberg (IBM)]

Attachment: what I have, 2nd try

Comment 66

[Doron Rosenberg (IBM)]

Since I already caused enough spam, some more data:

Setting of window.status 1000 times:

with changes: 3719ms
without     : 3651ms

Comment 67

[Doron Rosenberg (IBM)]

http://www.nexgenmedia.net/mozilla/117707.txt has my newest patch, and
comments/nits welcome if we want to get this working in 1.0

Comment 68

[Doron Rosenberg (IBM)]

callion asked me to change the code to use 

if (!IsCallerChrome()) && prefThatDisablesThisFunction)

It seems to be faster, in window.status setting it is now
with changes: 3666ms
without     : 3651ms

So it looks like chrome caller checking is indeed cheaper than pref getting. 
New patch coming up with his nits fixed.

Comment 69

[Johnny Stenback (:jst)]

While you're at it, please change the nsGlobalWindow code so that you don't need
all that code duplicated in all those methods. How about a
CanSetProperty(<pref_name>) helper that just returns true of false? Also, please
move the IsCallerChrome() into nsContentUtils in the content library so that you
don't need to duplicate that code in the content library. Oh, and *don't* prefix
local variable names with 'm', as in 'mDocShell', it makes it look like it's a
member of a class.

Comment 70

[Doron Rosenberg (IBM)]

Attachment: next version

I am still not convinced that checking for chrome first is the correct way.
Example, window.status setting, I would hazard a guess that this would be hit
more by websites, which means it will slow it down a bit compared to checking
for the pref first.

Comment 71

[Olav Vitters]

*** Bug 134455 has been marked as a duplicate of this bug. ***

Comment 72

[Doron Rosenberg (IBM)]

Attachment: fixes some tabs

Fixed some tabs and other minor nits from caillion

Comment 73

[Brendan Eich [:brendan]]

Preapproving to work on for 1.0 -- please get r= and sr= and mail drivers for a=.

/be

Comment 74

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 77040 [details] [diff] [review]
fixes some tabs

+static NS_DEFINE_CID(kPrefServiceCID, NS_PREF_CID);

Do you really need a CID? Can't you use a Contract ID?

+//static const char *sJSStackContractID =
"@mozilla.org/js/xpc/ContextStack;1";

Why not remove this line?

+    if (prefValue)
+      rv = PR_FALSE;
+    else
+      rv = PR_TRUE;

rv = !prefValue; ?

(This is not an r=)

Comment 75

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 77040 [details] [diff] [review]
fixes some tabs

+static NS_DEFINE_CID(kPrefServiceCID, NS_PREF_CID);

Do you really need a CID? Can't you use a Contract ID?

+//static const char *sJSStackContractID =
"@mozilla.org/js/xpc/ContextStack;1";

Why not remove this line?

+    if (prefValue)
+      rv = PR_FALSE;
+    else
+      rv = PR_TRUE;

rv = !prefValue; ?

(This is not an r=)

Comment 76

[Fabian Guisset]

Comment on attachment 77040 [details] [diff] [review]
fixes some tabs

boooring *yawn*
address biesi's comments and r=fabian either way.

Comment 77

[Doron Rosenberg (IBM)]

I'm at netscape and won't have my own mashine for a few days, so won't have a
tree.  and can't fix those minor nits.

Comment 78

[Christopher Aillon (sabbatical, not receiving bugmail)]

Doron, thanks for your initial work.  Since this is a wanted fix, I'll take over
the patch from here and address any other comments and seek sr/a=.  Assigning to
self so I don't forget to do this :)

Good luck with your new position!

Comment 79

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Updated patch

Most of the patch is still doron's work.  He forgot to do the changes in all.js
though and I updated the stuff against current tip, and made some additional
cleanup and modifications to pref-scripts.js.

Removed the commented out code as per biesi, but I left everything else alone
-- CIDs are already used throughout the files so I see no reason to change
that.  If a super-reviewer wants it changed though, I will.

Comment 80

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 77577 [details] [diff] [review]
Updated patch

Bringing forth r=fabian.

Also please ignore the change to nsDocumentEncoder.cpp -- I didn't realize that
was in there still.  It's from an unrelated bug and I'll make sure to not check
that in.

Comment 81

[Christian :Biesinger (don't email me, ping me on IRC)]

you still have this in there:
+    if (prefValue)
+      rv = PR_FALSE;
+    else
+      rv = PR_TRUE;

Why? To me that looks highly unlegant. Why not use rv = !prefValue?

Comment 82

[Doron Rosenberg (IBM)]

Comment on attachment 77577 [details] [diff] [review]
Updated patch

r=doron as well

Comment 83

[Johnny Stenback (:jst)]

Comment on attachment 77577 [details] [diff] [review]
Updated patch

Some nits, some real issues:

+nsContentUtils::IsCallerChrome()
+{
+

Loose the empty line.

+  PRBool rv = PR_TRUE;
+  nsCOMPtr<nsIDocShell> docShell;
+
+  JSContext *cx = nsnull;

cx isn't used outside the if (stack) statement below, move the declaration into
that statement.

+  nsCOMPtr<nsIThreadJSContextStack> stack(do_GetService(sJSStackContractID));
+
+  if (stack){
[...]
+  }
+

The below code has two issues, callerType should be declared more locally, and
the if check should check for if (item) and not for if (docShell),
do_QueryInterface() is null safe, so no need to check for null before callin
it.

+  PRInt32 callerType = nsIDocShellTreeItem::typeChrome;
+  if (docShell){ 
+    nsCOMPtr<nsIDocShellTreeItem> item(do_QueryInterface(docShell));  
+    item->GetItemType(&callerType);
+    if (callerType != nsIDocShellTreeItem::typeChrome) 
+      rv = PR_FALSE;
+  }

I.e. the above should be more like:

+  nsCOMPtr<nsIDocShellTreeItem> item(do_QueryInterface(docShell));  
+  if (item) { 
+    PRInt32 callerType = nsIDocShellTreeItem::typeChrome;
+    item->GetItemType(&callerType);
+
+    if (callerType != nsIDocShellTreeItem::typeChrome) {
+      rv = PR_FALSE;
+    }
+  }

And in stead of using a local |rv| variable for the return value, why not just
return early and not have the variable at all? Cleaner, and doesn't use |rv|
which typically is used for an nsresult.

- In nsHTMLImageElement::SetSrc()
> {
>+  /* If caller is not chrome and document.disable_image_src_set is set to true, block image.src setting by exiting*/

Please split the comment up so that it fits in 80 columns.

[...]
>+    if (!nsContentUtils::IsCallerChrome() && disableImageSrcSet)
>+    {

Move the '{' to the end of the line above, and swap the order of the checks in
the if condition. No need to call IsCallerChrome() if disableImageSrcSet is
false.

- In IsCallerChrome() in nsGlobalWindow.cpp, same as above.

+static PRBool CanSetProperty(const nsAFlatCString &prefName)

Since the callers of this method always have a const char * that they can pass
in, and you end up extracting that pointer out of the nsAFlatCString, why not
just make this function take a const char * and don't bother wrapping them in
string classes?

+{
+  PRBool rv = PR_FALSE;
+  

No need for |rv| here either...

+  nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
+
+  if (prefs){	 
+

Loose the empty line here. And make the above line:

+  if (!prefs) {
+    return PR_FALSE;
+  }

+    PRBool prefValue = PR_TRUE; 
+    //if pref is set to true, we can't set the property
+    prefs->GetBoolPref(prefName.get(), &prefValue);
+
+    if (prefValue)
+      rv = PR_FALSE;
+    else
+      rv = PR_TRUE;

... and then you simply return !prefValue here, as suggested by others as well.

 NS_IMETHODIMP
 GlobalWindowImpl::SetStatus(const nsAString& aStatus)
 {
+  /* If caller is not chrome and dom.disable_window_status_change is set to
true, block window.status setting by exiting*/

Wrap this so that it fits in 80 columns, same for all other new comments.

+
+  if (!IsCallerChrome() &&
!CanSetProperty(NS_LITERAL_CSTRING("dom.disable_window_status_change"))){

Add a space between ')' and '{', and loose the NS_LITERAL_CSTRING() wrappers
(all instances of both).

Index: modules/libpref/src/init/all.js
===================================================================
RCS file: /cvsroot/mozilla/modules/libpref/src/init/all.js,v
retrieving revision 3.363
diff -u -4 -r3.363 all.js
--- modules/libpref/src/init/all.js	1 Apr 2002 05:56:24 -0000	3.363
+++ modules/libpref/src/init/all.js	4 Apr 2002 01:35:25 -0000
@@ -348,18 +348,26 @@
 pref("capability.principal.codebase.foo.id", "http://www.netscape.com");
 pref("capability.principal.codebase.foo.granted", "UniversalFoo");
 //////////////////////////////////////////////////////////

-pref("dom.disable_open_during_load", false);
+// Scripts & Windows prefs
+pref("browser.block.target_new_window",     false);
+pref("document.disable_image_src_set",      false);
+pref("document.disable_cookie_get",	     false);
+pref("document.disable_cookie_set",	     false);
+pref("dom.disable_open_during_load",	     false);
+pref("dom.disable_window_move_resize",      false);
+pref("dom.disable_window_flip",	     false);
+pref("dom.disable_window_status_change",    false);

These pref names are not consistent, how about

+pref("browser.disable_target_new_window",   false);
+pref("dom.disable_image_src_set",	     false);
+pref("dom.disable_cookie_get", 	     false);
+pref("dom.disable_cookie_set", 	     false);

Other than that this looks good. Attach a new patch and I'll have one more
look.

Comment 84

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: New patch, incorporating jst's comments

Comment 85

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 77780 [details] [diff] [review]
New patch, incorporating jst's comments

Oops, forgot to carry forth Fabian's review.  Jst, would you have another look,
please?  :)

Comment 86

[Peter Janes]

Apologies for what may be a dumb question, but doesn't this remove the
site-specific nature of the security policy?  There may be sites that I want to
allow JavaScript cookies for, even though I've disabled them globally.

Leaning a bit into bug 122866 territory, should these prefs be

pref("capability.policy.{policyname}.dom.disable_{function}",{bool});

If this is close to getting into 1.0, though, all is certainly better than nothing.

Comment 87

[Boris Zbarsky [:bzbarsky]]

This bug is essentially about _not_ using security policies for the existing
UI.  Since the site-specific stuff has no UI yet, this solution is OK for now.

Making a version of security policies that would not halt code execution would
be a much larger task (certainly not 1.0-fodder).

Comment 88

[Johnny Stenback (:jst)]

Comment on attachment 77780 [details] [diff] [review]
New patch, incorporating jst's comments

sr=jst

Comment 89

[Doron Rosenberg (IBM)]

Indeed, if and when we get security policies that don't halt code execution,
this patch will get backed out and we return to the original security policy
patch I wrote.

Comment 90

[Asa Dotzler [:asa]]

Comment on attachment 77780 [details] [diff] [review]
New patch, incorporating jst's comments

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 91

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch updated against current cvs

Fixes comment wording to be consistent, and also moves a Truncate() up in 
nsHTMLDocument::GetCookie(nsAString& aCookie)

Comment 92

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment 78879 [details] [diff] has been checked in on both branch and trunk.  Marking fixed.

Comment 93

[Christopher Aillon (sabbatical, not receiving bugmail)]

Btw, if you are upgrading to a new build you have to delete some items from your
prefs.js in order to get this to work properly.  I'm working on a release note
for this. Any of these following prefs if set will need to be deleted manually
(they are still valid prefs, so we can't blindly delete them, however they throw
exceptions and cause most sites to break).  Once you delete them, you need edit
your Scripts/Windows prefs again.  Fortunately, this is a one time only deal :)

user_pref("capability.policy.default.Window.defaultStatus", "noAccess");
user_pref("capability.policy.default.Window.focus", "noAccess");
user_pref("capability.policy.default.Window.innerHeight.set", "noAccess");
user_pref("capability.policy.default.Window.innerWidth.set", "noAccess");
user_pref("capability.policy.default.Window.moveBy", "noAccess");
user_pref("capability.policy.default.Window.moveTo", "noAccess");
user_pref("capability.policy.default.Window.outerHeight.set", "noAccess");
user_pref("capability.policy.default.Window.outerWidth.set", "noAccess");
user_pref("capability.policy.default.Window.resizeBy", "noAccess");
user_pref("capability.policy.default.Window.resizeTo", "noAccess");
user_pref("capability.policy.default.Window.screenX.set", "noAccess");
user_pref("capability.policy.default.Window.screenY.set", "noAccess");
user_pref("capability.policy.default.Window.sizeToContent", "noAccess");
user_pref("capability.policy.default.Window.status", "noAccess");

Comment 94

[Doron Rosenberg (IBM)]

I suggest you create a bug to relnote the removal

Comment 95

[Vadim Berezniker]

*** Bug 137953 has been marked as a duplicate of this bug. ***

Comment 96

[Christopher Aillon (sabbatical, not receiving bugmail)]

Btw, the release note for this is being tracked on bug 137748.

Comment 97

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.

Comment 98

[Kai Lahmann (is there, where MNG is)]

*** Bug 138824 has been marked as a duplicate of this bug. ***

Comment 99

[Vaclav Dvorak]

*** Bug 122866 has been marked as a duplicate of this bug. ***

Comment 100

[Boris Zbarsky [:bzbarsky]]

*** Bug 123233 has been marked as a duplicate of this bug. ***