Closed Bug 413277 Opened 12 years ago Closed 10 years ago

window.resizeBy(0, 1000000) throws and asserts

Categories

(Core :: Widget: Cocoa, defect, minor)

x86
macOS
defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: sgreenlay)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 7 obsolete files)

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
Attached file testcase
Keywords: testcase
Assignee: joshmoz → nobody
Assignee: nobody → sgreenlay
Attached patch Resize Patch (v1.0) (obsolete) — Splinter Review
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').
Attachment #478822 - Flags: review?(joshmoz)
Attached patch Resize Patch (v1.1) (obsolete) — Splinter Review
Using [mainScreen visibleFrame] instead of [mainScreen frame] (to prevent small glitches when resizing past visible frame)
Attachment #478822 - Attachment is obsolete: true
Attachment #478873 - Flags: review?(joshmoz)
Attachment #478822 - Flags: review?(joshmoz)
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.
Attachment #478873 - Flags: review?(joshmoz) → review-
Attached patch Resize Patch (v1.2) (obsolete) — Splinter Review
Added above fixes
Attachment #478873 - Attachment is obsolete: true
Attachment #479120 - Flags: review?(joshmoz)
Comment on attachment 479120 [details] [diff] [review]
Resize Patch (v1.2)

Looks good, lets get a second opinion from mstange though.
Attachment #479120 - Flags: review?(mstange)
Attachment #479120 - Flags: review?(joshmoz)
Attachment #479120 - Flags: review+
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.
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.
(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.
Attached patch Resize Patch (v1.3) (obsolete) — Splinter Review
Attachment #479120 - Attachment is obsolete: true
Attachment #479538 - Flags: review?(joshmoz)
Attachment #479120 - Flags: review?(mstange)
Attached patch Resize Patch Maximum (v1.0) (obsolete) — Splinter Review
This is the same as the previous patch, but uses the maximum size to bound the window, not the size of the current screen.
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.
(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 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.
Attachment #479538 - Flags: review+
Comment on attachment 479538 [details] [diff] [review]
Resize Patch (v1.3)

Looks good, thanks.
Attachment #479538 - Flags: review?(joshmoz) → review+
Attachment #479541 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
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 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.
Attachment #479538 - Flags: approval2.0+
We should be able to write a test for this behavior as well.
Attached patch Resize Test Patch (v1.0) (obsolete) — Splinter Review
Turned on for all platforms right now, will disable those that don't pass after tryserver run.
Attachment #483583 - Flags: review?(joshmoz)
Test fails on linux, filed as bug 604789.
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.
Attachment #483583 - Flags: review?(joshmoz) → review+
Test fails on windows, filed as bug 605178.
Attached patch Resize Patch (v2.0) w/test (obsolete) — Splinter Review
Attachment #479538 - Attachment is obsolete: true
Attachment #483583 - Attachment is obsolete: true
Attachment #484086 - Flags: review?(joshmoz)
Attachment #484086 - Flags: review?(joshmoz) → review+
Attachment #484086 - Flags: approval2.0+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/a2aaf00c4acc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
backed out due to test failures

http://hg.mozilla.org/mozilla-central/rev/4ebc35e7695a
http://hg.mozilla.org/mozilla-central/rev/9b7e1b761cb7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #484086 - Attachment is obsolete: true
Attachment #485433 - Flags: review?(joshmoz)
Attachment #485433 - Flags: review?(joshmoz) → review+
pushed to mozilla-central again

http://hg.mozilla.org/mozilla-central/rev/0fed77757e88
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Do reftests on mac still need to create windows bigger than the screen size?
No, we fixed the screen size instead.
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.
Blocks: 609659
This fix causes Mozilla windows to "disappear" offscreen (when the windows are created/instantiated) on multi-screened displays). Please see bug 644733.
Depends on: 644733
You need to log in before you can comment on or make changes to this bug.