304089 - possible to hide security UI by resizing the browser offscreen via js

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Josh Aas]

Window size constraining is broken on the Mac.

window.resizeTo(2000,2000);

This will make the bottom of the window go offscreen by something like 20-22
pixels, enough to hide our secure site UI. Sites could quietly resize a
full-screen browser window and fake the security UI.

(I found this with Jesse Ruderman)

Comment 1

[Josh Aas]

Attachment: initial window size fix, v1.0

For one thing, we shouldn't be setting an initial window height that is taller
than the available screen height. This patch fixes that for screens with
(screen.availHeight > 600).

This patch does not fix the whole bug. That is coming up (I think I've almost
got it).

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 192178 [details] [diff] [review]
initial window size fix, v1.0

This is why resizing is evil, but ok.  If this is going to be fixed the right
way later, please amend the comment with the bug number you're going to fix it
under.

Comment 3

[Daniel Veditz [:dveditz]]

I imagine this is a 1.0x problem, too? Does the same thing happen in the 1.7 Suite?

Comment 4

[Josh Aas]

mconnor: Right now there is no better way to fix what "initial window size fix"
fixes without adding some sort of DOM mechanism for consistently taking into
account title bar heights. I have filed bug 304184 for adding such
functionality. For now I agree that we should go with my patch.

dveditz: yes, it is a problem on FF 1.0.x. Haven't tried suite.

Comment 5

[Josh Aas]

Attachment: real fix, v1.0

This patch actually fixes the problem. I am going to get some food - I'll look
it over again before requesting review.

Basically, we used old bad Quickdraw API stuff to get available screen real
estate, which didn't take into account the Mac OS X menu bar or the dock. Then
we didn't take a window's title bar into account.

Comment 6

[Josh Aas]

Comment on attachment 192317 [details] [diff] [review]
real fix, v1.0

Requesting review. All I see in my own patch is that we don't need to capture
aScreenX or aScreenY, and this comment could be more clear:

// we want available content size, so subtract off space for a title bar

This patch has r=shaver.

Comment 7

[Josh Aas]

BTW - this patch fixes a couple other Mac OS X bugs, which I will mark as fixed
when this lands.

Comment 8

[Simon Fraser [no longer active]]

