Summary: | Use unsigned consistently, and check for invalid casts when calling into SharedBuffer from other code. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | László Langó <llango.u-szeged> | ||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ap, bfulgham, buildbot, commit-queue, eflews.bot, gtk-ews, gyuyoung.kim, japhet, ossy, philn, rego+ews, rniwa, xan.lopez | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
László Langó
2013-11-19 06:03:41 PST
Created attachment 217294 [details]
Patch
Comment on attachment 217294 [details] Patch Attachment 217294 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/27118092 Comment on attachment 217294 [details] Patch Attachment 217294 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/27578006 Created attachment 217297 [details]
Patch
Comment on attachment 217297 [details] Patch Attachment 217297 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/26898113 Comment on attachment 217297 [details] Patch Attachment 217297 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/27538008 Comment on attachment 217297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217297&action=review > Source/WebCore/ChangeLog:3 > + Use unsigned consistently, and check for invalid casts when calling into SharedBuffer from other code. On 64-bit platforms, this changes from 32-bit to 64-bit, not just from signed to unsigned. Rationale? Comment on attachment 217297 [details]
Patch
You changed the type signature of some symbols that are exported by the WebCore.order file. Update the entry for __ZN7WebCore12SharedBufferC1EPKci to match the new __ZN7WebCore12SharedBufferC1EPKhi signature.
Comment on attachment 217297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217297&action=review To fix the build, please update WebCore.exp. There is no need to change WebCore.order, even if it gets out of date - it's generated from profile data when needed. But I think that this patch doesn't fix all the code paths where an implicit cast occurs. > Source/WebCore/loader/ResourceLoader.cpp:193 > -void ResourceLoader::addDataOrBuffer(const char* data, int length, SharedBuffer* buffer, DataPayloadType dataPayloadType) > +void ResourceLoader::addDataOrBuffer(const char* data, size_t length, SharedBuffer* buffer, DataPayloadType dataPayloadType) The patch changes this function, but doesn't change call sites. Specifically, ResourceLoader::didReceiveDataOrBuffer still calls this function with an int length. Thank you for looking into this. It's crazy that we still have a signed type here, and it's a very desirable improvement. > To fix the build, please update WebCore.exp.
More precisely, WebCore.exp.in.
(In reply to comment #11) > > To fix the build, please update WebCore.exp. > > More precisely, WebCore.exp.in. Sorry for my misinformation! Created attachment 217429 [details]
Patch
Comment on attachment 217429 [details] Patch Attachment 217429 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/30128097 Comment on attachment 217429 [details] Patch Attachment 217429 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/29688102 Comment on attachment 217429 [details] Patch Attachment 217429 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/29888098 Comment on attachment 217429 [details] Patch Attachment 217429 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/29878111 Comment on attachment 217429 [details] Patch Attachment 217429 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/29798007 Comment on attachment 217429 [details] Patch Attachment 217429 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/30378007 Created attachment 217523 [details]
Patch
Comment on attachment 217523 [details] Patch Attachment 217523 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/32348051 Comment on attachment 217523 [details] Patch Attachment 217523 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/31728015 Comment on attachment 217523 [details] Patch Attachment 217523 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/31768009 Comment on attachment 217523 [details] Patch Attachment 217523 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/31998060 Comment on attachment 217523 [details] Patch Attachment 217523 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/31998059 Created attachment 217529 [details]
Patch
Comment on attachment 217529 [details] Patch Attachment 217529 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/31978066 Comment on attachment 217529 [details] Patch Attachment 217529 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/32018074 Comment on attachment 217529 [details] Patch Attachment 217529 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/31588073 Created attachment 217539 [details]
Patch
Comment on attachment 217539 [details] Patch Attachment 217539 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/32348077 Comment on attachment 217539 [details] Patch Attachment 217539 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/31588099 Created attachment 217544 [details]
Patch
Comment on attachment 217544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217544&action=review There are even more threads that need to be pulled :( See for example this line in WebCoreResourceHandleAsDelegate.mm: m_handle->client()->willStopBufferingData(m_handle, (const char*)[data bytes], static_cast<int>([data length])); willStopBufferingData is the first and only name I grepped for, so probabilistically speaking, there are likely more implicit casts remaining. Please grep WebCore for every function name you changed. > Source/WebCore/loader/ResourceLoader.cpp:1 > -/* > + /* Accidental change here. (In reply to comment #34) > (From update of attachment 217544 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217544&action=review > > There are even more threads that need to be pulled :( > > See for example this line in WebCoreResourceHandleAsDelegate.mm: > > m_handle->client()->willStopBufferingData(m_handle, (const char*)[data bytes], static_cast<int>([data length])); > > willStopBufferingData is the first and only name I grepped for, so probabilistically speaking, there are likely more implicit casts remaining. Please grep WebCore for every function name you changed. Sorry, I missed the *.mm files. I only grepped in *.cpp and *.h files. :s I will check it twice before the next upload. I didn't expected that this change will affect so many files. Created attachment 217779 [details]
Patch
Comment on attachment 217779 [details] Patch Rejecting attachment 217779 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 217779, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: patching file Source/WebKit/win/WebKitDLL.cpp patching file Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp Hunk #1 succeeded at 191 (offset -1 lines). Hunk #2 succeeded at 393 (offset -1 lines). patching file Source/WebKit2/NetworkProcess/NetworkResourceLoader.h patching file Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Anders Carlsson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/51728022 Created attachment 220423 [details]
patch for landing
Comment on attachment 220423 [details] patch for landing Clearing flags on attachment: 220423 Committed r161338: <http://trac.webkit.org/changeset/161338> All reviewed patches have been landed. Closing bug. |