Re: [boost] [Boost-build] Milestone 11, release candidate 3

From: Vladimir Prus (ghost_at_[hidden])
Date: 2006-07-13 03:50:52

On Wednesday 12 July 2006 20:13, Bronek Kozicki wrote:

Hi Bronek,
I'm replying on list, so that Win32 experts can comment too.

> attached .diff is addressing 3 bugs in execnt.c
> - memory leak in is_parent_child
> - closing unrelated dialogs (displayed by descendants of explorer.exe)
> - missing dialogs displayed by csrss.exe ("... - Unable To Locate
> Component")
>
> there are also other improvements and changes that I tested much more
> thoroughly than previous update to execnt.c

> --- C:\Documents and Settings\Bronek Kozicki\Desktop\execnt.cšššThu Jun 8 02:25:58 2006 UTC
> +++ E:\DEVEL\BOOST_RTEST\boost\tools\jam\src\execnt.cšššSun Jun 25 18:55:27 2006 UTC
> @@ -1001,8 +1001,10 @@
> šint is_parent_child(DWORD parent, DWORD child)
> š{

While we're at it, can be have a comment for this function, at least saying what are 'parent' and
'child' arguments. Are they pids or something else?

> š š šHANDLE process_snapshot_h = INVALID_HANDLE_VALUE;
> š
> + š šif (!child)
> + š š š šreturn 0;
> š š šif (parent == child)
> š š š š šreturn 1;
> š
> š š šprocess_snapshot_h = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS,0);
> @@ -1015,49 +1017,100 @@
> š š š š š š šok = Process32First(process_snapshot_h, &pinfo); 
> š š š š š š šok == TRUE; 
> š š š š š š šok = Process32Next(process_snapshot_h, &pinfo) )
> š š š š š{
> - š š š š š šif (pinfo.th32ProcessID == child && pinfo.th32ParentProcessID)
> + š š š š š šif (pinfo.th32ProcessID == child)
> + š š š š š š{
> + š š š š š š š šCloseHandle(process_snapshot_h);
> + š š š š š š š šif (!stricmp(pinfo.szExeFile, "explorer.exe"))

This will be clearer if written like:

                if (stricmp(pinfo.szExeFile, "explorer.exe") == 0)

> + š š š š š š š š{
> + š š š š š š š š š š/* explorer.exe is orphaned and process_id of its parent may
> + š š š š š š š š š šaccidentally match process_id of process we are after. We must not
> + š š š š š š š š š šclose dialog boxes displayed by children of explorer.exe even 
> + š š š š š š š š š šthough (thanks to its parent process id) it might appear to be 
> + š š š š š š š š š šour child. This is not very reliable - there might be more 
> + š š š š š š š š š šorphaned processes or shell might be something else than 
> + š š š š š š š š š šexplorer.exe, but this is most common and important scenario */

The last line of this comment is longer than 80 characters. At the same time, thanks for 
detailed comments! This is tricky code so it's good to have explanations.

> + š š š š š š š š š šreturn 0; 
> + š š š š š š š š}
> + š š š š š š š šif (!stricmp(pinfo.szExeFile, "csrss.exe"))
> + š š š š š š š š{
> + š š š š š š š š š š/* csrss.exe may display message box like following:
> + š š š š š š š š š š š šxyz.exe - Unable To Locate Component
> + š š š š š š š š š š š šThis application has failed to start because boost_foo-bar.dll 
> + š š š š š š š š š š š šwas not found. Re-installing the application may fix the problem
> + š š š š š š š š š šThis actually happens when starting test process that depends on 
> + š š š š š š š š š šdynamic library which failed to build. We want to automatically 
> + š š š š š š š š š šclose these message boxes even though csrss.exe is not our 
> + š š š š š š š š š šchild process. We may depend on the fact that (in all current 
> + š š š š š š š š š šversions of Windows) csrss.exe is indirectly child of System which 
> + š š š š š š š š š šalways has process id == 4 */
> + š š š š š š š š š šif (is_parent_child(4, pinfo.th32ParentProcessID))
> + š š š š š š š š š š š šreturn 1;

Oh, closing those message box is cool. The comment has lines longer than 80 characters, though ;-)

> + š š š š š š š š}
> š š š š š š š š šreturn is_parent_child(parent, pinfo.th32ParentProcessID);
> + š š š š š š}
> š š š š š}
> š
> š š š š šCloseHandle(process_snapshot_h);
> š š š}
> š
> š š šreturn 0;
> š}
> š
> -int related(HANDLE h, DWORD p)
> +int related(DWORD d, DWORD p)
> š{
> - š šreturn is_parent_child(get_process_id(h), p);
> + š šreturn is_parent_child(d, p);
> š}

If this function just forwards to is_parent_child, can we just change all callers to call is_parent_child?
 š
