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.

Status: RESOLVED FIXED

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

Description

[Ieuan]

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 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Anybody else suspect bug 602580?

Comment 2

[Alice0775 White]

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 [:bzbarsky]]

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 [:bzbarsky]]

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

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 [:bzbarsky]]

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

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

Comment 8

[Timothy Nikkel (:tnikkel)]

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

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

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

(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 [:bzbarsky]]

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

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 [:bzbarsky]]

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 [:bzbarsky]]

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

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

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 [:bzbarsky]]

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

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: 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)]

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

(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] (inactive)]

Attachment: 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]

Bug 801341 also cause by Bug 602580.

And we have patch.
This should fix asap.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Attachment: unbitrotted patch

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

Comment 25

[Timothy Nikkel (:tnikkel)]

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

Comment 26

[Timothy Nikkel (:tnikkel)]

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

Comment 27

[Timothy Nikkel (:tnikkel)]

Attachment: flush all pending notifications

Oops, uploaded the wrong patch before.

Comment 28

[Yaoping Wang]

Any update? 20.0.1 still has the problem.

Comment 29

[Yaoping Wang]

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

Attachment: flush delayed resize

Comment 31

[Timothy Nikkel (:tnikkel)]

Attachment: look at the docshell size or prescontext visible area depending on if the visible area is overridden

Comment 32

[Timothy Nikkel (:tnikkel)]

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

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 [:bzbarsky]]

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

If this is green, r=me

Comment 35

[Timothy Nikkel (:tnikkel)]

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

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

Comment 37

[Jeremie Patonnier :Jeremie]

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