[PATCH setup] Make sure that fatal error messages are visible

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH setup] Make sure that fatal error messages are visible

Ken Brown-6
The message box produced by TOPLEVEL_CATCH could be hidden by whatever
window was previously being displayed, so that setup appeared to hang.
Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
---
 msg.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/msg.cc b/msg.cc
index 403e78a..0ba4839 100644
--- a/msg.cc
+++ b/msg.cc
@@ -83,7 +83,7 @@ fatal (HWND owner, int id, ...)
 {
   va_list args;
   va_start (args, id);
-  mbox (owner, "fatal", 0, id, args);
+  mbox (owner, "fatal", MB_SETFOREGROUND, id, args);
   Logger ().exit (1);
 }
 
--
2.15.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup] Make sure that fatal error messages are visible

Jon TURNEY
On 19/12/2017 00:53, Ken Brown wrote:
> The message box produced by TOPLEVEL_CATCH could be hidden by whatever
> window was previously being displayed, so that setup appeared to hang.
> Fix this by giving fatal error message boxes type MB_SETFOREGROUND.

This is good as far as it goes, but is kind of working around the fact
that fatal() is being called with an NULL owner HWND.

This is not idea because I guess it means that propsheet window is still
activate-able when this messagebox is displayed (MB_APPMODAL doesn't apply)?

> ---
>   msg.cc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/msg.cc b/msg.cc
> index 403e78a..0ba4839 100644
> --- a/msg.cc
> +++ b/msg.cc
> @@ -83,7 +83,7 @@ fatal (HWND owner, int id, ...)
>   {
>     va_list args;
>     va_start (args, id);
> -  mbox (owner, "fatal", 0, id, args);
> +  mbox (owner, "fatal", MB_SETFOREGROUND, id, args);
>     Logger ().exit (1);
>   }
>  
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup] Make sure that fatal error messages are visible

Ken Brown-6
On 12/20/2017 11:19 AM, Jon Turney wrote:

> On 19/12/2017 00:53, Ken Brown wrote:
>> The message box produced by TOPLEVEL_CATCH could be hidden by whatever
>> window was previously being displayed, so that setup appeared to hang.
>> Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
>
> This is good as far as it goes, but is kind of working around the fact
> that fatal() is being called with an NULL owner HWND.
>
> This is not idea because I guess it means that propsheet window is still
> activate-able when this messagebox is displayed (MB_APPMODAL doesn't
> apply)?
It turns out that MB_SYSTEMMODAL did the job.  I tried MB_APPLMODAL and
MB_TASKMODAL also, but both of those still allowed me to activate the
propsheet window.

Revised patch attached.

Ken


0001-Make-sure-that-fatal-error-messages-are-visible.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup] Make sure that fatal error messages are visible

Brian Inglis
On 2017-12-20 10:09, Ken Brown wrote:

> On 12/20/2017 11:19 AM, Jon Turney wrote:
>> On 19/12/2017 00:53, Ken Brown wrote:
>>> The message box produced by TOPLEVEL_CATCH could be hidden by whatever
>>> window was previously being displayed, so that setup appeared to hang.
>>> Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
>>
>> This is good as far as it goes, but is kind of working around the fact that
>> fatal() is being called with an NULL owner HWND.
>>
>> This is not idea because I guess it means that propsheet window is still
>> activate-able when this messagebox is displayed (MB_APPMODAL doesn't apply)?
>
> It turns out that MB_SYSTEMMODAL did the job.  I tried MB_APPLMODAL and
> MB_TASKMODAL also, but both of those still allowed me to activate the propsheet
> window.

Is it really a problem if users can look at other windows when there is an
error? It is often useful to be able to look at your inputs to see if they
played a role in causing the error, or it is some external issue.

--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup] Make sure that fatal error messages are visible

Ken Brown-6
On 12/20/2017 10:02 PM, Brian Inglis wrote:

> On 2017-12-20 10:09, Ken Brown wrote:
>> On 12/20/2017 11:19 AM, Jon Turney wrote:
>>> On 19/12/2017 00:53, Ken Brown wrote:
>>>> The message box produced by TOPLEVEL_CATCH could be hidden by whatever
>>>> window was previously being displayed, so that setup appeared to hang.
>>>> Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
>>>
>>> This is good as far as it goes, but is kind of working around the fact that
>>> fatal() is being called with an NULL owner HWND.
>>>
>>> This is not idea because I guess it means that propsheet window is still
>>> activate-able when this messagebox is displayed (MB_APPMODAL doesn't apply)?
>>
>> It turns out that MB_SYSTEMMODAL did the job.  I tried MB_APPLMODAL and
>> MB_TASKMODAL also, but both of those still allowed me to activate the propsheet
>> window.
>
> Is it really a problem if users can look at other windows when there is an
> error? It is often useful to be able to look at your inputs to see if they
> played a role in causing the error, or it is some external issue.

We're talking about situations in which something unexpected went wrong,
and setup has decided that it needs to display an error message, write
the log file, and exit.  I don't think we want to take a chance on
damaging users' systems by letting them interact with the propsheet
window at this point.

Ken

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup] Make sure that fatal error messages are visible

