Daniel Veditz [:dveditz] Reporter
	Description • 18 years ago

Firefox has had support for a cool anti-phishing microbar that shows the true location of a page even on popups that disable the full-size location bar, simply by changing the pref dom.disable_open_window_feature.location to true.

When we first tried setting this before Firefox 1.0 shipped (see bug 22183 comment 120 and following) and got a lot of pushback. Now that Firefox is more established, and phishing is understood to be a serious problem by a broader range of the web audience, it may be appropriate to finally do this in Firefox-2.

	Mike Connor [:mconnor]
	Comment 1 • 18 years ago

So, this is something that is better posted to d-a-f, but I'm not opposed to it, especially as IE7 seems to be doing the same thing as we had before.

plussing so it gets discussed

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 2 • 18 years ago

I'm for this, really, despite the fact that evidence shows that most users don't really look at (or understand) the location bar, and despite the fact that location bars can be spoofed, we can probably design something that calls a little bit more attention to itself and alerts users to the fact that the window has been opened by the browser, etc.

A similar but alternative solution might be to use a browsermessage notification which says something along the lines of:

[ This is a webpage                [ Show URL ] [ Show Toolbars ]  ]

Just spitballin' ideas here. 

	Ben Goodger (use ben at mozilla dot org for email)
	Comment 3 • 18 years ago

Hey Mike, here's one to pass to the interns ;-)

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 4 • 18 years ago

