798861 - Stub installer: Firefox is already dialog does not appear on front when it pops up, which makes the stub look like it's hung

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

I just tested the stub installer. In appearance, the installation seemed to end with "Application not responding". In fact, a dialog, was informing me that Firefox was already launched, but for some reason this dialog was hidden.

This makes us look quite bad in front of users.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: What I saw

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: What was hidden below

Comment 3

[Sean Stangl [:sstangl]]

Affected me also. Problems with modal dialog boxes aside, UX review (Bug 798869) may suggest not using modal dialogs at all.

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

The check for the Firefox Message Window can only be done reliably from the unelevated process which typically doesn't display UI. The options are to hide the existing installer window and not set MB_TOPMOST|MB_SETFOREGROUND on the messagebox which could be missed by the user or to set MB_TOPMOST|MB_SETFOREGROUND which will display the messagebox in front of other apps. I opted for the second.

Comment 6

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #5)
> Created attachment 669832 [details] [diff] [review]
> patch rev1
> 
> The check for the Firefox Message Window can only be done reliably from the
> unelevated process which typically doesn't display UI. 

Why can't it be done from the elevated process?
Even if you elevate to another user account it will be using the same session, window station and desktop.  I suspect the reason the message box shows underneath is because it sets its owner to the unelevated window which comes before in z-order.

> The options are to
> hide the existing installer window and not set MB_TOPMOST|MB_SETFOREGROUND
> on the messagebox which could be missed by the user or to set
> MB_TOPMOST|MB_SETFOREGROUND which will display the messagebox in front of
> other apps. I opted for the second.

If you set MB_TOPMOST the user will be unable to move anything on top of that message box unless it is also top most.  Even after the user sees the message.  Perhaps we can just set MB_SETFOREGROUND?

Also should we be doing this from the installer and elsewhere for other message boxes? I remember seeing a bug report at some point about this happening in the normal installer when the app is already running.

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #5)
> > Created attachment 669832 [details] [diff] [review]
> > patch rev1
> > 
> > The check for the Firefox Message Window can only be done reliably from the
> > unelevated process which typically doesn't display UI. 
> 
> Why can't it be done from the elevated process?
> Even if you elevate to another user account it will be using the same
> session, window station and desktop.  I suspect the reason the message box
> shows underneath is because it sets its owner to the unelevated window which
> comes before in z-order.
Because we need to find the Firefox Message Window which isn't available to the elevated process when launched as a different user.

> > The options are to
> > hide the existing installer window and not set MB_TOPMOST|MB_SETFOREGROUND
> > on the messagebox which could be missed by the user or to set
> > MB_TOPMOST|MB_SETFOREGROUND which will display the messagebox in front of
> > other apps. I opted for the second.
> 
> If you set MB_TOPMOST the user will be unable to move anything on top of
> that message box unless it is also top most.  Even after the user sees the
> message.  Perhaps we can just set MB_SETFOREGROUND?
Sure

> Also should we be doing this from the installer and elsewhere for other
> message boxes? I remember seeing a bug report at some point about this
> happening in the normal installer when the app is already running.
We don't do this elsewhere iirc and if we did that would need a new bug anyways.

Comment 8

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #7)
> (In reply to Brian R. Bondy [:bbondy] from comment #6)
> > (In reply to Robert Strong [:rstrong] (do not email) from comment #5)
> > > Created attachment 669832 [details] [diff] [review]
> > > patch rev1
> > > 
> > > The check for the Firefox Message Window can only be done reliably from the
> > > unelevated process which typically doesn't display UI. 
> > 
> > Why can't it be done from the elevated process?
> > Even if you elevate to another user account it will be using the same
> > session, window station and desktop.  I suspect the reason the message box
> > shows underneath is because it sets its owner to the unelevated window which
> > comes before in z-order.
> Because we need to find the Firefox Message Window which isn't available to
> the elevated process when launched as a different user.

A process can find another HWND it's looking for, as long as it's in the same Desktop.  
A process can interact with an HWND it's looking for, as long as it's in the same Desktop and as long as it's equal or higher integrity. (Some exceptions exist that low priority processes can interact with higher ones).

On the Window's Session running in front of you right now, you have a Winlogon Desktop (which is the lock screen), Default Desktop (which is what you see now), ScreenSaver Desktop, and Secure Desktop.  The Secure Desktop is only used for UAC prompts.  

Both the elevated installer and unelevated installer run inside the Default Desktop.  Even if you are a limited user account and you elevate to another user, if you see it on your screen alongside other apps, then you are running in the same Desktop.  Any API involving an HWND will work within the same Desktop, even if it's from different users.  This is part of the reason Windows Vista introduced Session 0 isolation.  People were showing UIs from services and then the first logged on user could interact with that UI and get system level access.

I would prefer to use the FindWindow call inside the elevated process and only use MB_SETFOREGROUND.

Comment 9

[Robert Strong (they/them - no direct email)]

Quite possibly... the last time I checked that it was a very long time ago and I was under a tight deadline (kind of like now). ;)

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Comment 11

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fae799ff95

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/dc83b62af3ec

Comment 14

[Jason Smith [:jsmith]]

Verified on 10/12 build.