Ken Brown-6
In reply to this post by Ken Brown-6
On 12/20/2017 12:09 PM, Ken Brown wrote:

> On 12/20/2017 11:19 AM, Jon Turney wrote:
>> On 19/12/2017 00:53, Ken Brown wrote:
>>> The message box produced by TOPLEVEL_CATCH could be hidden by whatever
>>> window was previously being displayed, so that setup appeared to hang.
>>> Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
>>
>> This is good as far as it goes, but is kind of working around the fact
>> that fatal() is being called with an NULL owner HWND.
>>
>> This is not idea because I guess it means that propsheet window is
>> still activate-able when this messagebox is displayed (MB_APPMODAL
>> doesn't apply)?
>
> It turns out that MB_SYSTEMMODAL did the job.  I tried MB_APPLMODAL and
> MB_TASKMODAL also, but both of those still allowed me to activate the
> propsheet window.

I just discovered that MB_SYSTEMMODAL doesn't prevent activation of the
propsheet in all circumstances.  I tried the following:  I created a
corrupt local package in the local cache and then tried to reinstall the
package.  This led to an exception thrown by check_for_cached(), which
normally would be caught by do_download_thread().  But I disabled that
as follows:

--- a/download.cc
+++ b/download.cc
@@ -278,8 +278,8 @@ do_download_thread (HINSTANCE h, HWND owner)
           catch (Exception * e)
             {
               // We know what to do with these..
-             if (e->errNo() == APPERR_CORRUPT_PACKAGE)
-               fatal (owner, IDS_CORRUPT_PACKAGE, pkg.name.c_str());
+             // if (e->errNo() == APPERR_CORRUPT_PACKAGE)
+             //        fatal (owner, IDS_CORRUPT_PACKAGE,
pkg.name.c_str());
               // Unexpected exception.
               throw e;
             }

As I result the exception was caught by the TOPLEVEL_CATCH for the
download thread.  When the fatal message box was shown, I was still able
to interact with the download progress reporting window.  Fortunately,
the only available button there was 'Cancel', so no harm was done.

In spite of this, MB_SYSTEMMODAL is still the best option I've been able
to come up with.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup] Make sure that fatal error messages are visible

Ken Brown-6
On 12/21/2017 5:55 PM, Ken Brown wrote:
> On 12/20/2017 12:09 PM, Ken Brown wrote:
>> On 12/20/2017 11:19 AM, Jon Turney wrote:
>>> On 19/12/2017 00:53, Ken Brown wrote:
>>>> The message box produced by TOPLEVEL_CATCH could be hidden by whatever
>>>> window was previously being displayed, so that setup appeared to hang.
>>>> Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
>>>
>>> This is good as far as it goes, but is kind of working around the
>>> fact that fatal() is being called with an NULL owner HWND.

I think I have a better solution, which simply avoids calling fatal()
with a NULL owner HWND (with one exception).

New patch attached.

Ken

0001-Give-TOPLEVEL_CATCH-an-owner-window.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH setup] Make sure that fatal error messages are visible

Jon TURNEY
On 28/12/2017 01:38, Ken Brown wrote:

> On 12/21/2017 5:55 PM, Ken Brown wrote:
>> On 12/20/2017 12:09 PM, Ken Brown wrote:
>>> On 12/20/2017 11:19 AM, Jon Turney wrote:
>>>> On 19/12/2017 00:53, Ken Brown wrote:
>>>>> The message box produced by TOPLEVEL_CATCH could be hidden by whatever
>>>>> window was previously being displayed, so that setup appeared to hang.
>>>>> Fix this by giving fatal error message boxes type MB_SETFOREGROUND.
>>>>
>>>> This is good as far as it goes, but is kind of working around the
>>>> fact that fatal() is being called with an NULL owner HWND.
>
> I think I have a better solution, which simply avoids calling fatal()
> with a NULL owner HWND (with one exception).
>
> New patch attached.

Great. Please apply.