Correct data size argument type in resource loading path to unsigned

Correct data size argument type in resource loading path to unsigned

int is used for ThreadableLoaderClient::didReceiveData()'s dataLength
argument and some of its implementations are calling strlen when
dataLength is -1. It never happens. We should give appropriate type to
the data path.

Chromium side CL: https://codereview.chromium.org/615823002/

R=tkent,kouhei
BUG=418885
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183121

[tyoshino (SeeGerritForStatus), 2014-09-30 07:16:22]

tyoshino@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2014-09-30 07:23:03]

https://codereview.chromium.org/618583002/diff/40001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/618583002/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoader.cpp:440: ASSERT(length >= 0);
is ASSERT safe enough? should we do RELEASE_ASSERT?

https://codereview.chromium.org/618583002/diff/40001/Source/core/html/ImageDo...
File Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/618583002/diff/40001/Source/core/html/ImageDo...
Source/core/html/ImageDocument.cpp:133: ASSERT(length <= UINT_MAX);
Can we have more reasonable limit for this?

https://codereview.chromium.org/618583002/diff/40001/Source/core/loader/Worke...
File Source/core/loader/WorkerLoaderClientBridge.cpp (right):

https://codereview.chromium.org/618583002/diff/40001/Source/core/loader/Worke...
Source/core/loader/WorkerLoaderClientBridge.cpp:79: ASSERT(vectorData->size() <=
UINT_MAX);
ditto

[kouhei (in TOK), 2014-09-30 07:41:22]

https://codereview.chromium.org/618583002/diff/40001/Source/core/html/ImageDo...
File Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/618583002/diff/40001/Source/core/html/ImageDo...
Source/core/html/ImageDocument.cpp:133: ASSERT(length <= UINT_MAX);
RELEASE_ASSERT(length <= std::numeric_limits<unsigned>::max());

[tyoshino (SeeGerritForStatus), 2014-09-30 08:14:28]

https://codereview.chromium.org/618583002/diff/40001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/618583002/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoader.cpp:440: ASSERT(length >= 0);
On 2014/09/30 07:23:03, kouhei wrote:
> is ASSERT safe enough? should we do RELEASE_ASSERT?

Agreed. When negative value is passed to appendData(), underflowed huge value
will be used for appending data to SharedBuffer. We should avoid it even in
release build.

https://codereview.chromium.org/618583002/diff/40001/Source/core/html/ImageDo...
File Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/618583002/diff/40001/Source/core/html/ImageDo...
Source/core/html/ImageDocument.cpp:133: ASSERT(length <= UINT_MAX);
On 2014/09/30 07:41:22, kouhei wrote:
> RELEASE_ASSERT(length <= std::numeric_limits<unsigned>::max());

Done.

https://codereview.chromium.org/618583002/diff/40001/Source/core/loader/Worke...
File Source/core/loader/WorkerLoaderClientBridge.cpp (right):

https://codereview.chromium.org/618583002/diff/40001/Source/core/loader/Worke...
Source/core/loader/WorkerLoaderClientBridge.cpp:79: ASSERT(vectorData->size() <=
UINT_MAX);
On 2014/09/30 07:23:03, kouhei wrote:
> ditto

Done.

[kouhei (in TOK), 2014-09-30 09:25:18]

lgtm

[tyoshino (SeeGerritForStatus), 2014-09-30 09:46:36]

+tkent

Kent-san, please take a look at platform/ and web/ dirs.

[tyoshino (SeeGerritForStatus), 2014-09-30 09:46:49]

tyoshino@chromium.org changed reviewers:
+ tkent@chromium.org

[tyoshino (SeeGerritForStatus), 2014-09-30 09:47:19]

+tkent

Kent-san, please take a look at platform/ and web/ dirs when you have time. This
CL is low priority.

[tkent, 2014-10-01 03:16:49]

Are you sure we don't need uint64_t?

https://codereview.chromium.org/618583002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/618583002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:445: void XMLHttpRequest::trackProgress(long
long length)
Why |long long|?

[kouhei (in TOK), 2014-10-01 03:28:58]

On 2014/10/01 03:16:49, tkent (high review load) wrote:
> Are you sure we don't need uint64_t?
Given the current limitation of renderer process memory in partition alloc, I
think unsigned is ok.

[tyoshino (SeeGerritForStatus), 2014-10-01 03:36:09]

https://codereview.chromium.org/618583002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/618583002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:445: void XMLHttpRequest::trackProgress(long
long length)
On 2014/10/01 03:16:49, tkent (high review load) wrote:
> Why |long long|?

The argument of didDownloadData() is still "int". This should also be fixed but
in another CL. Until that, I'd like to replace this with a type with larger
range not to care about overflow on trackProgress() call in didReceiveData()
while making no change on the path from didDownloadData() to trackProgress()
where int is passed to trackProgress().

[tkent, 2014-10-01 03:37:26]

ok, lgtm

[tyoshino (SeeGerritForStatus), 2014-10-01 03:42:27]

On 2014/10/01 03:28:58, kouhei wrote:
> On 2014/10/01 03:16:49, tkent (high review load) wrote:
> > Are you sure we don't need uint64_t?
> Given the current limitation of renderer process memory in partition alloc, I
> think unsigned is ok.

Agree with kouhei@. Might be worth doing for cleanness but there're some
functions expecting unsigned input. Replacing them with uint64_t requires more
consideration and inflates the CL bigger.

[tyoshino (SeeGerritForStatus), 2014-10-02 04:30:25]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2014-10-02 04:30:39]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2014-10-02 04:34:42]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-10-02 04:35:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618583002/80001

[commit-bot: I haz the power, 2014-10-02 04:40:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 04:40:38]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[tyoshino (SeeGerritForStatus), 2014-10-02 05:11:50]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-10-02 05:12:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618583002/80001

[commit-bot: I haz the power, 2014-10-02 05:17:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 05:18:01]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/25550)

[tyoshino (SeeGerritForStatus), 2014-10-02 05:37:59]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-10-02 05:38:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618583002/80001

[commit-bot: I haz the power, 2014-10-02 05:48:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 05:48:49]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[tyoshino (SeeGerritForStatus), 2014-10-02 06:04:27]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:05:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618583002/80001

[commit-bot: I haz the power, 2014-10-02 06:20:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:20:59]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[tyoshino (SeeGerritForStatus), 2014-10-02 06:27:23]

Added static_cast to RELEASE_ASSERT in
AssociatedURLLoader::ClientAdapter::didReceiveData to fix compilation error on
clang

[tyoshino (SeeGerritForStatus), 2014-10-02 06:27:30]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:28:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618583002/100001

[commit-bot: I haz the power, 2014-10-02 06:36:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:36:53]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)

[tyoshino (SeeGerritForStatus), 2014-10-02 06:37:50]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:38:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/618583002/100001

[commit-bot: I haz the power, 2014-10-02 08:07:44]

Committed patchset #6 (id:100001) as 183121