Summary: | Properly align members in WebInputEvent and subclasses to make Valgrind happy | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lei Zhang <thestig> | ||||||||||||||
Component: | UI Events | Assignee: | Tony Chang <tony> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, jamesr, tkent, tony, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Lei Zhang
2012-03-19 15:22:15 PDT
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. |