Closed Bug 872324 Opened 6 years ago Closed 6 years ago

Restored maximized windows blank or incorrectly positioned

Categories

(Firefox :: Session Restore, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox21 --- unaffected
firefox22 + verified
firefox23 + verified
firefox24 + verified

People

(Reporter: bws42, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

When restoring a session with a maximized browser window, the window chrome does not properly update when the session is restored. A single blank tab is shown and none of the chrome appears functional.

If you restore and re-maximize the window then everything works as expected.

This is a regression caused by bug 864107
I can't seem to reproduce this on my Linux VM (with Ubuntu 12.04); I wonder if it's dependent on a particular window manager or desktop environment, or something like that.

However, in experimenting with restoring maximized windows, I did uncover a somewhat different issue that I can reproduce on OS X - in some cases, the maximized window gets restored to an incorrect position, offset down and right from where it should be (and hence partially off-screen). This is definitely a regression from bug 864107. I wonder whether it'll turn out that your issue has the same root cause, just ends up displaying different symptoms in your environment.

Anyhow, I've got a patch for -that- issue, and I have started a tryserver build at https://tbpl.mozilla.org/?tree=Try&rev=7f2a3ea97b13. When those builds are ready, if you could test and let me know whether it affects the issue you're seeing, that would be really helpful - thanks.
Looks like my unfamiliarity with JS caught me out here! The parameters to restoreDimensions are strings, so an expression like "aLeft + aWidth" ends up doing a string concatenation rather than (arithmetic) addition, which of course makes nonsense of the tests for whether the right and bottom edges of the window are off-screen. Explicitly converting them to Number before doing the addition fixes the problem. (An alternative is to move aWidth and aHeight to the other side of the comparisons and subtract them instead, but I think making the conversion explicit seems the clearest fix.)
Attachment #749775 - Flags: review?(ttaubert)
Assignee: nobody → jfkthame
Will, if you could test one of the tryserver builds from

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-7f2a3ea97b13/try-linux/ (32-bit)

or

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-7f2a3ea97b13/try-linux64/ (64-bit)

and let me know whether this shows the same behavior, it'd be much appreciated.
Flags: needinfo?(mozilla)
Jonathan, that change fixes it for me as well.

I'm using Gnome 2.30 with Compiz 0.8.4 on Debian Squeeze x64 with a resolution of 1920x1080.
Flags: needinfo?(mozilla)
Great - thanks for testing. Once this is reviewed and landed on central, we should take this fix on the same branches as bug 864107.
Summary: Restored maximized windows blank → Restored maximized windows blank or incorrectly positioned
Do you know why these are strings? I see no reason for these to be, and it seems like we should fix that underlying problem rather than papering over it for this specific use.
Ah, it looks like _getWindowDimension can return an attribute value (string). Seems like we should convert to a number there? Not sure what other implications that might have. Since we have to fix this on beta, your patch might be safest there.
Yes, _getWindowDimension is also used to retrieve non-numeric attributes, so we can't simply make it convert blindly.

A tidier approach might be to use _getWindowDimension only for attributes that are known to be numeric (and so we could make it responsible for returning them as numbers), and use a separate method for non-numeric ones such as sizemode. But that'd be more code churn, and especially for beta, it seemed sensible to do the minimally-invasive thing.

(Care to steal the review? I see Tim's away for a few days, apparently.)
Comment on attachment 749775 [details] [diff] [review]
ensure we use arithmetic (not string concatenation) when calculating window edges

Review of attachment 749775 [details] [diff] [review]:
-----------------------------------------------------------------

If ss.restoreDimension() expects numbers the caller ss.restoreWindowFeatures() must be fixed. I think we should pass numbers right away using '+aWinData.width || 0' instead of 'aWinData.width || 0' and '"screenX" in aWinData ? +aWinData.screenX : NaN' instead of '"screenX" in aWinData ? aWinData.screenX : NaN' etc.
Attachment #749775 - Flags: review?(ttaubert)
OK, it also works to fix the issue here, ensuring the attributes are passed as numbers.
Attachment #752179 - Flags: review?(ttaubert)
Attachment #749775 - Attachment is obsolete: true
Comment on attachment 752179 [details] [diff] [review]
ensure window position/size attributes are passed to restoreDimensions as numbers, not strings

Review of attachment 752179 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #752179 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/1bfd6121bcf1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Verified fixed in http://hg.mozilla.org/mozilla-central/rev/00b264c7cced
Status: RESOLVED → VERIFIED
Blocks: 876244
Comment on attachment 752179 [details] [diff] [review]
ensure window position/size attributes are passed to restoreDimensions as numbers, not strings

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 864107

User impact if declined: windows reopened by Session Restore may be misplaced and/or blank until manually resized again if they were maximized at the time when the session was saved

Testing completed (on m-c, etc.): on m-c several days, confirmed fixed by reporter

Risk to taking this patch (and alternatives if risky): low risk: just ensures window-sizing attributes are handled by JS code as numbers, not strings, so that arithmetic is valid

String or IDL/UUID changes made by this patch: none
Attachment #752179 - Flags: approval-mozilla-beta?
Attachment #752179 - Flags: approval-mozilla-aurora?
Attachment #752179 - Flags: approval-mozilla-beta?
Attachment #752179 - Flags: approval-mozilla-beta+
Attachment #752179 - Flags: approval-mozilla-aurora?
Attachment #752179 - Flags: approval-mozilla-aurora+
I tried to reproduce this issue on a Firefox 22 beta 1 build (05/14) on Ubuntu 12.10 64bit and only got the following:
1. Create a session with several tabs open
2. Maximize the window, then close it
3. Reopen Firefox
4. Restore the previous session
Results: the title bar is not fully displayed until moving the mouse around on the browser. All the rest is restored properly.

This issue doesn't reproduce on the Firefox 22 beta 3 Ubuntu 64bit build (05/28).

Will, since you can reproduce this bug better, can you please try to also verify it on Firefox 22 (beta) and 23 (aurora)?
Flags: needinfo?(mozilla)
changing flag value per comment 14...
Verified broken in 22b2 and working in 22b3.

Broken:http://hg.mozilla.org/releases/mozilla-beta/1b5782f637b3 (22 beta 2)
Works: http://hg.mozilla.org/releases/mozilla-beta/51c78e9573f4 (22 beta 3)

Verified fixed in 23a1.

Broken:http://hg.mozilla.org/releases/mozilla-aurora/65bae28afb45
Works: http://hg.mozilla.org/releases/mozilla-aurora/e6a4948b6d9a
Flags: needinfo?(mozilla)
Marking as verified per comment 19. QA could not fully reproduce this issue locally, so we can't verify more.
You need to log in before you can comment on or make changes to this bug.