Bug 124579 - Use unsigned consistently, and check for invalid casts when calling into SharedBuffer from other code.
Summary: Use unsigned consistently, and check for invalid casts when calling into Shar...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-19 06:03 PST by László Langó
Modified: 2014-01-06 03:14 PST (History)
13 users (show)

See Also:


Attachments
Patch (8.94 KB, patch)
2013-11-19 06:06 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (11.95 KB, patch)
2013-11-19 07:01 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (17.75 KB, patch)
2013-11-20 07:29 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (18.34 KB, patch)
2013-11-21 00:04 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (24.23 KB, patch)
2013-11-21 01:05 PST, László Langó
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (40.51 KB, patch)
2013-11-21 02:42 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (40.91 KB, patch)
2013-11-21 03:40 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (44.01 KB, patch)
2013-11-25 02:14 PST, László Langó
no flags Details | Formatted Diff | Diff
patch for landing (44.05 KB, patch)
2014-01-06 01:46 PST, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description László Langó 2013-11-19 06:03:41 PST
Use unsigned consistently, and check for invalid casts when calling into SharedBuffer from other code.
Comment 1 László Langó 2013-11-19 06:06:09 PST
Created attachment 217294 [details]
Patch
Comment 2 Build Bot 2013-11-19 06:19:34 PST
Comment on attachment 217294 [details]
Patch

Attachment 217294 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/27118092
Comment 3 Build Bot 2013-11-19 06:35:00 PST
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
Comment 4 László Langó 2013-11-19 07:01:54 PST
Created attachment 217297 [details]
Patch
Comment 5 Build Bot 2013-11-19 07:15:53 PST
Comment on attachment 217297 [details]
Patch

Attachment 217297 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/26898113
Comment 6 Build Bot 2013-11-19 07:36:34 PST
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 7 Darin Adler 2013-11-19 09:27:10 PST
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 8 Brent Fulgham 2013-11-19 09:32:39 PST
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 9 Alexey Proskuryakov 2013-11-19 10:16:29 PST
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.
Comment 10 Alexey Proskuryakov 2013-11-19 10:18:14 PST
Thank you for looking into this. It's crazy that we still have a signed type here, and it's a very desirable improvement.
Comment 11 Alexey Proskuryakov 2013-11-19 10:23:14 PST
> To fix the build, please update WebCore.exp. 

More precisely, WebCore.exp.in.
Comment 12 Brent Fulgham 2013-11-19 10:26:38 PST
(In reply to comment #11)
> > To fix the build, please update WebCore.exp. 
> 
> More precisely, WebCore.exp.in.

Sorry for my misinformation!
Comment 13 László Langó 2013-11-20 07:29:44 PST
Created attachment 217429 [details]
Patch
Comment 14 EFL EWS Bot 2013-11-20 07:38:36 PST
Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/30128097
Comment 15 EFL EWS Bot 2013-11-20 07:43:15 PST
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 16 Build Bot 2013-11-20 08:02:28 PST
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 17 Build Bot 2013-11-20 08:10:44 PST
Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/29878111
Comment 18 kov's GTK+ EWS bot 2013-11-20 08:14:20 PST
Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/29798007
Comment 19 Build Bot 2013-11-20 08:26:40 PST
Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/30378007
Comment 20 László Langó 2013-11-21 00:04:54 PST
Created attachment 217523 [details]
Patch
Comment 21 EFL EWS Bot 2013-11-21 00:15:24 PST
Comment on attachment 217523 [details]
Patch

Attachment 217523 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/32348051
Comment 22 EFL EWS Bot 2013-11-21 00:32:00 PST
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 23 Build Bot 2013-11-21 00:34:15 PST
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 24 Build Bot 2013-11-21 00:37:05 PST
Comment on attachment 217523 [details]
Patch

Attachment 217523 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/31998060
Comment 25 Build Bot 2013-11-21 01:03:15 PST
Comment on attachment 217523 [details]
Patch

Attachment 217523 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/31998059
Comment 26 László Langó 2013-11-21 01:05:50 PST
Created attachment 217529 [details]
Patch
Comment 27 EFL EWS Bot 2013-11-21 01:13:10 PST
Comment on attachment 217529 [details]
Patch

Attachment 217529 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/31978066
Comment 28 Build Bot 2013-11-21 01:16:42 PST
Comment on attachment 217529 [details]
Patch

Attachment 217529 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/32018074
Comment 29 EFL EWS Bot 2013-11-21 01:17:16 PST
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
Comment 30 László Langó 2013-11-21 02:42:43 PST
Created attachment 217539 [details]
Patch
Comment 31 Build Bot 2013-11-21 03:13:39 PST
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 32 Build Bot 2013-11-21 03:34:11 PST
Comment on attachment 217539 [details]
Patch

Attachment 217539 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/31588099
Comment 33 László Langó 2013-11-21 03:40:35 PST
Created attachment 217544 [details]
Patch
Comment 34 Alexey Proskuryakov 2013-11-21 09:27:43 PST
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.
Comment 35 László Langó 2013-11-22 03:03:55 PST
(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.
Comment 36 László Langó 2013-11-25 02:14:15 PST
Created attachment 217779 [details]
Patch
Comment 37 WebKit Commit Bot 2013-12-22 08:06:38 PST
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
Comment 38 László Langó 2014-01-06 01:46:00 PST
Created attachment 220423 [details]
patch for landing
Comment 39 WebKit Commit Bot 2014-01-06 03:13:54 PST
Comment on attachment 220423 [details]
patch for landing

Clearing flags on attachment: 220423

Committed r161338: <http://trac.webkit.org/changeset/161338>
Comment 40 WebKit Commit Bot 2014-01-06 03:14:00 PST
All reviewed patches have been landed.  Closing bug.