> +typedef struct PROCESS_HANDLE_ID {HANDLE h; DWORD pid;} PROCESS_HANDLE_ID;
> +
> šBOOL CALLBACK window_enum(HWND hwnd, LPARAM lParam)
> š{
> - š šchar buf[10] = {0};
> - š šHANDLE h = *((HANDLE*) (lParam));
> + š šchar buf[7] = {0};
> + š šPROCESS_HANDLE_ID p = *((PROCESS_HANDLE_ID*) (lParam));
> š š šDWORD pid = 0;
> + š šDWORD tid = 0;
> +
> + š šif (!IsWindowVisible(hwnd))
> + š š š šreturn TRUE;
> š
> - š šif (!GetClassNameA(hwnd, buf, 10))
> - š š š šreturn TRUE; // failed to read class name
> + š šif (!GetClassNameA(hwnd, buf, 7))
> + š š š šreturn TRUE; /* failed to read class name; presume it's not a dialog */

Use sizeof(buf), perhaps? 
 š
> š š šif (strcmp(buf, "#32770"))

While we're at it, it will be clearer if written like:

       if (strcmp(buf, "#32770") != 0)

I realize this is not originally yours code, but anyway.

> -        return TRUE; // not a dialog
> +        return TRUE; /* not a dialog */

BTW, what does this code does? Do all dialogs have class name of #32770? 
Or this is specific class of dialogs we're interested in?

> - š šGetWindowThreadProcessId(hwnd, &pid);
> - š šif (related(h, pid))
> + š štid = GetWindowThreadProcessId(hwnd, &pid);
> + š šif (tid && related(p.pid, pid))

What's the point of check for 'tid'?

> š š š{
> - š š š šPostMessage(hwnd, WM_QUIT, 0, 0);
> - š š š š// just one window at a time
> + š š š š/* ask really nice */
> + š š š šPostMessageA(hwnd, WM_CLOSE, 0, 0);
> + š š š š/* now wait and see if it worked. If not, insist */
> + š š š šif (WaitForSingleObject(p.h, 200) == WAIT_TIMEOUT)
> + š š š š{
> + š š š š š šPostThreadMessageA(tid, WM_QUIT, 0, 0);
> + š š š š š šif (WaitForSingleObject(p.h, 500) == WAIT_TIMEOUT)
> + š š š š š š{
> + š š š š š š š šPostThreadMessageA(tid, WM_QUIT, 0, 0);
> + š š š š š š š šWaitForSingleObject(p.h, 500);

Is posting WM_QUIT message twice better than posting it once?

> + š š š š š š}
> + š š š š}
> š š š š šreturn FALSE;
> š š š}
> š
> š š šreturn TRUE;
> š}
> š
> švoid close_alert(HANDLE process)
> š{
> - š šEnumWindows(&window_enum, (LPARAM) &process);
> + š šDWORD pid = get_process_id(process);
> + š š/* If process already exited or we just cannot get its process id, do not go any further */
> + š šif (pid)
> + š š{
> + š š š šPROCESS_HANDLE_ID p = {process, pid};
> + š š š šEnumWindows(&window_enum, (LPARAM) &p);
> + š š}
> š}
> š
> šstatic int
> šmy_wait( int *status )
> @@ -1093,23 +1146,29 @@
> šššššššš}
> š š š
> š š šif ( globs.timeout > 0 )
> š š š{
> + š š š šunsigned int alert_wait = 1;
> š š š š š/* with a timeout we wait for a finish or a timeout, we check every second
> š š š š š to see if something timed out */
> - š š š šfor (waitcode = WAIT_TIMEOUT; waitcode == WAIT_TIMEOUT;)
> + š š š šfor (waitcode = WAIT_TIMEOUT; waitcode == WAIT_TIMEOUT; ++alert_wait)
> š š š š š{
> š š š š š š šwaitcode = WaitForMultipleObjects( num_active, active_handles, FALSE, 1*1000 /* 1 second */ );
> š š š š š š šif ( waitcode == WAIT_TIMEOUT )
> š š š š š š š{
> š š š š š š š š š/* check if any jobs have surpassed the maximum run time. */
> š š š š š š š š šfor ( i = 0; i < num_active; ++i )
> š š š š š š š š š{
> š š š š š š š š š š šdouble t = running_time(active_handles[i]);
> +
> + š š š š š š š š š š/* periodically (each 5 secs) review and close alert dialogs hanging around */
> + š š š š š š š š š šif ((alert_wait % ((unsigned int) 5)) == 0)
> + š š š š š š š š š š š šclose_alert(active_handles[i]);
> +

Is the point here to close dialogs right away, and not wait possibly long time for timeout? If so,
maybe this should be added to comment?

Overall, looks reasonable. Would you like to address the comments and sent the patch again,
or it will be more convenient if I commit the patch right now and you'll send further patches 
when you have the time?

Thanks,
Volodya