The HttpLock doesn't clear the stuck lock files on Windows platform.
Created attachment 72674 [details] proposed patch
> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:80 > + elif sys.platform in ('win32'): I think this isn't really what you wanted. You wanted to check if the sys.platform is in the tuple, but the right side is not a tuple. So this is the same as this: " sys.platform in 'win32' " which is clearly incorrect. In this case to create a tuple you should add a trailing comma: ('win32',)
Comment on attachment 72674 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=72674&action=review r- for sys.platform bug. > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:81 > + from win32com.client import GetObject I don't think this is a standard module. The Chromium win bots don't have it. Maybe we can install it using autopy?
(In reply to comment #3) > I don't think this is a standard module. The Chromium win bots don't have it. Maybe we can install it using autopy? That's strange, because the win32 api is also used by the buildbot and there are other references to win32 api in the WebKit trunk.
I do think win32all is a separate install, but I can't say I'm a true win32 python expert. webkitpy.thirdparty.autoinstalled.__init__ should do the trick for you though.
(In reply to comment #4) > (In reply to comment #3) > > I don't think this is a standard module. The Chromium win bots don't have it. Maybe we can install it using autopy? > > That's strange, because the win32 api is also used by the buildbot and there are other references to win32 api in the WebKit trunk. win32com isn't listed in the global module index: http://docs.python.org/modindex.html
Created attachment 72823 [details] proposed_patch_v2 This is another solution without win32com module. It's a little bit longer but it needs just the ctypes module. I also put some debug messages to it. What do you think about this?
Comment on attachment 72823 [details] proposed_patch_v2 View in context: https://bugs.webkit.org/attachment.cgi?id=72823&action=review In general, I think this approach is fine. > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:102 > + CreateToolhelp32Snapshot = ctypes.windll.kernel32.CreateToolhelp32Snapshot > + Process32First = ctypes.windll.kernel32.Process32First > + Process32Next = ctypes.windll.kernel32.Process32Next Can we move this win32 code into a separate method? Maybe _check_pid_win32? Also, I would declare the PROESSENTRY32 class inside this method. It seems like you could then unittest it (makes sure it kills a pid and make sure it doesn't infinite loop if the pid is not found). > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:103 > + CloseHandle = ctypes.windll.kernel32.CloseHandle Were you planning on using this? Do we need to close something to avoid a leak?
Please move all this logic to webkitpy.common.system.executive.py. That's our subprocess wrapper. As tony said, the win32 stuff needs to be encapsulated into its own method (or possibly its own class) and unit tested.
CCing aroben since he's recently been workign with getting webkit-patch working under win32.
Created attachment 72944 [details] proposed_patch_v3 I put check_running_pid() method to webkitpy.common.system.executive.py as Eric said and I created a unit test for it.
(In reply to comment #8) > > > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:103 > > + CloseHandle = ctypes.windll.kernel32.CloseHandle > > Were you planning on using this? Do we need to close something to avoid a leak? In check_running_pid() I'm using it now I just miss that, thanks for the remark.
Comment on attachment 72944 [details] proposed_patch_v3 View in context: https://bugs.webkit.org/attachment.cgi?id=72944&action=review This looks great! Thank you! We could break the windows check_running_pid out into a helper _win32_check_running_pid() method since it's so long. But it's also probably OK for now as is. > WebKitTools/Scripts/webkitpy/common/system/executive.py:252 > + We should probably assert(false) here, since we're handling a case we dont' expect.
Modified patch landed: http://trac.webkit.org/changeset/71338