(In reply to comment #7)
> BTW - this patch fixes a couple other Mac OS X bugs, which I will mark as fixed
> when this lands.

Can you mark them as dependencies please.

Comment 9

[Josh Aas]

My patch has some issues with multiple monitors. Posting a new one soon.

Comment 10

[Josh Aas]

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

Comment 11

[Josh Aas]

initial window size fix v1.0 checked in to trunk

Comment 12

[Josh Aas]

I changed my mind about "real fix, v1.0" breaking multiple monitors. I think it
is fine. We only have 2 ways to deal with available screen space in Carbon. If
we use GetGrayRgn, like we were before, we could hard-code some value for the
menu bar but we could never account for the dock. If we use
GetAvailableWindowPositioningBounds(), we can account for the dock and the menu
bar nicely, but only for a single screen. That is what my patch does now. That
means that we limit the size of the window to the size of its main screen. You
can still drag a window between screens, even split it between screens, but you
could not make it the size of both screens, vertically or horizontally.

Under normal multiple monitor use, that is fine really. Hardly anybody makes a
browser window so big that it could not possibly fit on one window. While doing
so might be a neat technical feat, allowing for that should not cause us to
sacrifice usability for the vast majority of users who only have one screen.

In theory we could iterate through all screens, figure out their relative
positions, and coalesce their avaialable rectangles into one big one. We'd have
to cache that value, since doing that 10-20 times in a normal drag-resize
operation would be slow, but then if anything changed the cached value would
stick and users would have a funny UI. Even if we got acceptable perf without
caching, I don't think that it is a good idea and I certainly don't want to
write and debug that code for a ridiculously small audience, if anyone.

In summary, this patch is great for single-monitor users and basically every
multiple-monitor user. We'll be free of this limitation when we can use Cocoa
anyway.

Also, I did not prefix a carbon function call with "::" - I'll fix that on checkin.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 192317 [details] [diff] [review]
real fix, v1.0

With this patch, one can only a window to be under the dock (not offscreen)
until that part (which is under the dock) has the same height as the height
between the window titlebar to the menubar. We should either allow to resize
windows until the bottom of the screen or disallow to resize windows to be
under the dock (I prefer the later, which is also the way Safari behaves).

Comment 14

[Asaf Romano (gone)]

s/one can only a window/one can only resize a window

Comment 15

[Josh Aas]

Attachment: real fix, v1.1

This patch isn't ready for review yet. It fixes Mano's problem, but has another
problem. If you move a window offscreen by any amount and then try to make it
bigger via window.resizeTo(), it will snap to a size that fits entirely on
screen. It might work to test whether or not the window is offscreen at all and
prevent a snapback if it is (allow the resize). If that doesn't work, we may be
stuck with the behavior of v1.0, which wouldn't be too bad. I'll try it
tomorrow.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 192350 [details] [diff] [review]
real fix, v1.1

Let's add the following:
1. If the window is not offscreen, but is out of the available rect, it should
be possible to resize it until screen limits.
2. If it's already offscreen, don't block offscreen resizing.

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

When the security part is fixed, please mark FIXED.  File a followup on the size
constraints for multiple monitors elsewhere, if desired.

Comment 18

[Mike Pinkerton (not reading bugmail)]

i just want to make a comment that as a widget module owner, i totally disagree
with any patch going in that limits the size of a window to that of a single
monitor. I have used that before, that breaks all of apple's guidelines. 

Doing so to fix a "security" issue is not a valid reason, IMHO.

Comment 19

[Josh Aas]

Attachment: real fix, v2.0

So, this is a really complicated UI issue and it will be difficult to please
everyone. I have spent a long time examining the issues, and I have 2
proposals. The patch I am attaching now is the more involved of my 2 proposals.
It takes a crack at fixing our problem on multiple monitor systems and adds
support for not allowing scripts to resize windows under the dock. See the big
comment in the patch for one limitation, concerning the menu bar's placement in
a multiple monitor setup. Also, the patch does not allow windows to become so
big that they could not fit entirely onscreen without going under the dock. In
many other applications they can. Problem is, we can't determine whether or not
the resize is coming from script or a person.

I recommend actually playing around with both solutions I am proposing instead
of judging their merit based on my descriptions. The behavior is complex and
full of tradeoffs.

Comment 20

[Josh Aas]

Attachment: real fix, v3.0

This is a much simpler patch that fixes the immediate problem only. It
shouldn't have any major behavior regressions as a tradeoff for attempting a
more involved fix. I recommend just doing this and moving on. Bugs in window
sizing will remain, but they will in the other patch too.

Essentially, we were not accounting for the title bar of the window requesting
a resize, nor were we accounting for the height of the menu bar. This fixes
that, and that only.

So, this is my recommended solution but I will let the module owner decide
whether or not to go with this or go the route of "real fix, v2.0" with or
without some behavioral adjustments. Also, maybe somebody else can come up with
a totally different idea .

Comment 21

[Josh Aas]

Attachment: real fix, v3.1

With "real fix v3.0", if you have a screen that is not the main screen or above
or below it, but is larger that the vertical column that includes the main
screen, you will not be able to fill 22 pixels of its height (the menu bar's
height). This might annoy people, though it is a tradeoff for guaranteeing
vertical visibility in the main screen's vertical column. There is not a decent
way around this to my knowledge if we wish to take the menu bar into account on
multi-screen systems.

Here is an even more conservative patch, that does the same thing but only
subtracts off the menu bar's height if we are on a single-screen system.
Probably better to just leave multi-monitors alone altogether (except for the
title bar adjustment which is always safe the way I have implemented it).

This is my new recommended patch, over v3.0. It should't cause any behavior
that is even arguably a regression on single or multiple screen systems, and
cleans up our single-screen story nicely.

Sorry for all the patches, but this is very complicated UI and everyone has
their own idea of what is and isn't an acceptable tradeoff or compromise. For
that reason, this (the most conservative patch yet) should be our choice I
think.

Comment 22

[Mark Mentovai]

Comment on attachment 192500 [details] [diff] [review]
real fix, v3.1

v3.1 doesn't take the dock into account.  If we only solve it for one display,
we should at least do it properly by not allowing resizes to be obscured by the
dock.

v2 stops working properly when any portion of the desktop is above the menu
bar.  I know it's an unconventional setup, but we should't break it as part of
an incomplete fix here.

I think I have a way to solve this problem for the single- and multi-monitor
cases, but it requires getting the menu bar and dock bounds.  I can think of
various ways to get that information, but what I'd really like (especially for
the dock) is a single clean function.  We'd need to build the desktop's
bounding rect and then clip out the menu bar and dock from the appropriate
edges iff the menu bar or dock is positioned on the edge.  (It's not a
guarantee with multiple monitors.)

