Bug 81570 – Properly align members in WebInputEvent and subclasses to make Valgrind happy

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Lei Zhang]

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.

[Lei Zhang]

Created attachment 132684 [details]
patch to fix the bug

[Lei Zhang]

Tony: can you review this?

[Tony Chang]

Should we just use #pragma pack(push, 4) instead?

[Tony Chang]

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

[Lei Zhang]

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

[Tony Chang]

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.

[Lei Zhang]

Created attachment 132917 [details]
address comments

[Lei Zhang]

ping.

[Tony Chang]

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.

[Tony Chang]

Created attachment 135714 [details]
Patch for landing

[Tony Chang]

Created attachment 135715 [details]
Patch for landing

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Tony Chang]

Created attachment 135724 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135724 [details]
Patch

Attachment 135724 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12340043

[James Robinson]

Alternate idea: can we make valgrind smarter?

[Lei Zhang]

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

[Darin Fisher (:fishd, Google)]

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?

[Tony Chang]

Created attachment 135848 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135848 [details]
Patch

Clearing flags on attachment: 135848

Committed r113344: <http://trac.webkit.org/changeset/113344>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.