Closed Bug 764240 Opened 9 years ago Closed 8 years ago

window.sizeToContent() is not clamped to prevent ridiculously small windows


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: jruderman, Assigned: mounir)


(Depends on 1 open bug)


(Keywords: csectype-spoof, dev-doc-complete, sec-low)


(3 files)

1. Load the testcase.
2. Click button #1: "Open an empty window"
3. Click button #5: "w.sizeToContent();"

Result: window is extremely tiny. (Possible security implications: full-screen button covers the close button on Mac; window could "hide"; maybe spoofing or clickjacking.)

Expected: window becomes 100x100 or so, like button #3 does.

Strangely, it is clamped correctly at the upper end (you can see this with buttons #2, #5, and #4.)
This is moot if you haven't checked "Allow scripts to: Move or resize popup windows".
Is that MocOS only?
OS: Mac OS X → All
Hardware: x86_64 → All
Attached patch Patch v1Splinter Review
Assignee: nobody → mounir
Attachment #636241 - Flags: review?(bzbarsky)
Comment on attachment 636241 [details] [diff] [review]
Patch v1

r=me if you fix the comment in the IDL to reflect reality.
Attachment #636241 - Flags: review?(bzbarsky) → review+
Mounir, anything preventing landing the patch (with the comment fixed)?
IIRC, I think the patch was failing some tests. I will send it again to try and see.
This is breaking layout/base/tests/test_bug458898.html

Locally, if I open a page with a 100/200 div, sizeToContent() produce a size that has scrollbars while, before, it wouldn't.


I will try to have a quick look at it later but I unfortunately doesn't have much cycles to dedicate :(
Olli, feel free to take this review from Boris ;)

Locally, it is fixing the test breakage.
Attachment #682458 - Flags: review?(bzbarsky)
Hmm.  So why is the new code better than the old?
(In reply to Boris Zbarsky (:bz) from comment #9)
> Hmm.  So why is the new code better than the old?

My original patch was changing the behaviour of the method.
nsIMarkupDocumentViewer.sizeToContent() was using nsIDocShellTreeOwner.sizeShellTo() to set the size and my patch was using nsIBaseWindow.setSize() because it was used in most sizing methods in nsGlobalWindow. I was assuming those two methods would likely do the same thing but obviously they are not. Actually, reading more the code, I realise SetInner{Width,Height} are using nsGlobalWindow::SetDocShellWidthAndHeight(), which I could use. This is actually the behiaiour sizeToContent() should have: the inner{Width,Height} should be changed, not the outer. I would assume nsIBaseWindow.setSize() does change the outher size.
Comment on attachment 682458 [details] [diff] [review]
Fix test breakage

Ah, yes.  The basewindow thing off the treeowner would set the outer size, indeed.

r=me if you use SetDocShellWidthAndHeight.
Attachment #682458 - Flags: review?(bzbarsky) → review+
(this is a test + a reminder to fix the Windows errors)
Flags: needinfo?(mounir)
(this is a test + a reminder to fix the Windows errors)

(and now, my bugzilla dashboard can show me the pending "needinfo" ;))
Flags: needinfo?(mounir) → in-testsuite+
Target Milestone: --- → mozilla19
Backed out in because the test timed out on Android.
Target Milestone: mozilla19 → ---
Blocks: 813271
Relanded with the test disabled on Android:

there is no reason to have the test enabled on Android given that Firefox Android opens a tab everytime a popup is opened.
Target Milestone: --- → mozilla20
Closed: 8 years ago
Resolution: --- → FIXED
(In reply to Phil Ringnalda (:philor) from comment #15)
> Backed out in
> because
> the test timed out on Android.

Merge of backout:
Depends on: 871434
Keywords: csec-spoof
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.