Comment 23

[Josh Aas]

v3.1 wasn't meant to solve the dock problem. It was only meant to stop windows
from getting too big to be onscreen, which is a more serious problem than them
going under the dock.

Comment 24

[Mark Mentovai]

Attachment: Handle multiple monitors gracefully - and dock too

This is an implementation of the logic I was describing.  It disallows any
resize that would put the window beyond the desktop rect, where the desktop
rect does not include the dock and menu bar if they're at the desktop edges. 
This is similar to what Safari does, except Safari never allows a script to
make a window larger than the screen it's primarily on.  Without being able to
discern between script- and user-triggered resizes, we don't have that luxury,
but I prefer it this way anyway - the philosophy is to not care about what
screen something is on if you can avoid it.

One pain that this creates is that it prevents vertical resizes that put the
window's bottom lower than the top of the space that the dock might occupy,
even if the window is entirely left or right of the dock.  Safari and most
other apps share this quirk, which may be the correct behavior depending on how
you look at it.

Very safe patch, I think.

Comment 25

[Josh Aas]

A fix for this bug fixes 211551, concerning the height of the window in full
screen mode.

Comment 26

[Josh Aas]

Comment on attachment 192564 [details] [diff] [review]
Handle multiple monitors gracefully - and dock too

You cannot restrict resizing based on where a window happens to be positioned.
Example: using this patch, put a window so its title bar is about 100 pixels
above the bottom of the screen (lets assume the dock is on the right for now).
Then go to View->Toolbars->Customize.

You have now chopped off that dialog (try to drag it to center screen), so that
it is not possible to close it, and put the menu bar in a permanently unusable
state.

Comment 27

[Mark Mentovai]

That's our behavior on the trunk (and now branch), under 1.0.x, and with any of
the patches posted here so far.  I didn't realize we were trying to solve that
problem too.  I'd be happy to help fix it - even here - but I don't think it's
really within the scope of this bug, and I don't think that Resize is the
correct place to handle it.

Comment 28

[Josh Aas]

Comment on attachment 192564 [details] [diff] [review]
Handle multiple monitors gracefully - and dock too

I didn't know that was our current behavior on the trunk. I just suspected it
and tried it after reading your patch.

