350299 - Software Update wizard no longer loses focus after clicking "Restart Now", if Firefox was not focused when the wizard appeared

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

I've noticed this in the past few 1.8 branch nightlies - after downloading the update and clicking "Restart Now", with multiple windows open each containing multiple tabs, it seems like nothing happens. The I moved the software update dialog away and noticed that the "Confirm close tabs" dialog had appeared behind it.

The JS console shows an error about window.setTimeout() throwing NS_ERROR_NOT_INITIALIZED, at line http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/wizard.xml&rev=1.30&mark=269#255 . That scares me a little bit.

I will try to see if I can reproduce this consistently.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Turns out this only happens when the Firefox window doesn't have focus when the software update dialog appears.

Comment 2

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

Nominating for blocking: drivers, this is pretty bad, since the width of the software update dialog almost completely occludes the "do you want to close n tabs" dialog making it potentially impossible to see why it's not working.

Comment 3

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

Tested this with 1.5.0.5, and in that build, the Software Update dialog disappears when you click "Update Now" (which is also what it does on Mac).

cc'ing seth, as he was the last dude to touch this code.

Comment 4

[(not reading, please use [e-mail] instead)]

taking and investigating.

Comment 5

[(not reading, please use [e-mail] instead)]

here's what I think is going on here:

when the user clicks "Restart XXX Now", we call gFinishedPage.onWizardFinish()

this code notifies all the observers that the app is trying to quit, and then tries to close all the windows.

When the browser doesn't have focus, this causes the "confirm close" prompt for the windows to come up "behind" the software update dialog.

One low risk fix that I'm trying is to switch focus to the various windows before we call tryToClose().

See http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1595

Comment 6

[(not reading, please use [e-mail] instead)]

> The JS console shows an error about window.setTimeout() throwing
> NS_ERROR_NOT_INITIALIZED, at line
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/wizard.xml&rev=1.30&mark=269#255
. That scares me a little bit.

That scares me too, but I haven't seen it yet.

Comment 7

[(not reading, please use [e-mail] instead)]

this should also be affecting tbird, with the tryToClose handler for the compose window.

Comment 8

[(not reading, please use [e-mail] instead)]

I thought I could reproduce (following gavin's tips) but I haven't been able to make it happen again.

I'm also not sure that focusing the window before calling win.tryToClose() is the right fix.  (looking at the code base in lxr, we use the nsIWindowMediator to enumerate all the windows in several places, and no one seems to need to focus the windows.)

but, I do see a related scenario that needs fixing that could also fix gavin's scenario.

here is the related scenario:

1)  open a window and open many tabs (make sure your "warn on close" pref is enabled)
2)  minimize the window
3)  wait for software update
4)  ...resume scenario per gavin

with the window minimized, the software update dialog does not lose focus (or move to the back) when you do "Restart Now".  (This is why I think it is related to gavin's original scenario)

the "confirm close" prompt (at least for me and my window manager on windows xp) opens up at the top left, and is "behind" in z order the software update window.

when the user clicks the "Restart Now" button, the onWizardFinished() attempts to quit and close before window.close() is finally called on the wizard window.  

when in limbo, and the "confirm close" prompts are up, the software update (if you switch to it) still has the "Restart Now" and "Later" buttons enabled.

I'm wondering, looking at the EM code, if we should be calling tryToClose() at all?  (the EM seems to check if we can quit, and then does the restart | quit.

I'll try this out, and report back.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

When I was testing, the case that seemed to trigger the problem most (and that led me to file the bug) was getting the prompt with several Firefox windows open without focus, but not minimized. E.g. have two Firefox windows open with multiple tabs in each, then open Notepad in the foreground and wait for the alert to appear. Then, Alt+Tab directly to the Update wizard and click "Restart now".

Comment 10

[(not reading, please use [e-mail] instead)]

my "remove call to tryToClose" has (at least) two problems.

1)  gavin points out, we want this prompt, see bug #341248 (where we decided to keep the prompt, as session restore is not perfect.)

2)  tbird, and other apps, don't have session restore yet.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: patch, comments to follow

Comment 12

[(not reading, please use [e-mail] instead)]

