Last Comment Bug 641188 - innerHeight and innerWidth are both initially set to 10 in a framed page or iframe when it is first loaded in a new tab.
: innerHeight and innerWidth are both initially set to 10 in a framed page or i...
Status: RESOLVED FIXED
: dev-doc-complete, regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla24
Assigned To: Timothy Nikkel (:tnikkel)
:
Mentors:
data:text/html;charset=utf-8,%3C!DOCT...
Depends on:
Blocks: 602580
  Show dependency treegraph
 
Reported: 2011-03-12 01:08 PST by Ieuan
Modified: 2013-06-20 02:12 PDT (History)
19 users (show)
tnikkel: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
-
-


Attachments
failure log (9.35 KB, text/plain)
2011-03-23 13:38 PDT, Timothy Nikkel (:tnikkel)
no flags Details
patch v1 (7.84 KB, patch)
2011-04-24 07:54 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
unbitrotted patch (8.56 KB, patch)
2012-10-22 22:59 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Review
flush all pending notifications (8.93 KB, patch)
2012-10-23 10:22 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Review
flush delayed resize (1.76 KB, patch)
2013-06-11 13:39 PDT, Timothy Nikkel (:tnikkel)
bzbarsky: review+
Details | Diff | Review
look at the docshell size or prescontext visible area depending on if the visible area is overridden (2.76 KB, patch)
2013-06-11 13:41 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Review

Description Ieuan 2011-03-12 01:08:36 PST
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre

innerHeight and innerWidth are both initially set to 10 in a framed page or iframe when it is first loaded into a new tab.
After body has loaded, innerHeight and innerWidth report correctly.


Reproducible: Always

Steps to Reproduce:
1. Create a page displaying innerHeight and innerWidth in an alert box.(page1)

2. Create a page to load page 1 in a frame or iframe.(page2)
3. Create a page with a link to page 2. (page3)
4. Open page 3 in Firefox.
5. Right click the link and open it in a new tab.

Actual Results:  
innerHeight = 10
innerWidth = 10

Expected Results:  
innerHeight = your viewports height
innerWidth = your viewports width

This bug was introduced in firefox4.0b9pre (2010-12-22).
Works correctly in firefox4.0b9pre (2010-12-21) and prior versions.
Comment 2 Alice0775 White 2011-03-13 06:43:55 PDT
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/e5ed12d16160
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221135104
Fails
http://hg.mozilla.org/mozilla-central/rev/a2a3a6e8b0e0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221145009
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e5ed12d16160&tochange=a2a3a6e8b0e0

In localbuild,
build from db27d37d7879 : fail
build from 77956e4dd8f8 : work
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-13 13:09:41 PDT
GAH.  The patch in bug 602580 is just totally wrong.  It's returning effectively uninitialized values.

