Bug 124579 – Use unsigned consistently, and check for invalid casts when calling into SharedBuffer from other code.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[László Langó]

Use unsigned consistently, and check for invalid casts when calling into SharedBuffer from other code.

[László Langó]

Created attachment 217294 [details]
Patch

[Build Bot]

Comment on attachment 217294 [details]
Patch

Attachment 217294 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/27118092

[Build Bot]

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

[László Langó]

Created attachment 217297 [details]
Patch

[Build Bot]

Comment on attachment 217297 [details]
Patch

Attachment 217297 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/26898113

[Build Bot]

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

[Darin Adler]

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?

[Brent Fulgham]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Thank you for looking into this. It's crazy that we still have a signed type here, and it's a very desirable improvement.

[Alexey Proskuryakov]

> To fix the build, please update WebCore.exp. 

More precisely, WebCore.exp.in.

[Brent Fulgham]

(In reply to comment #11)
> > To fix the build, please update WebCore.exp. 
> 
> More precisely, WebCore.exp.in.

Sorry for my misinformation!

[László Langó]

Created attachment 217429 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/30128097

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/29878111

[kov's GTK+ EWS bot]

Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/29798007

[Build Bot]

Comment on attachment 217429 [details]
Patch

Attachment 217429 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/30378007

[László Langó]

Created attachment 217523 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217523 [details]
Patch

Attachment 217523 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/32348051

[EFL EWS Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 217523 [details]
Patch

Attachment 217523 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/31998060

[Build Bot]

Comment on attachment 217523 [details]
Patch

Attachment 217523 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/31998059

[László Langó]

Created attachment 217529 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217529 [details]
Patch

Attachment 217529 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/31978066

[Build Bot]

Comment on attachment 217529 [details]
Patch

Attachment 217529 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/32018074

[EFL EWS Bot]

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

[László Langó]

Created attachment 217539 [details]
Patch

[Build Bot]

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

[Build Bot]

Comment on attachment 217539 [details]
Patch

Attachment 217539 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/31588099

[László Langó]

Created attachment 217544 [details]
Patch

[Alexey Proskuryakov]

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.

[László Langó]

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

[László Langó]

Created attachment 217779 [details]
Patch

[WebKit Commit Bot]

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

[László Langó]

Created attachment 220423 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 220423 [details]
patch for landing

Clearing flags on attachment: 220423

Committed r161338: <http://trac.webkit.org/changeset/161338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.