Closed Bug 465448 Opened 16 years ago Closed 15 years ago

sizeToContent summons new NS_ERROR_FAILURE exceptions

Categories

(Core :: Layout, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: erappleman, Assigned: roc)

References

(Blocks 1 open bug, )

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111616 Ubuntu/9.04 (jaunty) Firefox/3.0.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111616 Ubuntu/9.04 (jaunty) Firefox/3.0.4

The bug manifests itself differently across platforms and Firefox versions.

For example, almost all sizeToContent calls in Firefox 3.1 (all platforms) are not handled properly and cause rendering and/or redraw errors.

Examples:
https://bugzilla.mozilla.org/attachment.cgi?id=276818
https://bugzilla.mozilla.org/attachment.cgi?id=304326
https://bugzilla.mozilla.org/attachment.cgi?id=276997

However, when using a non-default theme (such as ifox graphite) on Firefox 3.0.x (all platforms EXCEPT Linux are affected), a sizeToContent call for a new nsIDOMWindow will create a dimensionless, 0 by 0 window that cannot be resized or closed.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
OS: Linux → All
Also, I hate to be a bother, but until I can find or make a testcase for the latter set of symptoms, here's the steps to replicate on a non-Linux installation of 3.0.x

1. Install the Text-to-Image addon.
2. Install and select iFox Graphite theme.
3. Restart Firefox.
4. Find an image URL: http://www.mozilla.com/img/tignish/home/feature-logo.png
5. Enable Text-to-Image by clicking once on the red hurricane icon in the status bar.
6. Double click on the image that has now appeared.
7. Observe the invisible, persistent "window" that results.

Note:

In Firefox 3.1 on ALL platforms, regardless of theme, the end result will be either a zero-dimension popup that can't be closed OR a popup that is only as tall as the window bar (observed on Linux).
(btw, before anyone complains, i am the author of text-to-image)
Flags: blocking-firefox3.1?
OS: All → Linux
Version: unspecified → 3.1 Branch
OS: Linux → All
I just realized I forgot the browser string for 3.1

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2) Gecko/20081128 Ubuntu/9.04 (jaunty) Shiretoko/3.1b2 ID:20081128200629
Breakpoint 1, DocumentViewerImpl::SizeToContent (this=0xac0bf6a0) at nsDocumentViewer.cpp:3137
3137 nsDocumentViewer.cpp: No such file or directory.
in nsDocumentViewer.cpp
Current language: auto; currently c++

Run till exit from #0 DocumentViewerImpl::SizeToContent (this=0xac0bf6a0) at nsDocumentViewer.cpp:3137
0xb704eb88 in nsGlobalWindow::SizeToContent (this=0xaafee330) at nsGlobalWindow.cpp:4546
4546 nsGlobalWindow.cpp: No such file or directory.
in nsGlobalWindow.cpp
Value returned is $1 = 2147500037
I'm splitting this bug into 2 as per a request.
--> Core::DOM
Component: General → DOM
Flags: blocking-firefox3.1?
Product: Firefox → Core
QA Contact: general → general
Version: 3.1 Branch → Trunk
Component: DOM → Layout
Flags: blocking1.9.1?
QA Contact: general → layout
(In reply to comment #5)
> I'm splitting this bug into 2 as per a request.

So what does this bug cover, and what's the other bug number?
Assignee: nobody → roc
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
The bug only seems to happen when you load one of those testcases as a content window. If you load them as top-level chrome windows, they work fine.

The bug seems to be that after one call to resizeToContent, in nsXULWindow::SizeShellTo shellAsWin->GetSize returns the wrong (old) size but this->GetSize returns the right size. For example if the sizeToContent-width is 50px and the window started at 400px, we get width==400, aCX=-50, winCX==50 so widthDelta==-350 and we call SetSize with a width of -300. Then all kinds of bad stuff happens.

But this could just be because nsDocumentViewer::SizeToContent isn't expecting a non-toplevel documents. The code looks like this:

   nsCOMPtr<nsIDocShellTreeItem> docShellParent;
   docShellAsItem->GetSameTypeParent(getter_AddRefs(docShellParent));

   // It's only valid to access this from a top frame.  Doesn't work from
   // sub-frames.
   NS_ENSURE_TRUE(!docShellParent, NS_ERROR_FAILURE);

Here the content document is the topmost content document so docShellParent is null. Should we be checking for *any* parent? Do we actually need sizeToContent to work in content subframes?
I'd say that sizeToContent in a non-chrome window should just be a no-op (just like all other window resizing methods, honestly).  I'd have no problems with making it so, personally.
This seems to have regressed between Firefox 2 and Firefox 3.

We do allow window resizing methods from content, by default. nsGlobalWindow::SizeToContent checks CanMoveResizeWindows so that can be disabled. I think we should probably fix this so it works.
OK, so the *real* reason this doesn't work is aptly described by this comment in SizeShellTo:

  // XXXTAB This is wrong, we should actually reflow based on the passed in
  // shell.  For now we are hacking and doing delta sizing.  This is bad
  // because it assumes all size we add will go to the shell which probably
  // won't happen.

It turns out that the toplevel window refuses to shrink its contents below a width of about 400px on my Mac. So here's what happens:
-- In the first sizeToContent call, we start with a 600px wide content area and window width and try to narrow the content area to 50px of size-to-content width. nsXULWindow::SizeShellTo computes a widthDelta of -550px and sets the window width to 50px.
-- However, because the toplevel window refuses to shrink below 400px, the content area gets a width of 400px.
-- In the next sizeToContent call, we start with a 400px wide content area and a 600px wide window and try to narrow the content area to 50px. So we compute a widthDelta of -350px and try to set the window width to -300px. This is of course a case where changing the window width does not flow through directly to the content area width.

So there are two bugs here:
1) A bug (that we've hit before) in the Firefox chrome where it refuses to let the content area shrink below some (largish) minimum width
2) As a fallback, nsXULWindow::SizeShellTo should at least not try to make the window smaller than the content area it's trying to achieve. Ideally it would compute the actual window size required assuming the content area has the fixed size-to-content size, but that does seem like more work than it's worth.
Attached patch fixSplinter Review
This seems worth doing, at least since there's a test here.
Attachment #354672 - Flags: superreview?(bzbarsky)
Attachment #354672 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
Blocks: 392417
Blocks: 439323
Comment on attachment 354672 [details] [diff] [review]
fix