I've tried a couple things, including doing the window.close() before the tryToClose(), but that led to the scary:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

Here's what I've got with that patch:

1)  the change to updates.js disables the two buttons when in limbo.  From my comment #8: 

'when in limbo, and the "confirm close" prompts are up, the software update (if
you switch to it) still has the "Restart Now" and "Later" buttons enabled.'

2)  in tabbrowser.xml, in the warnAboutClosingTabs() method, if I'm about to prompt, I first call window.focus().  this will raise minimized windows and should take care of gavin's scenario where the prompt gets hidden behind the software update wizard.  it also has a side benefit of making it clear (for minimized windows) which browser window the prompt is associated with.

the other place that calls warnAboutClosingTabs() is when you do "close other tabs", but in that case, the window already has focus, so there is no harm.

I think this is the right place for the window.focus() [and not in updates.js], because we only want to raise the window if we are going to show the prompt.

Note, it appears that tbird already has a window.focus() in the tbird compose window tryToClose handler, see http://lxr.mozilla.org/mozilla1.8/source/mail/components/compose/content/MsgComposeCommands.js#2500

Also note, that the ideal fix would be more involved (and more risky).  See ben's comment in onWizardFinish(), at http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1599

Comment 13

[(not reading, please use [e-mail] instead)]

another place that needs the window.focus() is the update wizard itself.

for example, if you minimize the software update wizard while downloading, and then quit, we have a similar issue.

http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1292

Comment 14

[(not reading, please use [e-mail] instead)]

Attachment: patch, includes adding focus() to the trytoclose in the update wizard.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

I'm wondering if the prompt service should take care of focusing the parent window of parented alerts, but that would be changing the behavior for all callers, which certainly isn't suitable for the branch at this stage, and isn't necessarily correct for all cases (this seems to be bug 87592). I'm also kind of worried that stealing focus might be the wrong thing to do in some cases, calling focus() directly has led to accessibility issues in the past (for users of screen readers, for example), but I guess that since it only happens after an explicit user action (clicking the "restart now" button) and only before an alert appears, this should be OK.

I'm not really clear on the second scenario you describe (and therefore am not sure that the second hunk in that patch is correct), could you elaborate? Does quitting Firefox without focusing the software update dialog really cause onWizardCancel to be called? In other words, how is onWizardCancel called without the user clicking on the cancel button (in which case the window is already focused)?

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

I think I agree about the promptservice behaviour gavin mentioned, please file a followup there, and mention that we should back out these calls if/when we change that behaviour

Comment 17

[(not reading, please use [e-mail] instead)]

gavin and mconnor:  thanks for the reviews.

> I'm not really clear on the second scenario you describe (and therefore am not
> sure that the second hunk in that patch is correct), could you elaborate? Does
> quitting Firefox without focusing the software update dialog really cause
> onWizardCancel to be called? In other words, how is onWizardCancel called
> without the user clicking on the cancel button (in which case the window is
> already focused)?

yes, this scenario can happen if you start software update over a slow link, minimize the window, and then attempt to quit the application.  onWizardCancel will be called in that scenario.  I'll go get some additional details of the calls stack to onWizardCancel in my scenario.

I'm not convinced (yet) that we want the prompt service to focus the calling window.  It seems reasonable, but I haven't wrapped my head around it.  But for the tryToClose() callers, it is what we want.  I will log a spin off bug to discuss doing that, and to include a note about removing the manually callers to window.focus() before the prompt service calls.

Comment 18

[(not reading, please use [e-mail] instead)]

ok, some more info about the updates.js side of the fix and the associated scenario.

yes, we still want to focus the window.  here's why:

in the download is happening, and the software update wizard is minimized, quitting the app *will not* cause a prompt to show up because onWizardCancel() for the downloading page of the wizard doesn't have code to prompt.  

The way to get the prompt (screen shot coming) on exit is if you pause the download, minimize, then quit (or if you cancel the dialog when paused, same code path.)

see
http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#1285

Note, the the "quit" scenario, hitting "yes" the prompt will not stop the closing of the app.  We should pass up the result of the prompt up to onWizardCancel(), and back up to tryToClose(), so that if you hit "yes" we will not shut down the app.  I'll spin up a new bug on this issue.