(In reply to comment #2)
> A similar but alternative solution might be to use a browsermessage
> notification which says something along the lines of:
> 
> [ This is a webpage                [ Show URL ] [ Show Toolbars ]  ]

Even better:

 .---------------------------------------------------------------------.
 |Paypal: Enter your personal information                        _ [] X|
 |---------------------------------------------------------------------'
 | http://221.241.11.2/itsatrap.html                (Show Toolbars) (X)|
 |---------------------------------------------------------------------|
 |                                                                     |
 .                                                                     .
 .                                                                     .
 .                                                                     .

	Mike Connor [:mconnor]
	Comment 5 • 18 years ago

Indeed!

	Madhava Enros [:madhava]
	Comment 6 • 18 years ago

If this goes ahead, it would probably be useful to provide users with a quick way to suppress the microbar for trusted domains.  In non-phishing cases, chromeless windows are likely to occur in web-apps and windows where some care has been taken with the visual aesthetics;  in both cases, a recurring microbar would likely been seen as "in the way."  If users turn it off permanently for all sites, they're prey for the wolves again.  Maybe even piggybacking on the 'allow popup' whitelist?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 7 • 18 years ago

(In reply to comment #6)
> If this goes ahead, it would probably be useful to provide users with a quick
> way to suppress the microbar for trusted domains.  In non-phishing cases,

I think this is a very good idea, actually. Supporting web-apps that want to control their user experience is something we should be thinking about, even though I cringe at the idea of more whitelists.

> would likely been seen as "in the way."  If users turn it off permanently for
> all sites, they're prey for the wolves again.  Maybe even piggybacking on the

Whatever we do, there should be no way to turn it off for all sites. That would be allowing the user to "shoot from hip, aim at foot."

	Mike Schroepfer
	Comment 8 • 18 years ago

Even without the notification message to suppress this, we need to get it for feature parity with IE7 as a way to stomp on phishers.

	Jeff Walden [:Waldo]
	Comment 9 • 18 years ago

I don't see a simple way to do this with the current code, so for now I'm just flipping the pref.  If someone else sees a simple way to figure out the original window parameters from which a window was opened using the current code, I'd be all ears.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 10 • 18 years ago

Connor was concerned that flipping this pref will break a bunch of our code which uses "was the window opened with chrome or not" as a heuristic to determine if a window is or is not a popup (f.e. opening a link from an external app in new tab when the last-focused Firefox window was a popup window).

Maybe we can just use the notificationmessage solution. Just show:

 .---------------------------------------------------------------------.
 |Paypal: Enter your personal information                        _ [] X|
 |---------------------------------------------------------------------'
 | /!\ This is 123.123.123.123                    (Trust this site) (X)|
 |---------------------------------------------------------------------|

	Mike Connor [:mconnor]
	Comment 11 • 18 years ago

Comment on attachment 227346 [details] [diff] [review]
Flip the pref

This regresses window targeting behaviour that relies on chromehidden. I think beltzner's solution is probably better overall.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 12 • 18 years ago

Decided not to block on this, since it's new functionality. Dan has said that he'd be willing to look at this and either:

 - flip the chromehidden pref and fix the browser.js and antiphishing dependencies
 - implement the notificationmessage as shown here and add an indication of whether or not the site is using SSL

Thanks, Dan! :)

	Daniel Veditz [:dveditz] Reporter
	Comment 13 • 18 years ago

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

	Jesse Ruderman
	Comment 14 • 18 years ago

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

	Michael Ventnor
	Comment 15 • 18 years ago

I've searched LXR and haven't found any code which flipping the pref may break. Does anyone know of any examples where this is the case? I might have a go at fixing them if so.

	Mike Connor [:mconnor]
	Comment 16 • 18 years ago

it'll break the external link handling code that bypasses popups by checking chromehidden.  Beltzner's solution is preferred anyway, per comment 11

	Michael Ventnor
	Comment 17 • 18 years ago

I found the code which handles external links but saw no dependency on the location bar or chromehidden (instead it checked window.toolbar.visible). I ran a window.open javascript which had everything hidden but the location bar. Not only was the location bar read-only, but Fx had no trouble opening a new full-chrome window when I typed in a URL in the Run dialog.

Did all the problems get fixed while this bug was hibernating?

	Daniel Veditz [:dveditz] Reporter
	Comment 18 • 18 years ago

Did you try it on the Mac? WFM fine on windows, but mconnor demonstrated some problems on a Mac.

	Michael Ventnor
	Comment 19 • 18 years ago

(In reply to comment #18)
> Did you try it on the Mac? WFM fine on windows, but mconnor demonstrated some
> problems on a Mac.

I don't have a Mac, sorry. This shouldn't show problems on the Mac since the code that handles external links is JS and checks for the toolbar, not the location bar, to determine if it's a popup or not.

This missed out on 2.0 obviously so -> Future.

	Jordan M.
	Comment 20 • 18 years ago

(In reply to comment #18)
> Did you try it on the Mac? WFM fine on windows, but mconnor demonstrated some
> problems on a Mac.
> 
Dan - Are either of the solutions to this (notification, flip) going to show up in Firefox 3?

Thanks.

	Jordan M.
	Comment 21 • 18 years ago

(In reply to comment #4)
> (In reply to comment #2)
> > A similar but alternative solution might be to use a browsermessage
> > notification which says something along the lines of:
> > 
> > [ This is a webpage                [ Show URL ] [ Show Toolbars ]  ]
> 
> Even better:
> 
>  .---------------------------------------------------------------------.
>  |Paypal: Enter your personal information                        _ [] X|
>  |---------------------------------------------------------------------'
>  | http://221.241.11.2/itsatrap.html                (Show Toolbars) (X)|
>  |---------------------------------------------------------------------|
>  |                                                                     |
>  .                                                                     .
>  .                                                                     .
>  .                                                                     .
> 
Wait, we're going to have an X on the window? Isn't this going to just bother users, instead of solve the actual problem? The average user will ignore the window, regardless if it said "YOU'RE BEING FOOLED" or not, they'll X out of the window. I thought we all read the phishing document with the stats on how many people ignore/don't realize these things?

I say we cut the ability to X out, as it's not going to hurt anyone to choose either to just show the location bar (then close), or a second option, show everything it hid (such as status, menu, location) (then close).
If that hurts any functionality on the user's side, I don't know what the world is coming to. If you don't want the location bar, too bad, we'll do it because it concerns security -- as someone so eloquently put it "we can't trust the user to protect themselves."

Just some ideas.

	Jordan M.
	Comment 22 • 18 years ago

(In reply to comment #10)
> Connor was concerned that flipping this pref will break a bunch of our code
> which uses "was the window opened with chrome or not" as a heuristic to
> determine if a window is or is not a popup (f.e. opening a link from an
> external app in new tab when the last-focused Firefox window was a popup
> window).
> 
> Maybe we can just use the notificationmessage solution. Just show:
> 
>  .---------------------------------------------------------------------.
>  |Paypal: Enter your personal information                        _ [] X|
>  |---------------------------------------------------------------------'
>  | /!\ This is 123.123.123.123                    (Trust this site) (X)|
>  |---------------------------------------------------------------------|
> 
Like this idea a bit more (sorry for all of the replies, I'm just throwing ideas out, in case we choose this-or-that), but still with the X. Not to fond of the "trust this site" being the only option, reminds me of an IE message which I just click through -- Unless the X is "don't trust this site?" Might want to clear what you're thinking up. As for the message, I was thinking more along the lines of "this site (123.123.123.123) is not the site you requested," since given the notification, I'm supposing: They requested the site (how else would we know 123.123.123.123 isn't the site they wanted? -- or are we thinking of something else?)

	Jordan M.
	Comment 23 • 18 years ago

After much deciphering of my previous post, these are two separate issues, unless I'm mistaken:

A.) This bug is dealing with disabled location bars, or whatnot, and we should ask: 1. If site X is the site they wanted 2. If they want to see the location bar 3. Combine the two into one message or what have you.

B.) Trusting a site would mean that you were either: Re-directed from somewhere, you requested another site, etc.

Not sure if B should even be considered a threat, seeing as by itself, it would mean the site you clicked was cracked and you got re-directed to a phony site.

	Phil Ringnalda (:philor)
	Comment 24 • 18 years ago

(In reply to comment #18)
>  mconnor demonstrated some problems on a Mac.

Was it a three card monte demonstration, or did he have some fix for bug 303568 that made it work in the non-disable_window_open_feature.location case?

	Jordan M.
	Comment 25 • 18 years ago

Allowing the menu bar and other things (e.g. toolbar) may be useful to unhide, such as in the testcase of bug 37055 as well, since we're on the topic of notification messages, if we're going to go with that (thought I'd reinforce my earlier mention of this in comment 21 with a testcase that might show more relevance).

	Jordan M.
	Comment 26 • 18 years ago

Me and my silly mistakes: bug 370555

	Jordan M.
	Comment 27 • 18 years ago

(In reply to comment #23)
> After much deciphering of my previous post, these are two separate issues,
> unless I'm mistaken:
> 
> A.) This bug is dealing with disabled location bars, or whatnot, and we should
> ask: 1. If site X is the site they wanted 2. If they want to see the location
> bar 3. Combine the two into one message or what have you.
> 
> B.) Trusting a site would mean that you were either: Re-directed from
> somewhere, you requested another site, etc.
> 
> Not sure if B should even be considered a threat, seeing as by itself, it would
> mean the site you clicked was cracked and you got re-directed to a phony site.
> 
Am I the only one who got stuck with this thinking?

The first chart: Check, makes sense
Second chart: Makes semi-sense. But, why would the pop-up window originate from somewhere else? Am I reading the chart wrong? Apparently a legit site is going to have a pop-up window which... Does absolutely nothing?
Third chart: This site is 123.123.123.123? Is that a pop-up window? And what is "trust this site?" If you type in "paypal.com" go to that page, there's going to be a pop-up about 123.123.123.123? Wouldn't that render that window pointless, seeing as the original window isn't exactly connected to the pop-up?

Can someone clarify? Because I must be confused somewhere.

	Jordan M.
	Comment 28 • 18 years ago

Haha, wow, I feel like an idiot now... Had nothing to do with what I thought it did...

Nevermind comments 27, 23, and 22.

Was being way too literal with the diagrams.

	Michael Ventnor
	Comment 29 • 18 years ago

(In reply to comment #28)
> Haha, wow, I feel like an idiot now... Had nothing to do with what I thought it
> did...
> 
> Nevermind comments 27, 23, and 22.
> 

If we had a "best bugzilla moments" site this would be almost at the top =)

Regarding this bug, I don't think a notification bar is enough. You'd have to listen for changes to the host and dynamically update the bar, not only that but the user may want to know the whole URL rather than just the host.

For the sake of code size and consistency I'd prefer if we can fix whatever's holding us back from flipping the pref then flip it. Based on what I've seen so far, and from my own private pref-flip, most of the concerns from this bug are outdated.

	Jordan M.
	Comment 30 • 18 years ago

(In reply to comment #29)
> (In reply to comment #28)
> > 
> 
> If we had a "best bugzilla moments" site this would be almost at the top =)
> 

Yeah, I have ADD, and I can't think of much at the same time, thus why I pretty much doubled this bug's comments.

Thanks for not slaughtering me. XP

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 31 • 17 years ago

Johnathan and I will finalize the design and either come up with a patch or work with someone to do so. This blocks Firefox 3.

	Johnathan Nightingale [:johnath] Assignee
	Comment 33 • 17 years ago

Notes on this patch:

- It adds browser/base/content/chromelessWindowOverlay.js to the tree
- It adds a pref (security.warn_chromeless_window.infobar) which defaults to true.  Turning it off (about:config only, this shouldn't be easy) disables the behaviour introduced in the patch.
- It doesn't have whitelist support.  I'm reluctant to introduce *another* whitelist here, and I also don't think it would be right to piggyback on the existing popup whitelist since the meanings/trust decisions are not identical. As schrep says, even without a way to suppress it, we need this behaviour for feature parity and phish-stomping anyhow.  If people want to discuss whitelisting, I would recommend opening a separate, follow-on bug.

	Daniel Veditz [:dveditz] Reporter
	Comment 35 • 17 years ago

Comment on attachment 265983 [details] [diff] [review]
Patch to add chromeless window detection/handling

>+// Show infobar on chromeless windows
>+pref("security.warn_chromeless_window.infobar", true);

Please use a browser.foo pref, security. is for back-end global security settings (primarily PSM) rather than app-specific ones.

>+  if(document.documentElement.getAttribute("chromehidden").indexOf("toolbar") != -1)

This is probably somewhat tangential, but someone could hide the location bar without hiding the toolbar.

javascript:open("http://www.mozilla.org","","location=no,toolbar=yes")

It doesn't let them add their own fake chrome so maybe it's outside the scope of this bug, but it does allow them to hide where they are. More or less, anyway, the real host would appear in the title bar but you couldn't find the rest of the URL.

Personally I like the current mini-bar, assuming it gets updated to show the new identity indicators. But it's relatively unobtrusive which means maybe it doesn't help as much. But it's not going to get as many intranet-app authors screaming at us, either. I fear those sorts of people will agitate to lock off the feature entirely if there's no way to whitelist their sites.

	Daniel Veditz [:dveditz] Reporter
	Comment 36 • 17 years ago

> Personally I like the current mini-bar

Your screen-shot is way prettier and better for Firefox in general, ignore my pointless obsession with knowing the full URL at all times. I will happily continue using the dom.disable_window_open_feature.location pref but there's no need to foist that on everyone else if we've got a better-looking solution.

	Michael Ventnor
	Comment 37 • 17 years ago

What if this window gets redirected to a totally different domain? We need a way to keep this microbar up-to-date at all times. Please make sure this listens for location changes, and update the microbar if the host changes.

	Michael Ventnor
	Comment 38 • 17 years ago

Actually, never mind. It appears you're using the opener instead.

> +chromelessWindow.warningMessage=The web site at %S has hidden your toolbar.
> +chromelessWindow.warningNoLocation=This web site has hidden your toolbar.

s/toolbar/toolbars.

	Johnathan Nightingale [:johnath] Assignee
	Comment 39 • 17 years ago

Dan - I like seeing the full URL too, but I agree we are in the minority.  :)  I have added a check for chromehidden location as well.  As you say, I think it's less likely, since it's a harder spoof, but it's annoying and probably deceptive, and this gives us an easy way to bring it back.

Ventron - As you mention, I am using opener, not current url (since it was the opener who hid things) but I have updated the strings as you suggested.

	Johnathan Nightingale [:johnath] Assignee
	Comment 40 • 17 years ago

Caught a bug where gBrowser is undefined for windows without web content (e.g. download manager, error console).  Fix to return immediately on !gBrowser, since we don't need to handle those here.

	Mike Kowalski
	Comment 41 • 17 years ago

Can't this message be spoofed? Look at bug 252257. Opera has a small bar with the url, and when you click it, a full toolbar appears.

	Jesse Ruderman
	Comment 42 • 17 years ago

It can't be spoofed because the top of the window is always either a real normal location bar or a real microbar.

	Johnathan Nightingale [:johnath] Assignee
	Comment 43 • 17 years ago

(In reply to comment #42)
> It can't be spoofed because the top of the window is always either a real
> normal location bar or a real microbar.

Right - a site could open a chromeless popup and spoof their own infobar, but our infobar would still be present.  And as mentioned in bug 252257, spoofing matters, but it's a generic problem.  I don't see that the spoofing issue is made any worse by this fix, whereas several other spoofing scenarios are prevented. 

	Mike Connor [:mconnor]
	Comment 44 • 17 years ago

Comment on attachment 266380 [details] [diff] [review]
Fix for gBrowser has no properties error

Index: browser/base/content/chromelessWindowOverlay.js

this is an odd nit, but why have a separate file?  this will just add overhead and extra add/remove listeners.

yes, browser.js is a mess, but breaking stuff into separate files just creates overhead that isn't needed.  After we wrap up Fx3 I want to get someone to split the files into manageable chunks that combine at build time.

+# The Initial Developer of the Original Code is
+#   Johnathan NIGHTINGALE <johnath@mozilla.com>
+#
+# Contributor(s):
+#

typically, aiui, you'd want to have Mozilla Corporation as the initial developer
with you listed as a contributor, but I think this should be in browser.js anyway

+  if(!gBrowser) {
+    return;
+  }

nit, missing space after if (you do this a lot, just stop please ;), braces around single line are bad

+  var prefs = Components.classes["@mozilla.org/preferences-service;1"].
+                    getService(Components.interfaces.nsIPrefBranch);

odd style here too (align periods when not using the Cc and Ci consts
and the alternate style for those)

  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
                        .getService(Components.interfaces.nsIPrefBranch);


 
+    var bundle_browser = document.getElementById("bundle_browser");
+    var messageString;    
+    if(window.content && window.content.opener && window.content.opener.location)
+      messageString = bundle_browser.getFormattedString("chromelessWindow.warningMessage",
+                                                        [window.content.opener.location.host]);
+    else
+      messageString = bundle_browser.getString("chromelessWindow.warningNoLocation");

use instead:

try {
  var messageString = bundle_browser.getFormattedString("chromelessWindow.warningMessage",
                                                        [window.content.opener.location.host]);
} catch (ex) {
  messageString = bundle_browser.getString("chromelessWindow.warningNoLocation");

} 

+    var notificationName = "chromeless-info";
+    var buttons = [{
+      label: bundle_browser.getString("chromelessWindow.showToolbarsButton"),
+      accessKey: bundle_browser.getString("chromelessWindow.accessKey"),
+      popup: null,
+      callback: function() { return showToolbar(); }
+    }];

+    var notificationBox = gBrowser.getNotificationBox();
+    if (!notificationBox.getNotificationWithValue(notificationName)) {

maybe make this 

if (notificationBox.getNotificationWithValue(notificationName)) {
  Components.utils.reportError("Already have a chromeless-info notification!")
  return;
}

and move the buttons stuff after the check (not that this should ever happen, but this is just weird flow control

+      const priority = notificationBox.PRIORITY_INFO_HIGH;
+      const iconURL = "chrome://browser/skin/Info.png";

You're only using these once, why bother with the overhead here?  same with notificationName, really.

+/**
+ * Callback for "Show Toolbars" button in notification box.  Resets visibility of
+ * the go button stack and url bar, basically counteracting the chromehidden work
+ * in browser.js
+ */
+function showToolbar() {
+  
+  // Unhide the chrome elements
+  var chromehiddenStr = document.documentElement.getAttribute("chromehidden");
+  chromehiddenStr = chromehiddenStr.replace("toolbar", "")
+                                   .replace("location", "");
+  document.documentElement.setAttribute("chromehidden", chromehiddenStr);

just nuke chromehidden here, I'd think.

+  // Undo the URLBar tweaks performed in browser.js when the url bar
+  // was chromehidden
+  if(gURLBar) {
+    gURLBar.setAttribute("readonly", "false");
+    gURLBar.setAttribute("enablehistory", "true");
+  }

+  var goButtonStack = document.getElementById("go-button-stack");
+  if (goButtonStack)
+    goButtonStack.setAttribute("hidden", "false");

removeAttribute("hidden") is faster here (same with readonly, above)

+/**
+ * It isn't critical that we run during the onload sequence, so just add
+ * ourselves to the end of the event loop.
+ */
+function startup() {
+  window.removeEventListener("load", checkForChromelessWindow, false);
+  setTimeout(checkForChromelessWindow, 0);
+}
+
+window.addEventListener("load", startup, false);

just call checkForChromelessWindow from browser.js's delayedStartup, no event listeners needed

\ No newline at end of file

bad johnath, no biscuit

Index: browser/base/content/global-scripts.inc
===================================================================
RCS file: /cvsroot/mozilla/browser/base/content/global-scripts.inc,v
retrieving revision 1.13
diff -u -8 -p -r1.13 global-scripts.inc
--- browser/base/content/global-scripts.inc	5 Jan 2007 11:45:15 -0000	1.13
+++ browser/base/content/global-scripts.inc	28 May 2007 14:12:12 -0000
@@ -49,8 +49,9 @@
 #ifndef MOZ_PLACES_BOOKMARKS
 <script type="application/x-javascript" src="chrome://browser/content/bookmarks/bookmarks.js"/>
 <script type="application/x-javascript" src="chrome://browser/content/bookmarks/bookmarksMenu.js"/>
 #endif
 <script type="application/x-javascript" src="chrome://global/content/viewZoomOverlay.js"/>
 <script type="application/x-javascript" src="chrome://browser/content/browser.js"/>
 <script type="application/x-javascript" src="chrome://global/content/inlineSpellCheckUI.js"/>
 <script type="application/x-javascript" src="chrome://global/content/viewSourceUtils.js"/>
+<script type="application/x-javascript" src="chrome://browser/content/chromelessWindowOverlay.js"/>

I think we don't want this in global-scripts, we want it in browser.xul directly

Index: browser/locales/en-US/chrome/browser/browser.properties
===================================================================
RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v
retrieving revision 1.35
diff -u -8 -p -r1.35 browser.properties
--- browser/locales/en-US/chrome/browser/browser.properties	28 Mar 2007 04:37:32 -0000	1.35
+++ browser/locales/en-US/chrome/browser/browser.properties	28 May 2007 14:12:13 -0000
@@ -86,8 +86,14 @@ tabContext.undoCloseTabAccessKey=U
 menuOpenAllInTabs.label=Open All in Tabs
 menuOpenAllInTabs.accesskey=o
 
 # Block autorefresh
 refreshBlocked.goButton=Allow
 refreshBlocked.goButton.accesskey=A
 refreshBlocked.refreshLabel=%S prevented this page from automatically reloading.
 refreshBlocked.redirectLabel=%S prevented this page from automatically redirecting to another page.
+
+# Chromeless popup handling
+chromelessWindow.warningMessage=The web site at %S has hidden your toolbars.
+chromelessWindow.warningNoLocation=This web site has hidden your toolbars.
+chromelessWindow.showToolbarsButton=Show Toolbars
+chromelessWindow.accessKey=s

S, not s (accesskeys search same case, then opposite case, so this would show at the end instead of the beginning

close, but not there yet

	Johnathan Nightingale [:johnath] Assignee
	Comment 46 • 17 years ago

(In reply to comment #44)
> this is an odd nit, but why have a separate file?  this will just add overhead
> and extra add/remove listeners.

Sold.  Now incorporated into browser.js.

> typically, aiui, you'd want to have Mozilla Corporation as the initial
> developer
> with you listed as a contributor, but I think this should be in browser.js
> anyway

Yeah, we've covered that.  :)

> nit, missing space after if (you do this a lot, just stop please ;), braces
> around single line are bad

Fixed 5 instances of if(, and removed 2 instances of single-statement braces.  Stupid IBM style guide.

> +  var prefs = Components.classes["@mozilla.org/preferences-service;1"].
> +                    getService(Components.interfaces.nsIPrefBranch);
> 
> odd style here too (align periods when not using the Cc and Ci consts
> and the alternate style for those)

Done.

> use instead:
> 
> try {
>   var messageString =
> bundle_browser.getFormattedString("chromelessWindow.warningMessage",
>                                                        
> [window.content.opener.location.host]);
> } catch (ex) {
>   messageString =
> bundle_browser.getString("chromelessWindow.warningNoLocation");
> } 

Done. 

> +    var notificationBox = gBrowser.getNotificationBox();
> +    if (!notificationBox.getNotificationWithValue(notificationName)) {
> 
> maybe make this 
> 
> if (notificationBox.getNotificationWithValue(notificationName)) {
>   Components.utils.reportError("Already have a chromeless-info notification!")
>   return;
> }
> 
> and move the buttons stuff after the check (not that this should ever happen,
> but this is just weird flow control

Done.

> +      const priority = notificationBox.PRIORITY_INFO_HIGH;
> +      const iconURL = "chrome://browser/skin/Info.png";
> 
> You're only using these once, why bother with the overhead here?  same with
> notificationName, really.

Done for priority and iconURL.  notificationName is used twice, so left as-is.

> +  // Unhide the chrome elements
> +  var chromehiddenStr = document.documentElement.getAttribute("chromehidden");
> +  chromehiddenStr = chromehiddenStr.replace("toolbar", "")
> +                                   .replace("location", "");
> +  document.documentElement.setAttribute("chromehidden", chromehiddenStr);
> 
> just nuke chromehidden here, I'd think.

Done, though I nuked by setting to "" instead of removeAttribute'ng, because there are multiple places in the code where we do:

document.documentElement.getAttribute("chromehidden").indexOf...

so I didn't want to undefine it.

> +    goButtonStack.setAttribute("hidden", "false");
> 
> removeAttribute("hidden") is faster here (same with readonly, above)

Done for both.

> just call checkForChromelessWindow from browser.js's delayedStartup, no event
> listeners needed

Seriously, I promise we're already moved to browser.js.  :)

> Index: browser/base/content/global-scripts.inc

> I think we don't want this in global-scripts, we want it in browser.xul
> directly

Browser.js, I take it you mean, and yes, done.

> +chromelessWindow.showToolbarsButton=Show Toolbars
> +chromelessWindow.accessKey=s
> 
> S, not s (accesskeys search same case, then opposite case, so this would show
> at the end instead of the beginning

Ah, whoops.  Done.

	Mike Connor [:mconnor]
	Comment 47 • 17 years ago

Comment on attachment 267309 [details] [diff] [review]
Post-review patch


+function checkForChromelessWindow() {
+
+  // Windows without web content (pref dialogs, download manager, etc) don't need
+  // any handling here.
+  if (!gBrowser)
+    return;

gBrowser is definitely defined, or the event listener call 150 lines earlier already threw.

Your option if you're really paranoid is to just replaces gBrowser below with getBrowser() but I think that's not necessary

+  if (document.documentElement.getAttribute("chromehidden").indexOf("toolbar") != -1 ||
+     document.documentElement.getAttribute("chromehidden").indexOf("location") != -1) {

off by one indent

+    var bundle_browser = document.getElementById("bundle_browser");
+    var messageString;
+    try {
+      messageString = bundle_browser.getFormattedString("chromelessWindow.warningMessage",
+                                                        [window.content.opener.location.host]);
+    } catch (ex) {
+      messageString = bundle_browser.getString("chromelessWindow.warningNoLocation");
+    }

no need to define messageString outside of the try, save yourself a line.

r=me with nits fixed

	Johnathan Nightingale [:johnath] Assignee
	Comment 48 • 17 years ago

Nits addressed, added my name to browser.js contributors list too.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 49 • 17 years ago

Comment on attachment 265985 [details]
Screencap of default presentation now

ui-r+

I'm a little worried about the interaction on sites (especially web applications and gallery sites) that throw up chromeless windows appropriately, but let's start here and see where it takes us.

	Johnathan Nightingale [:johnath] Assignee
	Comment 50 • 17 years ago

After discussion with Shaver, mconnor, and gavin in IRC, I've put together a slight modification which:

a) adds commenting around the try/catch block
b) logs the exception, if caught (we previously felt this would be log-spam, because there was a timing issue with window.content.opener being set.  The move to the end of browser.js's delayedStartup fixed that, though, so this really is exceptional)
c) Replaces the setAttribute("chromehidden", "") call with removeAttribute("chromehidden");  This was worrisome at first, since other code calls getAttribute("chromehidden").indexOf without sanity checks, but gavin confirmed that getAttr on removed attrs in XUL returns "", not null.

	Johnathan Nightingale [:johnath] Assignee
	Comment 51 • 17 years ago

Updating summary to reflect the direction this bug eventually took.

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 17 years ago

That last patch caused a mochitest failure, for good reasons.  The effect of showing the infobar in the particular way it's done on the standard |window.open("", "", "width=X,height=Y")| call is that the window is not the size the page requested until the infobar is dismissed.  That means that user has to click away the silly thing to view the image (almost always) the site is trying to show.

Is the expected workflow really that the user will click the bar away on every single image popup (e.g. the various image things on the BBC news site, similar for NY Times, etc) window they want to view?  That seems highly suboptimal.

Frankly, I think we should show whatever site indentity indicator we settle on (url bar replacement, whatever) in all windows at all times.  That should cover this bug, no?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 53 • 17 years ago

As bz mentions, this caused a failure of the test for bug 344861 (http://lxr.mozilla.org/seamonkey/source/docshell/test/test_bug344861.html). I've flipped the pref for now, to fix the orange.

	Johnathan Nightingale [:johnath] Assignee
	Comment 54 • 17 years ago

By and large, I'd say the expectation is that users will mostly leave it be, much as I expect a lot of them leave the "we blocked a popup" infobar currently.  Users who want to disable the pref can do so, but really, it exists to prevent a wide-open hole in terms of chrome-spoofing or even just hidden chrome, by giving users a way out ("Show toolbars".)

With that being said, I think earlier posters in this bug are right to observe that people may want a way to whitelist this away.  As the discussion in the bug already indicates, this is something anticipated, but we were hoping to get the behaviour into the alphas and betas, and see if the real world impact was enough to justify adding yet-another-whitelist, or possibly piggybacking of an existing one.

The suggestion that we microbar it instead, that is, that we take the URL-bar plus whatever hangers-on we think appropriate, is a possibility.  In fact, we could do it by just flipping the disable-location-bar pref as the first patch did, but:

a) mconnor and others dismissed this approach above by suggesting it would break other code which watches chromehidden

b) it still puts our users in the position of having no way to get those controls back (maybe this is okay, but it is a difference from the current approach)

c) The current infobar speaks in terms of the opener when it talks about hidden chrome, since that's really the opener's decision, not the content of the current page.  Going to a URL bar obscures that.

My preference would be to turn the pref back on, open a follow-up bug to track the question of whitelisting sites, and in the meantime tell mochitest to disable the pref, since it's really concerned with whether we made the right sized window (which we did), moreso than whether we then overlaid it with something.  

I'm willing to be wrong here, but I agree with schrep in comment 8 when he says (if I'm interpreting his words appropriately) that even without a whitelist way to suppress it, this is something we need for phish-stomping, and for parity with IE7.

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 17 years ago

If we expect users to leave the infobar in place, then we are NOT making the right-sized window.  The right-sized window is the one that will give the content the amount of space it asked for with the user's default behavior and the resulting chrome.  Keep in mind that the infobar doesn't "overlay" the content window; it just makes it smaller.

If the testcase were testing outerWidth/outerHeight window options, then the infobar would not matter.  But that's not what it tests, and what it tests is what we don't want to regress.

So if we stick with the current approach we need to be showing the infobar early enough that the window-sizing code that runs after onload will see the right chrome size.

	Johnathan Nightingale [:johnath] Assignee
	Comment 56 • 17 years ago

(In reply to comment #55)
> So if we stick with the current approach we need to be showing the infobar
> early enough that the window-sizing code that runs after onload will see the
> right chrome size.

Would doing that cause the window to be too big if they did dismiss it?

	Nelson Bolyard (seldom reads bugmail)
	Comment 57 • 17 years ago

Question:

I currently try to eliminate chromeless windows with these preferences:
It works in MOST (but, sadly, not all) cases.

user_pref("dom.disable_open_during_load", true);
user_pref("dom.disable_window_open_feature.close", true);
user_pref("dom.disable_window_open_feature.directories", true);
user_pref("dom.disable_window_open_feature.location", true);
user_pref("dom.disable_window_open_feature.menubar", true);
user_pref("dom.disable_window_open_feature.minimizable", true);
user_pref("dom.disable_window_open_feature.personalbar", true);
user_pref("dom.disable_window_open_feature.resizable", true);
user_pref("dom.disable_window_open_feature.scrollbars", true);
user_pref("dom.disable_window_open_feature.titlebar", true);
user_pref("dom.disable_window_open_feature.toolbar", true);

My question is: with this new notification bar feature, 
will I see both the chrome *AND* the notification bar in otherwise
chromeless windows?  (Hope not.)

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 17 years ago

> Would doing that cause the window to be too big if they did dismiss it?

Yes.  I really don't see a good way around that problem, to be honest, if we're using this transient UI.  I suppose we could shrink the window in the process of closing the notification bar, but that's a bit more work.

And given the choice, I'd take a few pixels too big over a few pixels too small from a usability perspective.  But this does hinge on the relative frequencies of users leaving it as-is and dismissing it...

	inarius
	Comment 59 • 17 years ago

Speaking of window size / position, wouldn't there be an easy way to circumvent this warning message by positioning the window above the screen?

If, for instance, the warning bar is 20 pixels tall, the spoofer could simply position the window with y = -20 and the first 20 pixels of the web page could be something inconspicuous, like a fake title bar.

I don't know how feasible this is... just a thought.

	Boris Zbarsky [:bzbarsky]
	Comment 60 • 17 years ago

> by positioning the window above the screen?

You're only allowed to do that if you have extra privileges.  See nsWindowWatcher::SizeOpenedDocShellItem the |if (!enabled)| clause; the one that starts with the comment:

  1903     // Security check failed.  Ensure all args meet minimum reqs.

Of course a 2-minute test of window.open would have shown that a negative top doesn't work... ;)

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 61 • 17 years ago

Just an rudimentary example of how a notificationbox could be popped up over the content. The notification is semi-transparent so you can see some content under it. a click anywhere (on or off the notification) will dismiss the notification

	Dão Gottwald [:dao]
	Comment 62 • 17 years ago

(In reply to comment #61)
> Created an attachment (id=267713) [details]
> example of using a notificationbox in a popup with transparency

Transparency doesn't seem to work for me, and even if it did, it would only be suboptimal. More important, overlaying the scroll bar seems like a real mistake.

Overall, I think flipping the Location Bar pref and fixing "window targeting behaviour that relies on chromehidden" is the obvious and right solution for this bug.

	Boris Zbarsky [:bzbarsky]
	Comment 63 • 17 years ago

I don't think we support popup transparency across all our platforms (though I could be wrong).

	Dão Gottwald [:dao]
	Comment 64 • 17 years ago

(In reply to comment #62)
> fixing "window targeting behaviour that relies on chromehidden"

The popups' Location Bar is readonly. How about adding that to the heuristics?

	Johnathan Nightingale [:johnath] Assignee
	Comment 65 • 17 years ago

mconnor suggested turning this on for A6 so that we get some more data on the scope of the impact.  That means todo'ng the mochitests in docshell/test/test_bug344861.html as well.  

After A6 goes out the door, we bring back the mochitest and toggle the pref back off, but at least we get some more user info.

	Jesse Ruderman
	Comment 66 • 17 years ago

We *know* that if we check this in with the problems the mochitests hit (comment 52), we'll break tons of sites unnecessarily.  Turning this on for A6 will just give us more noise, less testing of other changes, and less useful testing/feedback when we fix this correctly.

	Boris Zbarsky [:bzbarsky]
	Comment 67 • 17 years ago

For what it's worth, the only way I'm at all OK with that patch landing is in the release freeze when we're absolutely sure nothing else will land between it and release.  And I'd want it backed out before the tree reopens.  I don't want us missing other regressions in this.

All that said, I pretty much agree with Jesse.

	Johnathan Nightingale [:johnath] Assignee
	Comment 68 • 17 years ago

So the goal here was to get some data on how much breakage it actually created,
and also to see if the UI was useful enough to justify the effort of fixing the
sizing bugs that break the tests.  I do think a dismissible/whitelistable
notify bar is a better UI option than just locking the location bar on.

But I also agree that the time to do it is not on the eve of A6.

What do you think about leaving it as-is for A6, and flipping it once the tree
re-opens post-A6, to at least get it in front of the people dogfooding?  

	Mike Connor [:mconnor]
	Comment 69 • 17 years ago

I guess I'm not understanding just how fragile/breakable this behaviour is, and why this test is so critical that we can't have it off for even a single checkin.

FWIW, if any content triggers the popup blocked or missing plugins notification bar, they've been broken on that test since before Firefox 1.0, so this is just making that problem global, instead of buried in edge cases.

	Boris Zbarsky [:bzbarsky]
	Comment 70 • 17 years ago

I'd say that any regression test shouldn't be turned off once it's on.  This one is not special.  Given that, I'd have an even bigger problem with flipping this on trunk for a while than I do with doing it just for the release.

As for the other notifications, the most common (by far) use cases of sized windows don't hit those situations, as you noted.  So it hasn't really been a problem.  It's making every single sized window broken that's something we want to avoid.

	Johnathan Nightingale [:johnath] Assignee
	Comment 72 • 17 years ago

This patch flips the original pref to disable hiding of the location bar.  It also fixes the behaviour in nsBrowserContentHandler.js which was depending on 

window.toolbar.visible

replacing the check with 

!win.document.documentElement.getAttribute("chromehidden")

	Nelson Bolyard (seldom reads bugmail)
	Comment 73 • 17 years ago

Johnathan, does this mean that people who (like me) set set 
dom.disable_window_open_feature.location to true to disable hiding of 
the location bar must now reverse the sense of our preferences?

	Johnathan Nightingale [:johnath] Assignee
	Comment 74 • 17 years ago

(In reply to comment #73)
> Johnathan, does this mean that people who (like me) set set 
> dom.disable_window_open_feature.location to true to disable hiding of 
> the location bar must now reverse the sense of our preferences?

I'm not sure I understand your question but nothing should change for you -- you've already disabled locbar hiding, it will continue to be disabled.  This just changes new profiles to also set that pref by default.  

The changes to the targetting code mean that you might notice improved behaviour for external link clicks, which previously might have opened in your popup windows (if they were top-of-z-order) and should now open in real browser windows instead.

	Mike Connor [:mconnor]
	Comment 75 • 17 years ago

Comment on attachment 277763 [details] [diff] [review]
Flip the dom.disable_window_open_feature.location pref and fix the window targetting

let's get this landed.

I'm still interested in the notification, but that's not in scope anymore.

	Johnathan Nightingale [:johnath] Assignee
	Comment 76 • 17 years ago

I've confirmed that this patch passes mochitest as well.  Setting checkin-needed.

	Dave Townsend [:mossop]
	Comment 78 • 17 years ago

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.197; previous revision: 1.196
done
Checking in browser/components/nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.43; previous revision: 1.42
done

	Phil Ringnalda (:philor)
	Comment 81 • 17 years ago

If, as it appears, the attachment 267456 [details] [diff] [review] notification bar is going to stay in the tree preffed off, you might want to in-litmus? it, since it's quite unlikely that anyone mucking with toolbars or the addressbar will be checking the effects of their work on something so thoroughly hidden away (I'd completely forgotten it, until I was looking for things using PRIORITY_INFO_HIGH).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 82 • 17 years ago

(In reply to comment #81)
> If, as it appears, the attachment 267456 [details] [diff] [review] notification bar is going to
> stay in the tree preffed off

It's not, see bug 397695.

	t mau
	Comment 84 • 15 years ago

What I haven't read in this bug is discussion about other possible solutions. 

I gathered that you're trying to make sure phishing sites remain obvious to reasonably smart users. THat's great.

How about web applicatinos design? If the window.open() is fired from an explicit click by the end user, and if the new window is on the same domain as the referrer, then why can't the location bar be hidden?

I'm not familiar with how this is built, but it seems a possible way to appease web apps builders who are very much keen on controlling window size/behavior in order to deliver the best user experience.

	Boris Zbarsky [:bzbarsky]
	Comment 85 • 15 years ago

I don't see how the "if the new window is on the same domain as
the referrer" helps anything.  The whole problem is windows that are on the same site as the referrer, since that site is not the site the user thinks it's looking at.

	Daniel Veditz [:dveditz] Reporter
	Comment 86 • 15 years ago

And the popup can be navigated once opened, which is either going to lead to spoofing or ugly jittering as the location mini-bar opens and closes.

> I gathered that you're trying to make sure phishing sites remain obvious to
> reasonably smart users. THat's great.

That's part of the goal, but even on non-phishing sites we believe users should be able to know where they are. The users are not asking that the mini-bar go away so it's clearly not bothering them, and clever designers take that space into account. They'll have to -- IE behaves this way too.

Incidentally, random advocacy thrown into a RESOLVED FIXED bug isn't the way to get changes made. Especially for UI changes you need to convince people in our product newsgroups that it's a good change.