Ieuan Reporter
	Description • 13 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 13 years ago

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb629ae54510&tochange=b0c6a324e72f

Anybody else suspect bug 602580?

	Alice0775 White
	Comment 2 • 13 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 13 years ago

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.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 13 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 5 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 13 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 7 • 13 years ago

Oh yeah, you're right. Big oops on me.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 8 • 13 years ago

It seems to be just the delayed resize cases that are affected.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 9 • 13 years ago

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?

	Matt Brubeck (:mbrubeck)
	Comment 10 • 13 years ago

(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.)

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 13 years ago

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...

	Timothy Nikkel (:tnikkel) Assignee
	Comment 12 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 13 years ago

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...

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 13 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 15 • 13 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 16 • 13 years ago

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).

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 13 years ago

Where did you add the flush?  And what was the failure?

	Timothy Nikkel (:tnikkel) Assignee
	Comment 18 • 13 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 19 • 13 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 20 • 13 years ago

(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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 21 • 13 years ago

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.)

	Alice0775 White
	Comment 22 • 12 years ago

Bug 801341 also cause by Bug 602580.

And we have patch.
This should fix asap.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 23 • 12 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 24 • 12 years ago

This still fails the same test. I probably don't have time to look into the failure.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 25 • 12 years ago

But looks like it fails less tests and fails in a different way.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 26 • 12 years ago

Here is the try run
https://tbpl.mozilla.org/?tree=Try&rev=ef3d09293ac9

	Timothy Nikkel (:tnikkel) Assignee
	Comment 27 • 12 years ago

Oops, uploaded the wrong patch before.

	Yaoping Wang
	Comment 28 • 11 years ago

Any update? 20.0.1 still has the problem.

	Yaoping Wang
	Comment 29 • 11 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 32 • 11 years ago

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).

	Timothy Nikkel (:tnikkel) Assignee
	Comment 33 • 11 years ago

Comment on attachment 761148 [details] [diff] [review]
flush delayed resize

Do you remember this stuff Boris? Feel free to suggest another reviewer.

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 11 years ago

Comment on attachment 761148 [details] [diff] [review]
flush delayed resize

If this is green, r=me

	Timothy Nikkel (:tnikkel) Assignee
	Comment 35 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaea6b4c8f79

This definitely needs a test, I just didn't write one yet.

	Ed Morley [:emorley]
	Comment 36 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/aaea6b4c8f79

	Jeremie Patonnier :Jeremie
	Comment 37 • 11 years ago

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