593307 - Close Window from minimized taskbar icon loses control of the application

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Marco Bonardo [:mak]]

Steps to reproduce:

1. open Firefox new profile, then open multiple tabs (any number that will cause the save session dialog to appear, from 2 on)
2. minimize browser to taskbar
3. right click on the minimized icon in taskbar and choose Close Window

Expected:

1. I get asked to save session, window closes and process goes away

What happens:

1. Nothing seems to happen
2. clicking on any preview in Aeropeek brings up a small empty window frame (small rectangle in upper part of the desktop)
3. No way to get back control of the application, have to kill the process

I was able to reproduce at any try, but checking with another guy on IRC he was unable to reproduce.
Since if this is reproduceable causes completely lose of application control, and forces a process kill, I'm filing it to get further confirmation of it working or broken.

Comment 1

[Marco Bonardo [:mak]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100902 Firefox/4.0b6pre

Comment 2

[Justin Dolske [:Dolske]]

i can reproduce this with a currently nightly on my Win 7 machine, with a new profile.

Comment 3

[Marco Bonardo [:mak]]

So, this seems to be related to the Save session modal dialog, if a user choices "never ask me again" the dialog does not come up and it works fine.

Supposing related to opening a modal dialog when app is minimized.

Comment 4

[Marco Bonardo [:mak]]

Asking blocking because forces user to kill the process, there is no other way to exit this situation.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Probably the same as bug 586299, a regression from bug 520805. The new window inherits the X/Y position of the parent window. The parent is minimized and probably has negative coordinates, and that puts the new window offscreen where it can't receive focus. I'll try to confirm this hypothesis.

Comment 7

[Luke Iliffe (Harlequin99)]

Regression Range on XP:
works: 2010-08-02	http://hg.mozilla.org/mozilla-central/rev/a4d86c3a3494
broken: 2010-08-03	http://hg.mozilla.org/mozilla-central/rev/4daa2ea5747b

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4d86c3a3494&tochange=4daa2ea5747b

Comment 9

[Matthias Versen [:Matti]]

bug 593307 is about a http basic auth popup. Minimize FF while the page is loading and before you get the dialog.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Constrain initial position to same screen

So this was actually a small widget bug (first part of the patch), but I think that other uncovered bugs might also not be prepared to handle a window created offscreen.

This happens easily after bug 520805 if the top-left corner of the opener window is offscreen and we inherit it via "centerscreen". I think it'd be a good idea to ensure that the X/Y inherited falls into the same screen as the opener.

Johnny, requesting feedback on the approach here. If you think that's a reasonable thing I can clean up that code into a separate function (if wanted).

Comment 12

[logos]

yeah, that's exactly the issue I reported there https://bugzilla.mozilla.org/show_bug.cgi?id=593307 ... been like this for a while, since beta 3 actually up to the latest minefield build.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 476873 [details] [diff] [review]
Constrain initial position to same screen

feedback+. You could move left, top, width and height inside the if check where they're actually used etc, but generally looks like a good approach.

Comment 14

[Boris Zbarsky [:bzbarsky]]

How does this patch handle the case when I have two screens, do view-source, move the view-source window to a different screen from the browser, close the view-source window, and then view source again?

Comment 15

[:Felipe Gomes (needinfo for replies!)]

The patch won't affect it (view-source will return to the second screen through its persist screenX/Y attribute).
Even a window that have a defined screenX to start at isn't created at that spot, but rather moved there after its XUL is loaded.

(Besides, the patch only affects windows loaded as "centerscreen".. all others will continue to be created at 0,0. Though if that wasn't the case it wouldn't make a difference AFAICT)

Comment 18

[bugzilla]

Not an exact duplicate of 599090 - original bug report mentions being able to right click the minimized icon in the status bar but when modal window bug hits right clicking the tab in window's task bar does not produce the expected context window.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Attachment: Constrain initial position to same screen - v2

Update patch: cleaned the code to be added into a new function. Also wrote a test.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 478424 [details] [diff] [review]
Constrain initial position to same screen - v2

Jim, I see that you've looked at mBounds recently, so asking the addl. review for the widget change here. Here's what happened:

The window was created at negative coordinates, but mBounds assumed it was 0,0 and only retrieved the width/height of the window.

Later the window was supposed to be moved to 0,0, but as mBounds.{x,y} already had that value, nsWindow::Move bailed out early and no move happened, keeping the window off-screen.


(The test needs three files but it's simple: it opens a window at -3000,-3000, and this window opens a new "dependent,centerscreen" window and verifies that this window is correctly moved on-screen.)

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #19)
> Created attachment 478424 [details] [diff] [review]
> Constrain initial position to same screen - v2
> 
> Update patch: cleaned the code to be added into a new function. Also wrote a
> test.

Does everything still work correctly considering you're now creating windows offscreen? (I imagine it would, since windows itself moves windows off screen for various reasons.)

Also, do we get a WM_MOVE event here on create? I wonder how content / nsxulwindow handle the negative coords if we do.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #21)
> (In reply to comment #19)
> > Created attachment 478424 [details] [diff] [review] [details]
> > Constrain initial position to same screen - v2
> > 
> > Update patch: cleaned the code to be added into a new function. Also wrote a
> > test.
> 
> Does everything still work correctly considering you're now creating windows
> offscreen? (I imagine it would, since windows itself moves windows off screen
> for various reasons.)

Yeah, and also on multiple-monitor setups windows can have negative coordinates, so it's at least known that nothing breaks horribly in that case.

Note though that with the appshell changes in this patch, no window will be created offscreen, as that code's purpose is to ensure that a window is created at an X/Y at an existing screen.
(This means that the widget change here is not strictly necessary to resolve this bug, but I believe that for correctness it's needed)

> 
> Also, do we get a WM_MOVE event here on create? I wonder how content /
> nsxulwindow handle the negative coords if we do.

I'll check this

Comment 24

[:Felipe Gomes (needinfo for replies!)]

> > Also, do we get a WM_MOVE event here on create? I wonder how content /
> > nsxulwindow handle the negative coords if we do.
> 
> I'll check this

Confirmed that no WM_MOVE is sent with negative coordinates during window creation

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 478424 [details] [diff] [review]
Constrain initial position to same screen - v2

ok, r+ on the widget parts.

Comment 26

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/ab9936edd2ad