* [PATCH setup] Make sure that fatal error messages are visible @ 2017-12-19 0:53 Ken Brown 2017-12-20 16:19 ` Jon Turney 0 siblings, 1 reply; 8+ messages in thread From: Ken Brown @ 2017-12-19 0:53 UTC (permalink / raw) To: cygwin-apps 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH setup] Make sure that fatal error messages are visible 2017-12-19 0:53 [PATCH setup] Make sure that fatal error messages are visible Ken Brown @ 2017-12-20 16:19 ` Jon Turney 2017-12-20 17:09 ` Ken Brown 0 siblings, 1 reply; 8+ messages in thread From: Jon Turney @ 2017-12-20 16:19 UTC (permalink / raw) To: cygwin-apps; +Cc: Ken Brown 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); > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH setup] Make sure that fatal error messages are visible 2017-12-20 16:19 ` Jon Turney @ 2017-12-20 17:09 ` Ken Brown 2017-12-21 3:02 ` Brian Inglis 2017-12-21 22:55 ` Ken Brown 0 siblings, 2 replies; 8+ messages in thread From: Ken Brown @ 2017-12-20 17:09 UTC (permalink / raw) To: Jon Turney, cygwin-apps [-- Attachment #1: Type: text/plain, Size: 789 bytes --] 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 [-- Attachment #2: 0001-Make-sure-that-fatal-error-messages-are-visible.patch --] [-- Type: text/plain, Size: 929 bytes --] From 1f99ac4cc6c2b6c0b39aa84d80985cb21438a242 Mon Sep 17 00:00:00 2001 From: Ken Brown <kbrown@cornell.edu> Date: Mon, 18 Dec 2017 19:53:00 -0500 Subject: [PATCH setup v2 1/2] Make sure that fatal error messages are visible 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_SYSTEMMODAL. This also prevents the user from activating the property sheet window while the fatal message box is being displayed. --- msg.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msg.cc b/msg.cc index 403e78a..92c9675 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_SYSTEMMODAL, id, args); Logger ().exit (1); } -- 2.15.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH setup] Make sure that fatal error messages are visible 2017-12-20 17:09 ` Ken Brown @ 2017-12-21 3:02 ` Brian Inglis 2017-12-21 13:20 ` Ken Brown 2017-12-21 22:55 ` Ken Brown 1 sibling, 1 reply; 8+ messages in thread From: Brian Inglis @ 2017-12-21 3:02 UTC (permalink / raw) To: cygwin-apps 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH setup] Make sure that fatal error messages are visible 2017-12-21 3:02 ` Brian Inglis @ 2017-12-21 13:20 ` Ken Brown 0 siblings, 0 replies; 8+ messages in thread From: Ken Brown @ 2017-12-21 13:20 UTC (permalink / raw) To: cygwin-apps 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH setup] Make sure that fatal error messages are visible 2017-12-20 17:09 ` Ken Brown 2017-12-21 3:02 ` Brian Inglis @ 2017-12-21 22:55 ` Ken Brown 2017-12-28 1:38 ` Ken Brown 1 sibling, 1 reply; 8+ messages in thread From: Ken Brown @ 2017-12-21 22:55 UTC (permalink / raw) To: cygwin-apps 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH setup] Make sure that fatal error messages are visible 2017-12-21 22:55 ` Ken Brown @ 2017-12-28 1:38 ` Ken Brown 2018-01-05 13:54 ` Jon Turney 0 siblings, 1 reply; 8+ messages in thread From: Ken Brown @ 2017-12-28 1:38 UTC (permalink / raw) To: cygwin-apps [-- Attachment #1: Type: text/plain, Size: 676 bytes --] 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 [-- Attachment #2: 0001-Give-TOPLEVEL_CATCH-an-owner-window.patch --] [-- Type: text/plain, Size: 5209 bytes --] From b47adcb00209d717177d8932e8756736f61c80f2 Mon Sep 17 00:00:00 2001 From: Ken Brown <kbrown@cornell.edu> Date: Mon, 18 Dec 2017 19:46:36 -0500 Subject: [PATCH setup 1/2] Give TOPLEVEL_CATCH an owner window The fatal message box produced by TOPLEVEL_CATCH had a NULL owner window. This meant that the box could be hidden by whatever window was previously being displayed, so that setup appeared to hang. In addition, the user could interact with the propsheet window while the message box was displayed. Fix this by giving TOPLEVEL_CATCH an "owner" parameter. In all uses but one (TOPLEVEL_CATCH("main")), there is a non-NULL owner available that we can use. --- Exception.h | 6 +++--- download.cc | 2 +- ini.cc | 2 +- install.cc | 2 +- main.cc | 2 +- postinstall.cc | 2 +- prereq.cc | 2 +- proppage.cc | 2 +- site.cc | 2 +- 9 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Exception.h b/Exception.h index 7b16612..4aeb49e 100644 --- a/Exception.h +++ b/Exception.h @@ -37,15 +37,15 @@ private: #define APPERR_IO_ERROR 2 #define APPERR_LOGIC_ERROR 3 -#define TOPLEVEL_CATCH(threadname) \ +#define TOPLEVEL_CATCH(owner, threadname) \ catch (Exception *e) \ { \ - fatal(NULL, IDS_UNCAUGHT_EXCEPTION_WITH_ERRNO, (threadname), \ + fatal((owner), IDS_UNCAUGHT_EXCEPTION_WITH_ERRNO, (threadname), \ typeid(*e).name(), e->what(), e->errNo()); \ } \ catch (std::exception *e) \ { \ - fatal(NULL, IDS_UNCAUGHT_EXCEPTION, (threadname), \ + fatal((owner), IDS_UNCAUGHT_EXCEPTION, (threadname), \ typeid(*e).name(), e->what()); \ } diff --git a/download.cc b/download.cc index 697f94a..683376e 100644 --- a/download.cc +++ b/download.cc @@ -384,7 +384,7 @@ do_download_reflector (void *p) // Tell the progress page that we're done downloading Progress.PostMessageNow (WM_APP_DOWNLOAD_THREAD_COMPLETE, 0, next_dialog); } - TOPLEVEL_CATCH("download"); + TOPLEVEL_CATCH((HWND) context[1], "download"); ExitThread(0); } diff --git a/ini.cc b/ini.cc index f021ed2..e9152ec 100644 --- a/ini.cc +++ b/ini.cc @@ -422,7 +422,7 @@ do_ini_thread_reflector (void* p) // Tell the progress page that we're done downloading Progress.PostMessageNow (WM_APP_SETUP_INI_DOWNLOAD_COMPLETE, 0, succeeded); } - TOPLEVEL_CATCH ("ini"); + TOPLEVEL_CATCH ((HWND) context[1], "ini"); ExitThread (0); } diff --git a/install.cc b/install.cc index 8505199..d6af331 100644 --- a/install.cc +++ b/install.cc @@ -997,7 +997,7 @@ do_install_reflector (void *p) // Tell the progress page that we're done downloading Progress.PostMessageNow (WM_APP_INSTALL_THREAD_COMPLETE); } - TOPLEVEL_CATCH("install"); + TOPLEVEL_CATCH((HWND) context[1], "install"); ExitThread (0); } diff --git a/main.cc b/main.cc index b44f9b6..7c1170e 100644 --- a/main.cc +++ b/main.cc @@ -349,7 +349,7 @@ WinMain (HINSTANCE h, finish_up: ; } - TOPLEVEL_CATCH("main"); + TOPLEVEL_CATCH(NULL, "main"); // Never reached return 0; diff --git a/postinstall.cc b/postinstall.cc index c871201..3cd6ff0 100644 --- a/postinstall.cc +++ b/postinstall.cc @@ -253,7 +253,7 @@ do_postinstall_reflector (void *p) Progress.PostMessageNow (WM_APP_POSTINSTALL_THREAD_COMPLETE, 0, s.empty() ? IDD_DESKTOP : IDD_POSTINSTALL); } - TOPLEVEL_CATCH("postinstall"); + TOPLEVEL_CATCH((HWND) context[1], "postinstall"); /* Revert primary group to admins group. This allows to create all the state files written by setup as admin group owned. */ diff --git a/prereq.cc b/prereq.cc index 8b1bbbb..0a46ad1 100644 --- a/prereq.cc +++ b/prereq.cc @@ -364,7 +364,7 @@ do_prereq_check_reflector (void *p) // Tell the progress page that we're done prereq checking Progress.PostMessageNow (WM_APP_PREREQ_CHECK_THREAD_COMPLETE, 0, next_dialog); } - TOPLEVEL_CATCH("prereq_check"); + TOPLEVEL_CATCH((HWND) context[1], "prereq_check"); ExitThread(0); } diff --git a/proppage.cc b/proppage.cc index d4d2926..6b83640 100644 --- a/proppage.cc +++ b/proppage.cc @@ -359,7 +359,7 @@ PropertyPage::DialogProc (UINT message, WPARAM wParam, LPARAM lParam) return OnMessageApp (message, wParam, lParam); } } - TOPLEVEL_CATCH("DialogProc"); + TOPLEVEL_CATCH(GetHWND (), "DialogProc"); // Wasn't handled return FALSE; diff --git a/site.cc b/site.cc index a945d28..5e20b3b 100644 --- a/site.cc +++ b/site.cc @@ -445,7 +445,7 @@ do_download_site_info_thread (void *p) Progress.PostMessageNow (WM_APP_SITE_INFO_DOWNLOAD_COMPLETE, 0, IDD_SITE); } } - TOPLEVEL_CATCH("site"); + TOPLEVEL_CATCH((HWND) context[1], "site"); ExitThread(0); } -- 2.15.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH setup] Make sure that fatal error messages are visible 2017-12-28 1:38 ` Ken Brown @ 2018-01-05 13:54 ` Jon Turney 0 siblings, 0 replies; 8+ messages in thread From: Jon Turney @ 2018-01-05 13:54 UTC (permalink / raw) To: Ken Brown, cygwin-apps 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-01-05 13:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-19 0:53 [PATCH setup] Make sure that fatal error messages are visible Ken Brown 2017-12-20 16:19 ` Jon Turney 2017-12-20 17:09 ` Ken Brown 2017-12-21 3:02 ` Brian Inglis 2017-12-21 13:20 ` Ken Brown 2017-12-21 22:55 ` Ken Brown 2017-12-28 1:38 ` Ken Brown 2018-01-05 13:54 ` Jon Turney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).