[Installer Test] Update chrome_helper.GetProcessIDAndPathPairs() to use psutil.

[Installer Test] Update chrome_helper.GetProcessIDAndPathPairs() to use psutil.

BUG=399643
Committed: https://crrev.com/02247c02b993b041e0da5a1f03e2f173ff81e888
Cr-Commit-Position: refs/heads/master@{#294652}

[huangs, 2014-09-12 14:18:03]

huangs@chromium.org changed reviewers:
+ grt@chromium.org

[huangs, 2014-09-12 14:18:04]

PTAL

[grt (UTC plus 2), 2014-09-12 15:01:12]

Thanks for taking a stab at this!

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
File chrome/test/mini_installer/chrome_helper.py (right):

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:7: import ctypes
unused

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:9: import pywintypes
unused

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:11: import win32con
unused

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:22:
process_id_and_path_pairs.append((process.pid, process.exe))
http://pythonhosted.org/psutil/#psutil.Process says: "When accessing methods of
this class always be prepared to catch NoSuchProcess and AccessDenied
exceptions." Please add another "except" clause to handle NoSuchProcess since
it's possible that the process terminates between the time the iterator returns
the Process instance and the pid/path are retrieved.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:22:
process_id_and_path_pairs.append((process.pid, process.exe))
process.exe -> process.exe()

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:23: except
psutil._error.AccessDenied:
perhaps just catch psutil.Error here, since that's the base for NoSuchProcess
and AccessDenied.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:42: def
GetWindowHandles(process_ids):
this looks unnecessarily racy for the case where it's called by
quit_chrome.py:CloseWindows(). i think it would be better if CloseWindows()
ended up doing something more like this (possibly indirectly via helper
functions in a pythonic way):
  for each hwnd,pid pair found by EnumWindows (dynamically, not caching all
pairs and then looping over them):
    process = psutil.Process(pid)
    if process.exe() == process_path and process.is_running():
      post WM_CLOSE to the hwnd
it may make sense to cache the Process instances (mapped by pid) while doing the
window iteration since each proc may have many, many hwnds. reset the item in
the cache if is_running() ever returns false, since that will mean the pid has
potentially been reused.

swing by to chat if this doesn't seem to make any sense. :-)

[huangs, 2014-09-12 18:25:33]

Updated.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
File chrome/test/mini_installer/chrome_helper.py (right):

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:7: import ctypes
On 2014/09/12 15:01:11, grt wrote:
> unused

Done.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:9: import pywintypes
On 2014/09/12 15:01:11, grt wrote:
> unused

Done.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:11: import win32con
On 2014/09/12 15:01:11, grt wrote:
> unused

Done.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:22:
process_id_and_path_pairs.append((process.pid, process.exe))
From
https://code.google.com/p/psutil/wiki/Documentation
.exe is a string.

I tested the code by printing the return value of GetProcessIDAndPathPairs(),
while 64-bit Chrome was running (also verified that the old code was not
working).

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:22:
process_id_and_path_pairs.append((process.pid, process.exe))
Catching psutil.Error, per suggested below.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:23: except
psutil._error.AccessDenied:
On 2014/09/12 15:01:11, grt wrote:
> perhaps just catch psutil.Error here, since that's the base for NoSuchProcess
> and AccessDenied.

Done.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:42: def
GetWindowHandles(process_ids):
EnumWindow() only looks at top-level windows, so there won't be 10,000 entries;
more like in the order of 100.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms633497(v=vs.85).aspx

Ideally we should use "yield".  Problem is win32gui.EnumWindows() does not allow
this.  Solving this would lead to overengineering, so per discussion, keeping
the array way...  :(

[grt (UTC plus 2), 2014-09-12 18:35:20]

Looks good to me. I've fired off a tryjob to see if the psutil on the bot
matches yours or the source I found online. Let's see how things go on the bot.

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
File chrome/test/mini_installer/chrome_helper.py (right):

https://codereview.chromium.org/565963002/diff/1/chrome/test/mini_installer/c...
chrome/test/mini_installer/chrome_helper.py:22:
process_id_and_path_pairs.append((process.pid, process.exe))
On 2014/09/12 18:25:33, huangs1 wrote:
> From
> https://code.google.com/p/psutil/wiki/Documentation
> .exe is a string.

Odd. http://pythonhosted.org/psutil/#psutil.Process.exe says it's a method, as
does https://code.google.com/p/psutil/source/browse/psutil/__init__.py#509

> I tested the code by printing the return value of GetProcessIDAndPathPairs(),
> while 64-bit Chrome was running (also verified that the old code was not
> working).

[huangs, 2014-09-12 18:47:58]

Peeking into psutil in depot_tools, exe() has annotation @cached_property:
https://pypi.python.org/pypi/cached-property/0.1.5

So looks like this is intended.

[grt (UTC plus 2), 2014-09-12 19:31:49]

lgtm

LGTM. Let 'er rip!

[grt (UTC plus 2), 2014-09-12 19:31:54]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2014-09-12 19:38:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565963002/20001

[commit-bot: I haz the power, 2014-09-12 20:41:06]

Committed patchset #2 (id:20001) as f09a84957d7d2ae920f86878b4ae5773d57eef57

[commit-bot: I haz the power, 2014-09-12 20:44:39]

Patchset 2 (id:??) landed as
https://crrev.com/02247c02b993b041e0da5a1f03e2f173ff81e888
Cr-Commit-Position: refs/heads/master@{#294652}