Closed Bug 798861 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Installer, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 19
Tracking Status
firefox18 --- verified

People

(Reporter: Yoric, Assigned: robert.strong.bugs)

References

Details

(Whiteboard: [stub+])

Attachments

(3 files, 1 obsolete file)

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.
Affected me also. Problems with modal dialog boxes aside, UX review (Bug 798869) may suggest not using modal dialogs at all.
Summary: Stub installer: Application not responding → Stub installer: Firefox is already dialog does not appear on front when it pops up, which makes the stub look like it's hung
Assignee: nobody → robert.bugzilla
Duplicate of this bug: 799175
Whiteboard: [stub+]
Status: NEW → ASSIGNED
Attached patch patch rev1 (obsolete) — Splinter Review
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.
Attachment #669832 - Flags: review?(netzen)
(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.
(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.
(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.
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). ;)
Attached patch patch rev2Splinter Review
Attachment #669832 - Attachment is obsolete: true
Attachment #669832 - Flags: review?(netzen)
Attachment #669854 - Flags: review?(netzen)
Attachment #669854 - Flags: review?(netzen) → review+
Keywords: verifyme
QA Contact: jsmith
https://hg.mozilla.org/mozilla-central/rev/f2fae799ff95
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Attachment #669854 - Flags: approval-mozilla-aurora?
Attachment #669854 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified on 10/12 build.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.