Bug 128211 - &HWND should not be used as OLE_HANDLE*
Summary: &HWND should not be used as OLE_HANDLE*
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 8
: P2 Blocker
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-04 15:06 PST by Alex Christensen
Modified: 2014-02-11 13:19 PST (History)
3 users (show)

See Also:


Attachments
Patch (13.11 KB, patch)
2014-02-04 15:26 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
patch (17.57 KB, patch)
2014-02-11 12:51 PST, Alex Christensen
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2014-02-04 15:06:43 PST
An OLE_HANDLE is a UINT, which is always a 32-bit integer.  A HWND is a HWND__*, which is 32-bits for Win32 and 64-bits for Win64.  Casting between the two seems to be ok in most cases, and they're often used interchangeably without a problem.  There are several functions in WebKit that take an OLE_HANDLE* to put something into, and we're giving it a casted pointer to a HWND.  This does not cause a problem on Win32 because they're equivalent, but it puts a 32-bit integer at the beginning of a 64-bit integer with my Win64 builds, which causes some crashes sometimes.  This fixes those crashes.
Comment 1 Alex Christensen 2014-02-04 15:26:03 PST
Created attachment 223170 [details]
Patch
Comment 2 WebKit Commit Bot 2014-02-04 15:28:10 PST
Attachment 223170 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/win/TestRunnerWin.cpp:717:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebDropSource.cpp:102:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alex Christensen 2014-02-04 15:49:32 PST
I'll fix the spacing, but is it ok to use (HWND) style casting like everything else in this code, or should I change it to reinterpret_cast?
Comment 4 Brent Fulgham 2014-02-05 16:25:16 PST
(In reply to comment #3)
> I'll fix the spacing, but is it ok to use (HWND) style casting like everything else in this code, or should I change it to reinterpret_cast?

You should use static_cast/reinterpret_cast/const_cast, etc., as necessary to be clear about what's going on.

Thanks for working on this!
Comment 5 Alex Christensen 2014-02-05 20:30:36 PST
Would you be ok with me using helpers like PtrToUInt in windows-specific code?

http://msdn.microsoft.com/en-us/library/windows/desktop/aa384267(v=vs.85).aspx
Comment 6 Alex Christensen 2014-02-11 12:51:52 PST
Created attachment 223891 [details]
patch
Comment 7 WebKit Commit Bot 2014-02-11 12:54:48 PST
Attachment 223891 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/win/TestRunnerWin.cpp:717:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebDropSource.cpp:102:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Alex Christensen 2014-02-11 13:03:30 PST
I think the style failure is a bug, and I think reinterpret_cast is better than PtrToUInt because it does not assume that a OLE_HANDLE is an unsigned int.
Comment 9 Brent Fulgham 2014-02-11 13:04:45 PST
Comment on attachment 223891 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223891&action=review

Looks good, but I'd like the one C-cast to be changed to a C++ cast, please!  :-)

> Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp:66
> +    OLE_HANDLE nativeMenu = (OLE_HANDLE)menu->platformContextMenu();

reinterpret_cast<OLE_HANDLE>
Comment 10 Alex Christensen 2014-02-11 13:19:29 PST
http://trac.webkit.org/changeset/163903 with fixed c-cast and removed duplicate change log entry