Per the description: any arbitrary XUL document can hide the window's titlebar without any security checks. Needless to say, this is very disorienting for a user, and therefore could trick the user into thinking the page is trusted. The testcase is very simple; it just sets the hidechrome attribute onload. I assume it wasn't intentional that this is accessible to content...

And by "All", I'm sure Reed meant this bug applies to both trunk and branch on Windows and Linux but not on Mac. (Tested 2.0.0.6 on all three platforms.)

Yeah; it doesn't apply to Mac because HideWindowChrome isn't implemented on Mac.

I don't think we can trust the caller principal here; this isn't getting called by content directly. bz, any comments?

In the testcase it's being called directly, actually. But maybe it would be simpler to only do HideChrome handling if our NodePrincipal() has UniversalXPConnect or something? Alternately, perhaps only the hidechrome attribute on the root document in the window should apply? That is, XUL documents loaded in the content area, even if they're system, should not affect the window chrome. That would make the most sense to me.

(In reply to comment #5) > In the testcase it's being called directly, actually. Well, yes, but there are other ways to hit this code... it's not reliable. I agree that it makes sense to check whether the document is the root document in the window.

So I think this is what is wanted, right? As a sidenote, this kind of check is on initial load here: http://lxr.mozilla.org/seamonkey/source/layout/generic/nsContainerFrame.cpp#436

Here are some testcase I made. They need to be run from chrome.

Comment on attachment 276977 [details] [diff] [review] patch? This looks correct; I'm not sure if I should be r+ing patches in content, though.

Comment on attachment 276977 [details] [diff] [review] patch? Ok, thanks, I'll ask roc then.

Comment on attachment 276977 [details] [diff] [review] patch? I'm not a content peer, actually, either.

Comment on attachment 276977 [details] [diff] [review] patch? r=jst

Comment on attachment 276977 [details] [diff] [review] patch? I am trying to make some mochitests for this bug, but it's bit hard. I don't think there is a direct way of knowing whether hidechrome succeeded or not, is there? Otherwise, I guess I need to check with window.innerWidth/outerWidth comparing. But I'm seeing some oddness (bug 392509) which might make it kind of difficult/unsatisfactory.

Comment on attachment 276977 [details] [diff] [review] patch? So out of curiousity.... how does this affect non-XUL-UI Gecko browsers (e.g. Camino)? There every non-frame document has no parent document... I have to admit to not being able to follow the widget code well enough to tell what happens there, and I would think we want to avoid doing the hiding in those cases. That is, it might be better to check for a chrome-type docshell in addition to checking that we're the root document. See nsPresContext::IsChrome().

Ok, like this, right?

Comment on attachment 277405 [details] [diff] [review] patchv2 Yeah, that looks great!

Comment on attachment 277405 [details] [diff] [review] patchv2 a1.9=dbaron

Checking in nsXULElement.cpp; /cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v <-- nsXULElement.c pp new revision: 1.727; previous revision: 1.726 done Checked in.

I'm pretty sure nsPresContext::IsChrome is trunk-only...

As in, you'd just need to copy that code on branch.

Comment on attachment 277504 [details] [diff] [review] branch patch Ah, yeah, I still needed to test the patch. But I was in the middle of building something else.

Ok, this works, I tested it on a 1.8.1 tree. I'm just asking review, to be completely sure. Btw, fwiw, the non-dynamic testcase doesn't seem to work at all on branch, for some reason.

Comment on attachment 277531 [details] [diff] [review] branch patch Sure.

Comment on attachment 277531 [details] [diff] [review] branch patch approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers

Checking in content/xul/content/src/nsXULElement.cpp; /cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v <-- nsXULElement.c pp new revision: 1.578.2.1.2.12; previous revision: 1.578.2.1.2.11 done Checking in layout/base/nsPresContext.h; /cvsroot/mozilla/layout/base/nsPresContext.h,v <-- nsPresContext.h new revision: 3.150.4.2.4.1; previous revision: 3.150.4.2 done Checking in layout/base/nsPresShell.cpp; /cvsroot/mozilla/layout/base/nsPresShell.cpp,v <-- nsPresShell.cpp new revision: 3.852.2.11.2.9; previous revision: 3.852.2.11.2.8 done Fixed on 1.8.0.x branch. Checking in content/xul/content/src/nsXULElement.cpp; /cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v <-- nsXULElement.c pp new revision: 1.578.2.25; previous revision: 1.578.2.24 done Checking in layout/base/nsPresContext.h; /cvsroot/mozilla/layout/base/nsPresContext.h,v <-- nsPresContext.h new revision: 3.150.4.3; previous revision: 3.150.4.2 done Checking in layout/base/nsPresShell.cpp; /cvsroot/mozilla/layout/base/nsPresShell.cpp,v <-- nsPresShell.cpp new revision: 3.852.2.20; previous revision: 3.852.2.19 done Fixed on 1.8.1 branch.

verified fixed 1.8.1.7 using the testcase and: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/2007090308 BonEcho/2.0.0.7pre and: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070903 BonEcho/2.0.0.7pre ID:2007090304 Titlebar is not hidden in this 1.8.1 builds -> adding verified keyword

This looks like the likely cause of Bug 400744.

(In reply to comment #28) > This looks like the likely cause of Bug 400744. That seems unlikely to me. What makes you think that?

Let me retract the previous comment. I thought that the demo URL for 400744 uses XUL, which led me to believe that 391043 was the cause, but on second thought I need to do more investigation before pointing the finger at 391043.

Verified FIXED on 1.8.0.x, using: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre The testcase https://bugzilla.mozilla.org/attachment.cgi?id=275391 doesn't hide my titlebar at all using this build. Replacing fixed1.8.0.14 keyword with verified1.8.0.14