Note, in the quit scenario, the wording of this prompt needs some work.  I'll log a new bug on this issue.

I'm going to check the r/sr patch into the trunk, and then seek approval for the branch.

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: here is the prompt I'm talking about (notice how for quit, the wording isn't quite right)

Comment 20

[Mike Connor [:mconnor]]

did this make it into trunk yet?

Comment 21

[(not reading, please use [e-mail] instead)]

> did this make it into trunk yet?

sorry for the delay, I just landed it.  

Here are the scenarios that I re-tested on the trunk that this patch addresses:

1)  when a browser window is minimized (or "behind" in the z-order), we will raise the window if we attempt to confirm the close of multiple tabs on quit.  (adding window.focus() fixes this.)

2)  when the software update window is minimized (or "behind" in the z-order), we will raise the wizard if we attempt confirm a paused download on quit. [as noted earlier, the wording of this prompt is good for cancel, but not quitting the app.  spin of bugs coming...]

3)  In the software update wizard after downloading an update and upon clicking "Restart Now", we disable the "Restart Now" and "Later" buttons so that while the user is dealing with various "try to close handlers", the user can't switch back to the software update wizard and click the "Restart Now" and "Later" buttons.

I'll go log the 3 or 4 spin off bugs previously mentioned in this bug (non of which block ff2) and then seek approval for the branch.

Comment 22

[(not reading, please use [e-mail] instead)]

Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

seeking approval

Comment 23

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

Comment on attachment 237075 [details] [diff] [review]
patch, includes adding focus() to the trytoclose in the update wizard.

a=beltzner on behalf of 181drivers

Comment 24

[(not reading, please use [e-mail] instead)]

fix landed on the branch.

gavin or adam:  if you guys know of any "when I quit the app, the 'confirm close' prompt is at position 0,0" or "when I quit the app, the 'confirm close' prompt does not appear modal to the browser window" bugs, this fix would fix those issues.

Comment 25

[David Dabbs]

Attachment: "Update Downloaded" dialog covering "Confirm Close" dialog

In this case, the "Confirm Close" dialog was slightly visible. However, clicking on it would not bring it forward even though it has focus.

Comment 26

[David Dabbs]

Attachment: "Update Downloaded" dialog covering Photoshop

Even after loading Photoshop, the dialog remains on top of all other windows. Notice that the hidden Photoshop dialog appears to have focus.

Comment 27

[David Dabbs]

Comment on attachment 239371 [details]
"Update Downloaded" dialog covering "Confirm Close" dialog

This and the other "Update Downloaded" image were created after applying an update from the 2.0b nightly channel.


FF windows open:
* one browser window with multiple open tabs.
* I do not believe FF had focus, but I was away from my desk when SoftUpdt fired

Clicked [Update & Restart Bon Echo now].

... here are the last two entries from updates.xml ...
<update type="minor" name="Bon Echo 2.0" version="2.0" extensionVersion="2.0" serviceURL="https://aus2.mozilla.org/update/2/Firefox/2.0/2006091905/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/update.xml" installDate="1158773232165" buildID="2006092004" isCompleteUpdate="false" channel="nightly">
<patch type="complete"
URL="http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006-09-20-04-mozilla1.8/firefox-2.0.en-US.win32.complete.mar" size="7379034" selected="false" state="undefined"/>

<patch type="partial"
URL="http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006-09-19-05-mozilla1.8/firefox-2.0.en-US.win32.partial.2006091905-2006092004.mar" size="7434" selected="true" state="succeeded"/>
</update>

<update type="minor" name="Bon Echo 2.0" version="2.0" extensionVersion="2.0" 
serviceURL="https://aus2.mozilla.org/update/2/Firefox/2.0/2006091804/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/update.xml" installDate="1158680072403" 
buildID="2006091905" isCompleteUpdate="false" channel="nightly">
<patch type="complete"
URL="http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006-09-19-05-mozilla1.8/firefox-2.0.en-US.win32.complete.mar"
size="7378593" selected="true" state="succeeded"/>
</update>