Make sure IE and Engine are actually being closed by custom action

Make sure IE and Engine are actually being closed by custom action

[Oleksandr, 2014-06-20 22:36:05]

We need to send the WM_ENDSESSION message to the IE frame window, which is
hosted in a process that does not load AdblockPlus.dll's. So instead of checking
for the DLL we are now checking if the process is immersive (Windows Store app
process)

[Eric, 2014-06-25 15:08:50]

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:27: HINSTANCE user32Dll =
LoadLibrary(L"user32.dll");
What's the reason for a dynamic load of this library? It's one of the oldest in
Windows. It's almost certainly already loaded. GetModuleHandle() would seem to
be more appropriate.

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:33: BOOL retValue =
!IsImmersiveDynamicCall(procHandle);
IsImmersiveDynamicCall commingles a false result with an error condition; both
return zero. As a rule, I'd expect to see a call to GetLastError(). In this
particular case, I'm not sure how much it matters. I think I'd pick a policy to
mask the error (what's already happening) but also log it to the installer log
file.

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:34: CloseHandle(procHandle);
Resource Leak. CloseHandle() won't be called under several conditions: exitCode
== STILL_ACTIVE, IsImmersiveDynamicCall == 0, etc. A simple wrapper class that
puts OpenProcess(...) in the constructor and CloseHandle in the destructor would
fix this and leave the body of this function clean.

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
File installer/src/installer-lib/test/process_test.cpp (left):

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/test/process_test.cpp:117:
process_by_any_exe_with_any_module find_in_set_w_non_existent(
multiple_name_set, non_existent_name_set_modules ) ;
These are filters that need updating. We don't need both of these at this point,
but we do need one of the. Something like 'find_in_set_not_immersive'

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/test/process_test.cpp:316: }
With the removal of these tests we're not exercising the new filter. If we have
'find_in_set_not_immersive', then it's easy to restore a test here.

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
File installer/src/installer-lib/test/process_test.cpp (right):

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/test/process_test.cpp:50: struct
our_process_by_name_subclassed
It appears that this struct is dead code. I know I was using it before, but
evidently neglected to remove it.

The upshot is that changing this function name does not actually invoke unit
tests on the new function.

[Oleksandr, 2014-06-26 13:25:57]

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
File installer/src/installer-lib/process.cpp (right):

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:27: HINSTANCE user32Dll =
LoadLibrary(L"user32.dll");
Why make assumptions? From MSDN
http://msdn.microsoft.com/en-us/library/windows/desktop/ms684175(v=vs.85).aspx:
"When no path is specified, the function searches for loaded modules whose base
name matches the base name of the module to be loaded. If the name matches, the
load succeeds."

On 2014/06/25 15:08:51, Eric wrote:
> What's the reason for a dynamic load of this library? It's one of the oldest
in
> Windows. It's almost certainly already loaded. GetModuleHandle() would seem to
> be more appropriate.

http://codereview.adblockplus.org/5663180676136960/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:33: BOOL retValue =
!IsImmersiveDynamicCall(procHandle);
I agree it wouldn't hurt to log the GetLastError here, but since we don't have a
session handle here, adding that log functionality will really just clutter a
lot of our code for no particular reason. I don't see when we could use that
error code anyway.

On 2014/06/25 15:08:51, Eric wrote:
> IsImmersiveDynamicCall commingles a false result with an error condition; both
> return zero. As a rule, I'd expect to see a call to GetLastError(). In this
> particular case, I'm not sure how much it matters. I think I'd pick a policy
to
> mask the error (what's already happening) but also log it to the installer log
> file.

[Eric, 2014-07-18 16:49:22]

Patch set #2 LGTM