Currently, on 64-bit Linux, WebInputEvents have alignment issues with its data members that leave gaps in the in-memory representation. So even though one has initialized all the data members, Valgrind will still complain about uninitialized memory when Chromium readys WebInputEvents byte-by-byte when sending WebInputEvents over IPC. To fix this, I've rearranged the data member order and added dummy padding members as needed. To ensure this problem does not occur in the future, I've added COMPILE_ASSERT() checks for all the classes to make sure there's no gaps.
Created attachment 132684 [details] patch to fix the bug
Tony: can you review this?
Should we just use #pragma pack(push, 4) instead?
(In reply to comment #3) > Should we just use #pragma pack(push, 4) instead? I'm dumb. Will pointed out that we would still need to reorder variables to make sure doubles come before ints. Here's another idea from Will: Can we memset the struct before memcpy?
(In reply to comment #4) > (In reply to comment #3) > > Should we just use #pragma pack(push, 4) instead? > > I'm dumb. Will pointed out that we would still need to reorder variables to make sure doubles come before ints. > > Here's another idea from Will: Can we memset the struct before memcpy? The downside to using memset is that we cannot use initializer lists.
Comment on attachment 132684 [details] patch to fix the bug View in context: https://bugs.webkit.org/attachment.cgi?id=132684&action=review You're right, memset won't work in this case. Reordering seems unavoidable considering how content/public/common/webkit_param_traits.h handles WebInputEvent*. It might still be worth packing to 4 bytes to avoid having padding ints. > WebKit/chromium/public/WebInputEvent.h:225 > + sizeof(WebInputEvent) == (sizeof(WebInputEvent::timeStampSeconds) + > + sizeof(WebInputEvent::size) + We normally just make a reference struct and make sure the real object has the same size as the reference struct. See for example the SameSizeAsCSSValue class in WebKit/Source/WebCore/css/CSSValue.cpp. For your SameSizeAs* class, you can just put doubles in them.
Created attachment 132917 [details] address comments
ping.
Comment on attachment 132917 [details] address comments Sorry, missed the update. I'll get a patch that applies and put it in the commit queue.
Created attachment 135714 [details] Patch for landing
Created attachment 135715 [details] Patch for landing
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 135715 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/public/WebInputEvent.h:155: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/public/WebInputEvent.h:239: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebKit/chromium/public/WebInputEvent.h:407: input_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/public/WebInputEvent.h:411: keyboard_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/public/WebInputEvent.h:415: mouse_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/public/WebInputEvent.h:419: mousewheel_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/public/WebInputEvent.h:423: gesture_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit/chromium/public/WebInputEvent.h:427: touch_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 8 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 135724 [details] Patch
Comment on attachment 135724 [details] Patch Attachment 135724 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12340043
Alternate idea: can we make valgrind smarter?
(In reply to comment #16) > Alternate idea: can we make valgrind smarter? base/third_party/valgrind/memcheck.h has macros like VALGRIND_MAKE_MEM_DEFINED to say "trust me, this block of data is defined." But I think the problem is we would need to use it in every place that accesses these data structures byte by byte.
Comment on attachment 135724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135724&action=review > Source/WebKit/chromium/public/WebInputEvent.h:406 > +class SameSizeAsWebInputEvent { nit: consider putting this code into a nested namespace b/c these classes are never meant to be used by consumers of the WebKit API. WebKit::Internal:: perhaps?
Created attachment 135848 [details] Patch
Comment on attachment 135848 [details] Patch Clearing flags on attachment: 135848 Committed r113344: <http://trac.webkit.org/changeset/113344>
All reviewed patches have been landed. Closing bug.