Just a nit:
-  if (mWindowMadeHere) {
-      // Sanity check against screen size
-      Rect screenRect;
-    ::GetRegionBounds(::GetGrayRgn(), &screenRect);
+  if (mWindowMadeHere)
+  {

Plz put "{" on the same line with the "if" test, as in the code you removed.

Comment 29

[Asaf Romano (gone)]

Comment on attachment 192564 [details] [diff] [review]
Handle multiple monitors gracefully - and dock too

So, this has the same issue as v1.1 of josh's patch (see comment 15).

Also, the behavior of resizing the window when it's already under the dock is
still unexpected.

Comment 30

[Mark Mentovai]

Attachment: Address Mano's concerns

To address comment 15 (offscreen window snaps back), I'm now using max(window,
desktop) to set the max right edge.  If the window is already offscreen, it
won't be forced to shrink in order to remain on-screen.  This seems to be
exactly what Safari does.

To address the dock issue as most other applications do, we'd need to know the
window bounds at the beginning of a live resize.  This would need an event
handler.  However, we can handle the dock as the Finder and some other apps do
by letting the system do the hard work for us.	Here, I'm allowing the window
to resize into the dock area if it's already there: by the time we get aWidth
and aHeight during a user-initiated resize, they'll overlap the dock if the
system's own constraints allow it.  This effectively lets us retain our
existing behavior for UI resizes, but beware: the system's constraints aren't
perfect.

Comment 31

[Asaf Romano (gone)]

Comment on attachment 192674 [details] [diff] [review]
Address Mano's concerns

Looks good; please fix the error message here:

>+      if (::GetAvailableWindowPositioningBounds(screen, &availableRect)
>+          != noErr)
>+      {
>+        NS_ERROR("::GetAvailableWindowPositioningBounds couldn't");

Comment 32

[Mark Mentovai]

Attachment: Nits picked

The error message in comment 31 was partially intentional.  :)	I also added a
check for an empty device rect.  That, and Josh's braces in comment 28 are
cleaned up here.  (I'm having a hard time keeping on top of "prevailing style",
nsMacWindow is a mishmash.)

I'll take review from Mano, Josh, or both.  Core doesn't seem to have an
addl.review flag field.

Comment 33

[Mark Mentovai]

Comment on attachment 192682 [details] [diff] [review]
Nits picked

+    aWidth = PR_MAX(PR_MIN(aWidth, maxWidth), kMinWindowWidth);
+    aHeight = PR_MAX(PR_MIN(aHeight, maxHeight), kMinWindowHeight);

Enforcing the min heights is no good here, because there are nsMacWindows that
need to be smaller.  This uglifies the autofill popup.

So, I won't use the PR_MAX(..., kMin) wrappers - we'll retain the present
behavior of allowing resizes down to the OS' own minimum, and we can worry
about this if we want for 1.9.

Comment 34

[Mark Mentovai]

Attachment: v4.5

As above, r from either Josh or Mano (or Mike, who's been quiet).

Comment 35

[Asaf Romano (gone)]

Comment on attachment 192689 [details] [diff] [review]
v4.5

+    screen = ::DMGetFirstScreenDevice(TRUE);
+    while (screen != NULL) {
+      Rect screenRect = (*screen)->gdRect;
+      if (::EmptyRect(&screenRect)) {
+	 NS_ERROR("Couldn't determine screen dimensions");
+	 return NS_ERROR_FAILURE;
+      }

Better to warn here instead of error-and-return.


+    if (!::EmptyRect(&dockRect)) {
+      foundDock = PR_TRUE;
+    }

Let's declare |foundDock| here and change this to:
  PRBool foundDock = !::EmptyRect(&dockRect);


+    // desktopRect is now clear of the menu bar and dock, iff they

s/iff/if

+    if ((currentWidth != aWidth) || (currentHeight != aHeight)) {

please remove extra parens.


r=mano.

Comment 36

[Josh Aas]

Comment on attachment 192689 [details] [diff] [review]
v4.5

My original concern in this bug was that windows could be made bigger than
could fit onto a screen. While this patch fixes that bug, I am uncomfortable
with the fact that it does not always allow windows to expand to the size of
the screen. An example of what I mean is, say putting a 600 pixel wide window
the the middle of a monitor so that there is a at least a couple hundred pixels
to the left before the screen's edge. Then enter this js in the URL bar:

javascript: window.resizeTo(6000,6000)

Notice that the window stops growing at the dock, and does not become as large
as the screen's available space. This is a new behavior in this patch, and I'm
not comfortable with it as the surprise factor for developers at the whim of
their users' window positioning is much greater than before.

I looked at the code for this patch, and it looks fine. While I would be
willing to try this patch on the trunk, I do not want to r+ it for the 1.5
branch as I think the behavior is too risky. I think for the branch we should
be more conservative about behavior changes. Something along the lines of my
patch v3.1 but with more support for the dock.

While the code for this patch looks good, I am going to abstain from giving it
r+ or r- right now. Changing the review flag to point at pink.

Comment 37

[Asaf Romano (gone)]

(In reply to comment #36)

> Notice that the window stops growing at the dock, and does not become as large
> as the screen's available space. This is a new behavior in this patch, and I'm
> not comfortable with it as the surprise factor for developers at the whim of
> their users' window positioning is much greater than before.

Josh, there are only two in-tree chrome callers for this code (neither do
maybe-offscreen resizes), I also don't see why would we want to support this for
web developers (Note that |window.resizeTo| turned off by default for content,
at least on the trunk). I doubt the existence of extensions which rely on
offscreen resizes.

Comment 38

[Josh Aas]

I don't see any immediate problems with the patch either, but changes of that
magnitude have a way of causing problems. I do not think the benefit (that which
goes beyond my original problem from comment 36) is worth that risk for 1.5
right now. Not my decision though.

Comment 39

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 192689 [details] [diff] [review]
v4.5

looks like it covers all the bases. i wish there was a way such that it wasn't
so complicated but i'm not certain that's possible.

do we need to do something similar to cocoa widget?

r=pink

Comment 40

[Simon Fraser [no longer active]]

Comment on attachment 192689 [details] [diff] [review]
v4.5

I really think that it's pointless to do this without fixing the changing of
the window origin by JS as well. Also, the logic seems overly complicated, and
it doesn't prevent the hiding of the status bar on multi-screen configurations.

Comment 41

[Simon Fraser [no longer active]]

Comment on attachment 192689 [details] [diff] [review]
v4.5

I really think that it's pointless to do this without fixing the changing of
the window origin by JS as well. Also, the logic seems overly complicated, and
it doesn't prevent the hiding of the status bar on multi-screen configurations.

Comment 42

[Mark Mentovai]

Attachment: v4.6

smfr suggested that if a window is already contained entirely within a single
screen, then that screen's available area should be used instead of the
desktop's area.  That's implemented here.  This only affects script resizing,
the user is still able to manually resize a window on one screen to be as large
as the desktop area.

Note that on multi-monitor systems, if you position a window so that it's
entirely on one screen except for a portion which is not on any screen, you'll
still fall into the "not on a single screen" case and your usable desktop area
won't be constrained to the single screen.

I agree that there should also be restraints on (top,left) moves, but that's
almost certainly something for another patch, if not another bug entirely.  In
preparation for such a bug, I've moved the code that determines the usable
desktop bounds into its own method, GetDesktopRect, which comes with a big
juicy fat comment to help readers (and reviewers!) figure out what's going on
inside.

I don't have a spare monitor here to test with at the moment, but considering
the types of changes I've made, the only thing I'd want to verify is that
manual resizes allow the window to grow beyond a single screen.  (They should,
but paranoia...)

Comment 43

[Mark Mentovai]

Attachment: v4.7

> Note that on multi-monitor systems, if you position a window so that it's
> entirely on one screen except for a portion which is not on any screen,
you'll
> still fall into the "not on a single screen" case and your usable desktop
area
> won't be constrained to the single screen.

For my next trick, I've changed this: windows are now thought to be on a single
screen as long as they're visible on exactly one screen.  They're permitted to
leave that screen and have portions that aren't visible without being treated
as though they're on multiple monitors (unless, of course, multiple monitors
contain the window.)

Comment 44

[Mark Mentovai]

Restricting the size based on the window's position on screen breaks the
variable-height preferences window, when the window is too close to the bottom
of the desktop.

If this is adjusted to not take position into account, always allowing resizes
to the desktop size, things break down in the areas intended to solve Mano's
(comment 29) and Simon's (comment 40) concerns.  For example, even on a
single-screen system, with the dock at the bottom of the monitor, place the
window vertically halfway down the screen and size it so it is not obscured by
the dock.  Then, do javascript:window.resizeTo(10000,10000); to get it to max
size.  Because the window was not obscured by the dock, it will grow so that it
will be as tall as the screen, less the height of the dock and menu bar, so that
if it were moved to (0,0), it would fit perfectly.  But its position is not
(0,0), and it's now covered by the dock at its present position, so a subsequent
resizeTo max dimensions will make the window even taller, so that it will be
obscured by the dock even if it's moved to (0,0).

It's not correct to get different maxima on a subsequent resize attempt when
nothing else has changed.

What this really needs is a way to distinguish between resizes that come out of
content (JS resizes that should take window position into account) and those
that come out of chrome and from the UI (trusted "do-what-I-said" resizes with
less restrictive checking).  That means digging into nsXULWindow/nsIBaseWindow.
 I'm not sure if that's the type of change people will be comfortable with on
the branch at this point.

The other option is to not worry about dock overlap, but that's barely better
than what happens now.

Comment 45

[Mark Mentovai]

To fix this properly, I need to add an enum argument to nsIBaseWindow::setSize
and nsIBaseWindow::setPositionAndSize, and for congruity,
nsIBaseWindow::setPosition too, even if it's unused at this point.  The enum
needs to identify the source of the resize/reposition request: content, chrome,
or UI.  Resize requests from content need to be treated most restrictively. 
Requests from chrome need sanity checking, and requests from the UI are to be
treated least restrictively because the system enforces whatever constraints are
appropriate to the platform.

Although nsIBaseWindow isn't frozen, it's probably pretty widely used.

So, nsIBaseWindow2, which will be implemented at least by nsXULWindow.

Comment 46

[Simon Fraser [no longer active]]

Why isn't this a problem for other platforms?

Comment 47

[Mark Mentovai]

It is on Windows, as far as I can tell, but it seems to be less of a problem due
to the "correct" behavior of windows on that platform.  I haven't yet tested an
X build.

(Someone more familiar with Windows is welcome to step in and correct any
incorrect assumptions or conclusions here.)

On Windows, gecko passes the resize values straight down to the system without
checking them or enforcing any maxima.  The system's behavior crimps the resize
at roughly the size of the screen (or desktop?) - it actually allows it to be a
little bit larger, such that the window will fill the screen entirely when its
border is offscreen.  It even enforces this on resizes that come from the UI:
it's impossible to drag a window offscreen to the left and then make it wider
than the screen.

The Mac doesn't work like this.  On the Mac, windows can be arbitrarily sized. 
If you ask the system to give you SHRT_MAX x SHRT_MAX pixels, you'll get it,
regardless of your screen or desktop size.  A user is free to make wide windows
from within the UI by dragging them offscreen and expanding them.

Gecko on Windows is more than willing to let the window be obscured by the
taskbar, or to allow windows not positioned at 0,0 to have their bottom and
right edges pushed offscreen.  WinIE takes the window origin into account and
won't allow JS resizes to push the window's bottom or right edges offscreen or
under the taskbar.  It also prevents moves from pushing the window offscreen or
under the taskbar; they're snapped back to the closest on-screen position to the
target given the current size.  Note that WinIE enforces this behavior even when
the window is already partially offscreen.

So do we have a security problem on Windows here too?  You betcha.  Considering
the way bug 309251 seems to be heading, the severity is about to get more
critical here.

Comment 48

[Mark Mentovai]

Here's my plan:

For 1.8, I'd like to fix this on the Mac by crimping all content and chrome
resizes to the desktop dimensions without taking its origin into account.  I can
handle this entirely in widget, without changing any public interfaces.

For 1.9, I'd like to fix moves and resizes on all platforms by separating the
various classes of callers.  I do think that allowing resizes and moves from
content to push the window offscreen is a security flaw, especially if
move/resize will be turned back on by default, but it's the security group's call.

I'd like my proposed 1.9 fix for 1.8, but the issue doesn't seem to have gotten
any attention so far.

Comment 49

[Mark Mentovai]

Attachment: Safe fix, v.N

This is the plan for 1.8.  It's mostly the same as my previous patches, with a
couple of tweaks.  No checking is applied to resizes that come from the UI. 
For other resizes, the maximum size is the size of the desktop.  If the window
is already taller or wider than the desktop, then the window's larger dimension
is used instead, so resizes won't force windows to shrink.

Assuming the user hasn't manually resized a window to be larger than the
desktop, this patch always results in windows that can be dragged to fit
entirely on the desktop without being obscured by the dock.

There's one gotcha with this patch: if there are multiple screens but the
window is only on one of them, then its home screen is treated as the desktop. 
If it's not positioned at the top left of its screen, then one resize to max
would push it off of its screen (so that it has the dimensions of its former
home screen) and on to another, so a subsequent resize to max will cause it to
be resized so that it can fill all screens.  This is what we get for not being
able to take the origin into account.  There's no clean way around this without
implementing my 1.9 plan.  Even so, in no case will a window grow so large that
it won't fit on the desktop or will be obscured.

If there's another platform-agnostic bug on how window resizing is broken in
general, could someone please Cc me?  I haven't found any, but it may be marked
security too.

Comment 50

[Mark Mentovai]

Comment on attachment 196952 [details] [diff] [review]
Safe fix, v.N

I'll put a better comments in around the preprocessor conditional:

+#if 1
+      // Use the desktop's dimensions as the maximum size without taking
+      // the window's position into account.
+      //
[...]
+#else
+      // Consider the window's position when computing the maximum size.
+      // The goal is to use this on resizes that come from content, but
+      // it's currently not possible to distinguish between resizes that
+      // come from chrome and content.
+      //

Comment 51

[Asa Dotzler [:asa]]

mano and smfr, time is getting really short for 1.8b5. Can you guys help out
with some reviews here?

Comment 52

[Asaf Romano (gone)]

Comment on attachment 196952 [details] [diff] [review]
Safe fix, v.N

Index: mozilla/widget/src/mac/nsMacEventHandler.cpp
===================================================================

-	mTopLevelWidget->Resize(macRect.right - macRect.left, macRect.bottom -
macRect.top, PR_FALSE);
+	mTopLevelWidget->Resize(macRect.right - macRect.left, macRect.bottom -
macRect.top, PR_FALSE, PR_TRUE); // resize came from the UI via an event

this line is a bit too long


Index: mozilla/widget/src/mac/nsMacWindow.cpp
===================================================================

+NS_IMETHODIMP nsMacWindow::Resize(PRInt32 aWidth, PRInt32 aHeight, PRBool
aRepaint, PRBool aFromUI)

make it nsresult.


+// GetDesktopRect
+//
+// Determines a suitable desktop size for this window.  The desktop
+// size can be used to restrict resizes and moves.
+//
+// If a window is already lies on a single screen (possibly with portions

is already lies?


+nsresult nsMacWindow::GetDesktopRect(Rect* desktopRect) {
...
+    if (::EmptyRect(&screenRect)) {
+      NS_WARNING("Couldn't determine screen dimensions");

continue to the next screen here.


r=mano with those fixed.

Comment 53

[Simon Fraser [no longer active]]

Comment on attachment 196952 [details] [diff] [review]
Safe fix, v.N

> Index: mozilla/widget/src/mac/nsMacWindow.cpp
> ===================================================================

> +
> +#if 1
> +      // The maximum window dimensions are the desktop area, but if the
> +      // window is already larger than the desktop, don't force it to

Add a comment explaining the #if 1, and when/if it will be changed.

> +nsresult nsMacWindow::GetDesktopRect(Rect* desktopRect) {

Opening brace on new line please.

> +  // This is how to figure out where the dock and menu bar are, in global
> +  // coordinates, when there seems to be no way to get that information
> +  // directly from the system.  Compare the real dimensions of each screen
> +  // with the dimensions of the screen less the space used for the dock (as
> +  // given by GetAvailableWindowPositioningBounds) and apply deductive
> +  // reasoning.  While doing that, it's a convenient time to add each
> +  // screen's full rect to the union in desktopRect.
> +
> +  ::SetRect(desktopRect, 0, 0, 0, 0);
> +  Rect menuBarRect = {0, 0, 0, 0};
> +  Rect dockRect = {0, 0, 0, 0};
> +
> +  Rect windowRect;
> +  if (::GetWindowBounds(mWindowPtr, kWindowStructureRgn, &windowRect)
> +      != noErr) {
> +    NS_ERROR("::GetWindowBounds() didn't get window bounds");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  Rect tempRect; // for various short-lived purposes, such as holding
> +                 // unimportant overlap areas in ::SectRect calls
> +
> +  PRUint32 windowOnScreens = 0; // number of screens containing window
> +  Rect windowOnScreenRect = {0, 0, 0, 0}; // only valid if windowOnScreens==1
> +  GDHandle screen = ::DMGetFirstScreenDevice(TRUE);
> +  while (screen != NULL) {
> +    Rect screenRect = (*screen)->gdRect;
> +    if (::EmptyRect(&screenRect)) {
> +      NS_WARNING("Couldn't determine screen dimensions");
> +    }
> +
> +    ::UnionRect(desktopRect, &screenRect, desktopRect);
> +
> +    if(::SectRect(&screenRect, &windowRect, &tempRect)) {

if<space>(

> +      // The window is at least partially on this screen.  Set
> +      // windowOnScreenRect to the bounds of this screen.
> +      // If, after checking any other screens, the window is only on one
> +      // screen, then windowOnScreenRect will be used as the bounds,
> +      // limiting the window to the screen that contains it.
> +      windowOnScreens++;
> +      ::SetRect(&windowOnScreenRect, screenRect.left, screenRect.top,
> +                                     screenRect.right, screenRect.bottom);

Why not
    windowOnScreenRect = screenRect; ?


> +  if (windowOnScreens == 1) {
> +    // The window is contained entirely on a single screen.  It may actually
> +    // be partly offscreen, the key is that no part of the window is on a
> +    // screen other than the one with screen bounds in windowOnScreenRect.
> +    // Use that screen's bounds.
> +    //
> +    // It's tempting to save the screen's available positioning bounds and
> +    // return early here, but we still need to check to see if the window is
> +    // under the dock already, and permit it to remain there if so.
> +    //
> +    // Keep in mind that if the resize came from the UI as opposed to a
> +    // script, the window's dimensions will already appear to exceed the
> +    // screen it had formerly been contained within if the user has resized
> +    // it to cover multiple screens.  In other words, this won't constrain
> +    // UI resizes to a single screen.  That's perfect, because we only
> +    // want to police script resizing.
> +    ::SetRect(desktopRect, windowOnScreenRect.left, windowOnScreenRect.top,
> +                           windowOnScreenRect.right, windowOnScreenRect.bottom);

*desktopRect = windowOnScreenRect; ?

> +  // Use kWindowContentRgn to get past the title bar to the "meat."
> +  if (::GetWindowBounds(mWindowPtr, kWindowContentRgn, &windowRect)
> +      != noErr) {
> +    NS_ERROR("::GetWindowBounds() didn't get window bounds");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // Clip where appropriate.
> +
> +  if (::SectRect(desktopRect, &menuBarRect, &tempRect) &&
> +      menuBarRect.top == desktopRect->top) {
> +    // The menu bar is in the desktop rect at the very top.  Adjust the
> +    // top of the desktop to clip out the menu bar.
> +    // menuBarRect.bottom is the height of the menu bar because
> +    // menuBarRect.top is 0, so there's no need to subtract top
> +    // from bottom.  When using multiple monitors, the menu bar
> +    // might not be at the desktop top - in that case, don't clip it.
> +    desktopRect->top += menuBarRect.bottom;
> +  }
> +
> +  if (::SectRect(desktopRect, &dockRect, &tempRect)) {
> +    // There's a dock and it's somewhere in the desktop rect.
> +    // If the window already overlaps the dock, allow it to continue
> +    // overlapping the dock.  If the resize operation comes from the
> +    // user through the UI, windowRect will already be adjusted to the
> +    // desired bounds and will already overlap the dock if the system
> +    // allows it even if the original bounds did not, so the system's
> +    // resize constraints will be in effect.
> +
> +    // Determine the orientation of the dock before checking to see if
> +    // it's on an edge, so that the proper edge can be clipped.  If
> +    // the dock is on an edge corresponding to its orientation, clip
> +    // the dock out of the available desktop.

Couldn't you have saved the orientation from earlier?

sr=me with those issues examined.

Comment 54

[Mark Mentovai]

Attachment: Patch as checked in

This addresses all of the review comments.

Comment 55

[Mark Mentovai]

Fixed on trunk.

Also, from comment 49:
> If there's another platform-agnostic bug on how window resizing is broken in
> general, could someone please Cc me?  I haven't found any, but it may be marked
> security too.

Comment 56

[Mark Mentovai]

fixed1.8, will open a new bug to address content resizes on all (tier 1) platforms.