413277 - window.resizeBy(0, 1000000) throws and asserts

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Allow scripts to resize windows.  (Preferences > Content > Advanced)
2. Load the testcase.

Expected: The window should be resized to the maximum height allowed.

Result: NS_ERROR_FAILURE is thrown, and the window is not resized at all.  The following assertion appears in the console:

###!!! ASSERTION: Requested Cocoa window height of 1000593 is too much, max allowed is 100000
: 'Error', file /Users/jruderman/trunk/mozilla/widget/src/cocoa/nsCocoaWindow.mm, line 189

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Patch (v1.0)

This patch solves the problem by removing the 'WindowSizeAllowed' check on resize (which limits the maximum window size to 100000x100000) and instead bounds the window to the current screen (using 'FitToScreen').

Comment 3

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Patch (v1.1)

Using [mainScreen visibleFrame] instead of [mainScreen frame] (to prevent small glitches when resizing past visible frame)

Comment 4

[Josh Aas]

Comment on attachment 478873 [details] [diff] [review]
Resize Patch (v1.1)

>+static void FitToScreen(nsIntRect &aRect)

This function name is too vague. Call it "FitRectToVisibleAreaForScreen" or something like that.

>+  NSScreen *mainScreen = [NSScreen mainScreen];

Shouldn't we use the window's screen? The resize constraints will seem arbitrary on a screen that is not the main screen. Maybe make the fit function also take a screen argument and pass "[mWindow screen]".

>+  nsIntRect mNewBounds = nsIntRect(aX, aY, aWidth, aHeight);

The "m" prefix on variables means "member" - it's used to denote member variables for a class. Don't use it for stack-based variables. Use the name "newBounds" here instead.

I think you've got the right idea in general, thanks.

Comment 5

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Patch (v1.2)

Added above fixes

Comment 6

[Josh Aas]

Comment on attachment 479120 [details] [diff] [review]
Resize Patch (v1.2)

Looks good, lets get a second opinion from mstange though.

Comment 7

[Markus Stange [:mstange]]

What's the reason for clamping to the screen size instead of the old 100000x100000 size? I sometimes make the window wider than the screen to test drawing performance, and I don't know why we should disallow it.

In any case, you should use the same numbers in all places where we used WindowSizeAllowed before, i.e. also when creating the window. Then WindowSizeAllowed can be removed.

FitRectToVisibleAreaForScreen doesn't need exception guards. It's only called from Objective C++ code which can deal with the exceptions and already protects them from pure C++ code at the relevant entry points. They're slow (see bug 516046 or bug 513493 comment 23) so we shouldn't add them when not necessary.

Comment 8

[Scott Greenlay [:sgreenlay]]

The reason for clamping the screen size on resizeBy is because some developers (especially those who make unwanted pop-ups) use window.resizeBy(1000000,1000000) in order to make a window fill the users' screen. This does not affect the user simply dragging the window to make it larger then their current screen.

Bounding to the current screen is also done by Safari.

If we want to keep the current functionality, then we could just cap any resize at the 100000x100000 size.

Comment 9

[Markus Stange [:mstange]]

(In reply to comment #8)
> This does not affect the user simply dragging the window to make it
> larger then their current screen.

Ah, I see. At least not with the native window resizer. (It would affect it if we were using XUL resizers, which call into nsIWidget::Resize, but we don't use them on Mac, so yeah.)

> If we want to keep the current functionality, then we could just cap any resize
> at the 100000x100000 size.

I'd prefer that.

Comment 10

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Patch (v1.3)

Comment 11

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Patch Maximum (v1.0)

This is the same as the previous patch, but uses the maximum size to bound the window, not the size of the current screen.

Comment 12

[Scott Greenlay [:sgreenlay]]

From a users' standpoint I would prefer to bound to the current screen (because I can't think of any instance where I would want a window to resize itself outside of my current screen), but from a testing perspective I see the benefit of being able to create large windows through JS.

Let me know what solution you want.

Comment 13

[Markus Stange [:mstange]]

(In reply to comment #8)
> The reason for clamping the screen size on resizeBy is because some developers
> (especially those who make unwanted pop-ups) use
> window.resizeBy(1000000,1000000) in order to make a window fill the users'
> screen.

I've changed my mind - this is a very persuasive argument for clamping to the screen size. We should do that.

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 479538 [details] [diff] [review]
Resize Patch (v1.3)

 NS_IMETHODIMP nsCocoaWindow::Resize(PRInt32 aWidth, PRInt32 aHeight, PRBool aRepaint)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
-
-  if (!WindowSizeAllowed(aWidth, aHeight))
-    return NS_ERROR_FAILURE;
-
+  

Don't add these two spaces.

Comment 15

[Josh Aas]

Comment on attachment 479538 [details] [diff] [review]
Resize Patch (v1.3)

Looks good, thanks.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

I have no idea what the other widget implementations do, but has any thought been given to trying to get consistent window size clamping behavior across the three main ones, if possible? Worth a followup bug, perhaps?

Comment 17

[Josh Aas]

Comment on attachment 479538 [details] [diff] [review]
Resize Patch (v1.3)

This shouldn't block but it is a nice improvement for 2.0. We should take it.

Comment 18

[Josh Aas]

We should be able to write a test for this behavior as well.

Comment 19

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Test Patch (v1.0)

Turned on for all platforms right now, will disable those that don't pass after tryserver run.

Comment 20

[Scott Greenlay [:sgreenlay]]

Test fails on linux, filed as bug 604789.

Comment 21

[Josh Aas]

Comment on attachment 483583 [details] [diff] [review]
Resize Test Patch (v1.0)

r+, please combine this test with your fix and have the test disabled on Windows and Linux since it doesn't pass there.

Comment 22

[Scott Greenlay [:sgreenlay]]

Test fails on windows, filed as bug 605178.

Comment 23

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Patch (v2.0) w/test

Comment 24

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/a2aaf00c4acc

Comment 25

[Josh Aas]

backed out due to test failures

http://hg.mozilla.org/mozilla-central/rev/4ebc35e7695a
http://hg.mozilla.org/mozilla-central/rev/9b7e1b761cb7

Comment 26

[Scott Greenlay [:sgreenlay]]

Attachment: Resize Patch (v2.1) w/test

Comment 27

[Josh Aas]

pushed to mozilla-central again

http://hg.mozilla.org/mozilla-central/rev/0fed77757e88

Comment 28

[Timothy Nikkel (:tnikkel)]

Do reftests on mac still need to create windows bigger than the screen size?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, we fixed the screen size instead.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although it would still be nice to be able to create windows bigger than the screen size, for users running reftests on their own machines with small screen sizes.

Comment 31

[John Hawkinson]

This fix causes Mozilla windows to "disappear" offscreen (when the windows are created/instantiated) on multi-screened displays). Please see bug 644733.