Closed Bug 641188 Opened 10 years ago Closed 8 years ago
Height and inner Width are both initially set to 10 in a framed page or iframe when it is first loaded in a new tab .
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.
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb629ae54510&tochange=b0c6a324e72f Anybody else suspect bug 602580?
Assignee: nobody → tnikkel
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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. :(
blocking2.0: --- → ?
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.
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.
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.
Oh yeah, you're right. Big oops on me.
It seems to be just the delayed resize cases that are affected.
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?
(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.)
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...
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.
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...
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.
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.
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).
blocking2.0: .x+ → ?
Where did you add the flush? And what was the failure?
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.
Attachment #521293 - Attachment is patch: false
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.
(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.
blocking2.0: ? → -
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.)
Bug 801341 also cause by Bug 602580. And we have patch. This should fix asap.
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.
This still fails the same test. I probably don't have time to look into the failure.
Attachment #528002 - Attachment is obsolete: true
But looks like it fails less tests and fails in a different way.
Here is the try run https://tbpl.mozilla.org/?tree=Try&rev=ef3d09293ac9
Oops, uploaded the wrong patch before.
Attachment #674127 - Attachment is obsolete: true
Any update? 20.0.1 still has the problem.
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.
Attachment #674270 - Attachment description: unbitrotted patch → flush all pending notifications
Attachment #521293 - Attachment is obsolete: true
Attachment #674270 - Attachment is obsolete: true
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 on attachment 761148 [details] [diff] [review] flush delayed resize Do you remember this stuff Boris? Feel free to suggest another reviewer.
Attachment #761148 - Flags: review?(bzbarsky)
Comment on attachment 761148 [details] [diff] [review] flush delayed resize If this is green, r=me
Attachment #761148 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaea6b4c8f79 This definitely needs a test, I just didn't write one yet.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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
You need to log in before you can comment on or make changes to this bug.