Bug 48515 – [NRWT] Clear invalid http locks on Windows platform as well

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gabor Rapcsanyi]

The HttpLock doesn't clear the stuck lock files on Windows platform.

[Gabor Rapcsanyi]

Created attachment 72674 [details]
proposed patch

[Peter Gal]

> 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',)

[Tony Chang]

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?

[Peter Gal]

(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.

[Eric Seidel (no email)]

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.

[Tony Chang]

(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

[Gabor Rapcsanyi]

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?

[Tony Chang]

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?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

CCing aroben since he's recently been workign with getting webkit-patch working under win32.

[Gabor Rapcsanyi]

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.

[Gabor Rapcsanyi]

(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.

[Eric Seidel (no email)]

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.

[Csaba Osztrogonác]

Modified patch landed: http://trac.webkit.org/changeset/71338