This affects behavior in desktop Firefox in all sorts of cases: newly-opened tabs, iframes, etc.  This may well be worth respinning for.  :(
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-13 13:12:23 PDT
In particular, nothing in GetInner* actually makes sure the _prescontext's_ size is up to date.

On the other hand, it looks like we propagate that information sync in some cases, so it might only be the "delayed resize" cases that are affected...  So only iframes in background tabs, not all iframes; that sort of thing.
Comment 5 Timothy Nikkel (:tnikkel) 2011-03-13 13:14:58 PDT
So the problem is that nsDocShell::GetSize flushes, and 602580 removed that call with something that doesn't flush. I thought that EnsureSizeUpToDate was doing the same thing so there was no need for another flush, but EnsureSizeUpToDate flushes the parent.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-13 13:16:59 PDT
nsDocShell::GetSize also only flushes the parent, fwiw.  The point is that flushing the parent updates the _docshell_'s size.  It doesn't synchronously update the prescontext's size, for hidden things.
Comment 7 Timothy Nikkel (:tnikkel) 2011-03-13 13:19:52 PDT
Oh yeah, you're right. Big oops on me.
Comment 8 Timothy Nikkel (:tnikkel) 2011-03-13 13:31:19 PDT
It seems to be just the delayed resize cases that are affected.
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-13 15:36:16 PDT
bz, given comment 8 and the fact that we've shipped 8 betas with this bug (the patch in question has been in Firefox 4 since Beta 4!) do you think this requires a respin, or is a high priority .x fix?
Comment 10 Matt Brubeck (:mbrubeck) 2011-03-13 17:39:54 PDT
(In reply to comment #9)
> bz, given comment 8 and the fact that we've shipped 8 betas with this bug (the
> patch in question has been in Firefox 4 since Beta 4!) do you think this
> requires a respin, or is a high priority .x fix?

Minor correction - the patch has been in since *Fennec* 4.0b4, or Firefox 4.0b9.  (It landed on 2010-12-21, and shipped in 4 desktop betas.)
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-13 19:18:49 PDT
Mike, since this only affects tabs opened in the background (and any subframes in them), we can probably live with a .x fix.  Worst case the layout of a page you open in a background tab is completely busted, so you reload and it looks ok...

If we do respin, I think we should take this as a ridealong.

Timothy, would calling FlushDelayedResize(PR_FALSE) before looking at the prescontext size do the trick here?  The important part there being whether something will come along and do the resize reflow later...
Comment 12 Timothy Nikkel (:tnikkel) 2011-03-13 23:08:18 PDT
I was thinking check if we are using a custom viewport on the presshell, and if so do a flush layout and then get the visible area from the prescontext. If no custom viewport is being used then get the value from the docshell like we used to do. This fails one panorama test (browser/base/content/test/tabview/browser_tabview_bug594958.js) that was checked in after bug 602580, so I'm thinking the test might be broken. I'm continuing to work on this.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-14 06:18:11 PDT
That gives a noticeable performance penalty on some scripts when using a custom viewport.  We can do that if we have to, but it would really be better to avoid the self-flush if at all possible...
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-14 06:22:04 PDT
Ah, and indeed if you FlushDelayedResize(PR_FALSE) that will update the prescontext size but not clear mDelayedResize on the viewmanager or resize the root view, I think, so the resize reflow will happen when the resize is flushed PR_TRUE.  Worth double-checking all that, though.
Comment 15 Timothy Nikkel (:tnikkel) 2011-03-14 11:49:17 PDT
Ok. Either way the difference between getting the inner properties from the docshell or the prescontext seems to be causing differences in browser/base/content/test/tabview/browser_tabview_bug594958.js that I need to understand.
Comment 16 Timothy Nikkel (:tnikkel) 2011-03-23 13:00:13 PDT
browser_tabview_bug594958.js must be wrong because adding a flush to the test makes it fail without any other changes. So we either need to disable the failing parts of that test or fix that test (or its related patch).
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-23 13:29:19 PDT
Where did you add the flush?  And what was the failure?
Comment 18 Timothy Nikkel (:tnikkel) 2011-03-23 13:38:59 PDT
Created attachment 521293 [details]
failure log

I added the flush just after the resizeBy call here http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/tabview/browser_tabview_bug594958.js#110 so that the size change gets flushed to the prescontext.

This is a log of the failures and a dump of the canvases the test is looking at. I also made it dump the same canvases in the "passing" case. Looking at the first image it seems the "failed" case is the correct image for the document mentioned.
Comment 19 Timothy Nikkel (:tnikkel) 2011-03-23 16:58:00 PDT
So four of the five failures are due to bug 625561: it changed the expected pixels values when it removed a flush in the tabview thumbnail generation code without any reason stated. So I'm concluding that this was a "just make the test pass" change.

The other failure is a little more interesting.
Comment 20 Daniel Veditz [:dveditz] 2011-04-08 10:41:29 PDT
(In reply to comment #11)
> If we do respin, I think we should take this as a ridealong.

That was 4 weeks ago and still no patch -- how was this ever going to make it as a ridealong? Doesn't seem realistic for Macaw at this point; please renominate if that's wrong.
Comment 21 Tim Taubert [:ttaubert] 2011-04-24 07:54:04 PDT
Created attachment 528002 [details] [diff] [review]
patch v1

I attached a patch that corrects browser_tabview_bug594958.js. WFM when running mochitests locally. The patch is extracted from bug 633096 so winner is the bug that lands first :)

Included: Timothy's changes he sent me by mail.

(I'm sorry that it took so long to respond.)
Comment 22 Alice0775 White 2012-10-21 09:39:08 PDT
Bug 801341 also cause by Bug 602580.

And we have patch.
This should fix asap.
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-22 15:16:04 PDT
There's no urgency here to push in a fix for something that's been in product for this long, we should confirm if bug 801341 is showing up in nightly/aurora/beta as well as 16, since we won't respin 16 for this if it's only (for some strange reason) affecting that version.  I'd consider an uplift nomination once there's a patch landed on trunk that is verified to fix this, but we wouldn't block a release on this long standing bug.
Comment 24 Timothy Nikkel (:tnikkel) 2012-10-22 22:59:29 PDT
Created attachment 674127 [details] [diff] [review]
unbitrotted patch

This still fails the same test. I probably don't have time to look into the failure.
Comment 25 Timothy Nikkel (:tnikkel) 2012-10-22 23:38:55 PDT
But looks like it fails less tests and fails in a different way.
Comment 26 Timothy Nikkel (:tnikkel) 2012-10-23 00:27:18 PDT
Here is the try run
https://tbpl.mozilla.org/?tree=Try&rev=ef3d09293ac9
Comment 27 Timothy Nikkel (:tnikkel) 2012-10-23 10:22:05 PDT
Created attachment 674270 [details] [diff] [review]
flush all pending notifications

Oops, uploaded the wrong patch before.
Comment 28 Yaoping Wang 2013-05-09 00:26:11 PDT
Any update? 20.0.1 still has the problem.
Comment 29 Yaoping Wang 2013-05-14 18:43:01 PDT
page1 has a link to page2
page2 has svg which contains script to use innerWidth/innerHeight
open page2 through the link to page1 in new tabs in fast succession, sometimes the innerWidth/innerHeight is 10/10.
Comment 30 Timothy Nikkel (:tnikkel) 2013-06-11 13:39:53 PDT
Created attachment 761148 [details] [diff] [review]
flush delayed resize
Comment 31 Timothy Nikkel (:tnikkel) 2013-06-11 13:41:21 PDT
Created attachment 761151 [details] [diff] [review]
look at the docshell size or prescontext visible area depending on if the visible area is overridden
Comment 32 Timothy Nikkel (:tnikkel) 2013-06-11 14:53:43 PDT
We have two potential approaches here.

1) flush any delayed resizes queued in the view subsystem and get the inner width/height from the prescontext visible area
2) get the inner width/height from the prescontext visible area if the viewport is overridden (it will always be up to date if overridden, there are no delayed resizes), or the docshell if not.

1) should be green.
2) fails in test_resize_move_windows.html, but that is because of bug 871434. We can trivially modify test_resize_move_windows.html so that it is not exposed to this problem (it is a test that is intended to test general functionality and not this specific case).
Comment 33 Timothy Nikkel (:tnikkel) 2013-06-11 14:55:30 PDT
Comment on attachment 761148 [details] [diff] [review]
flush delayed resize

Do you remember this stuff Boris? Feel free to suggest another reviewer.
Comment 34 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-06-11 19:54:35 PDT
Comment on attachment 761148 [details] [diff] [review]
flush delayed resize

If this is green, r=me
Comment 35 Timothy Nikkel (:tnikkel) 2013-06-12 08:27:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaea6b4c8f79

This definitely needs a test, I just didn't write one yet.
Comment 36 Ed Morley [:emorley] 2013-06-13 02:39:07 PDT
https://hg.mozilla.org/mozilla-central/rev/aaea6b4c8f79
Comment 37 Jeremie Patonnier :Jeremie 2013-06-20 02:12:51 PDT
Add a note about this bug on the MDN documentation:

https://developer.mozilla.org/en-US/docs/Web/API/window.innerWidth
https://developer.mozilla.org/en-US/docs/Web/API/window.innerHeight

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