r+sr=bzbarsky with s/try/trying/ and a followup bug filed to sort out the seamonkey/windows stuff.
Attachment #354672 - Flags: superreview?(bzbarsky)
Attachment #354672 - Flags: superreview+
Attachment #354672 - Flags: review?(bzbarsky)
Attachment #354672 - Flags: review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
So, is my follow up bug (https://bugzilla.mozilla.org/show_bug.cgi?id=467853) redundant?

It has more test cases than this one.

I'll repost them here for your convenience.

https://bugzilla.mozilla.org/attachment.cgi?id=276818
https://bugzilla.mozilla.org/attachment.cgi?id=304326
https://bugzilla.mozilla.org/attachment.cgi?id=276997
https://bugzilla.mozilla.org/attachment.cgi?id=349968 (this one causes complete destruction of the firefox window if you fiddle with the buttons)
(my mistake, the 4th testcase was the only new one.)
Blocks: 467853
one last thing, how can i check for when this fix makes it into a nightly or hourly?
Whiteboard: [needs review] → [needs landing]
Pushed 4e1a2c3bc40d.

The fix for this will be available in the next nightly build.
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Pushed 0de153e2246f to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
(In reply to comment #18)

Well, whilst SeaMonkey does (still) have bug 471959,

the test added by this bug does *not* fail, at least on 1.9.1:
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1231467839.1231473189.13721.gz
Linux comm-central dep unit test on 2009/01/08 18:23:59
*** 42442 ERROR TEST-UNEXPECTED-PASS | /tests/layout/base/tests/test_bug465448.xul | innerWidth
*** 42443 ERROR TEST-UNEXPECTED-PASS | /tests/layout/base/tests/test_bug465448.xul | innerHeight
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1231469596.1231476862.21343.gz
MacOSX 10.4 comm-central dep unit test on 2009/01/08 18:53:16
*** 42460 ERROR TEST-UNEXPECTED-PASS | /tests/layout/base/tests/test_bug465448.xul | innerWidth
*** 42461 ERROR TEST-UNEXPECTED-PASS | /tests/layout/base/tests/test_bug465448.xul | innerHeight
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1231469281.1231475027.17708.gz
Win2k3 comm-central dep unit test on 2009/01/08 18:48:01
*** 42466 ERROR TEST-UNEXPECTED-PASS | /tests/layout/base/tests/test_bug465448.xul | innerHeight

http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=0ce3af5127dc&tochange=0c1f894cd595
}

Could you remove the SeaMonkey workaround ?
Target Milestone: --- → mozilla1.9.1b3
Blocks: 472813
(In reply to comment #19)
> Could you remove the SeaMonkey workaround ?

KaiRo filed bug 472813...
You need to log in before you can comment on or make changes to this bug.