[wasm] Fix WasmMemoryObject constructor for when a module has no initial memory

[wasm] Fix WasmMemoryObject constructor for when a module has no initial memory

BUG=chromium:724972
R=clemensh@chromium.org, rossberg@chromium.org

Review-Url: https://codereview.chromium.org/2917603002
Cr-Commit-Position: refs/heads/master@{#45665}
Committed: https://chromium.googlesource.com/v8/v8/+/5c0baf7127cae07e1aa7dd8349eab9c4641a3b4f

[gdeepti, 2017-05-31 06:42:33]

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 06:42:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 07:11:14]

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

[commit-bot: I haz the power, 2017-05-31 07:11:15]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-05-31 07:56:37]

Would it make sense to allocate the JSArrayBuffer already for the wasm instance,
not just for the exported memory? That would remove even more special cases.

https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:473: uint32_t old_size =
old_buffer->byte_length()->Number();
Can you use "uint32_t old_size = 0;
CHECK(old_buffer->byte_length()->ToUInt32(&old_size);" here? We had problems
with too large memory buffers before, and the CHECK would catch it early.

https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:479: if (!old_buffer.is_null() && old_size != 0) {
old_buffer cannot be null here.

https://codereview.chromium.org/2917603002/diff/1/test/mjsunit/regress/wasm/r...
File test/mjsunit/regress/wasm/regression-727560.js (right):

https://codereview.chromium.org/2917603002/diff/1/test/mjsunit/regress/wasm/r...
test/mjsunit/regress/wasm/regression-727560.js:5: class Binary extends Array {
Please reduce this test case to also use wasm-module-builder.js.

[gdeepti, 2017-06-01 06:48:20]

The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 06:48:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gdeepti, 2017-06-01 07:01:26]

On 2017/05/31 07:56:37, Clemens Hammacher wrote:
> Would it make sense to allocate the JSArrayBuffer already for the wasm
instance,
> not just for the exported memory? That would remove even more special cases.
> 
> https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc
> File src/wasm/wasm-objects.cc (right):
> 
>
https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:473: uint32_t old_size =
> old_buffer->byte_length()->Number();
> Can you use "uint32_t old_size = 0;
> CHECK(old_buffer->byte_length()->ToUInt32(&old_size);" here? We had problems
> with too large memory buffers before, and the CHECK would catch it early.
> 
>
https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:479: if (!old_buffer.is_null() && old_size != 0) {
> old_buffer cannot be null here.
> 
>
https://codereview.chromium.org/2917603002/diff/1/test/mjsunit/regress/wasm/r...
> File test/mjsunit/regress/wasm/regression-727560.js (right):
> 
>
https://codereview.chromium.org/2917603002/diff/1/test/mjsunit/regress/wasm/r...
> test/mjsunit/regress/wasm/regression-727560.js:5: class Binary extends Array {
> Please reduce this test case to also use wasm-module-builder.js.

This particular CL fixes bugs (724972, 727560)  that were introduced in a recent
refactoring change, and affects GrowMemory calls through the JS API for
imported/exported memory. It makes sense for the runtime GrowMemory call to
behave the same way as well, but I'd prefer to keep refactoring/bug fixes in
separate CLs.

[gdeepti, 2017-06-01 07:01:38]

https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:473: uint32_t old_size =
old_buffer->byte_length()->Number();
On 2017/05/31 07:56:37, Clemens Hammacher wrote:
> Can you use "uint32_t old_size = 0;
> CHECK(old_buffer->byte_length()->ToUInt32(&old_size);" here? We had problems
> with too large memory buffers before, and the CHECK would catch it early.

Done.

https://codereview.chromium.org/2917603002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:479: if (!old_buffer.is_null() && old_size != 0) {
On 2017/05/31 07:56:37, Clemens Hammacher wrote:
> old_buffer cannot be null here.

Done.

https://codereview.chromium.org/2917603002/diff/1/test/mjsunit/regress/wasm/r...
File test/mjsunit/regress/wasm/regression-727560.js (right):

https://codereview.chromium.org/2917603002/diff/1/test/mjsunit/regress/wasm/r...
test/mjsunit/regress/wasm/regression-727560.js:5: class Binary extends Array {
On 2017/05/31 07:56:37, Clemens Hammacher wrote:
> Please reduce this test case to also use wasm-module-builder.js.

Done.

[commit-bot: I haz the power, 2017-06-01 07:18:38]

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

[commit-bot: I haz the power, 2017-06-01 07:18:39]

Dry run: This issue passed the CQ dry run.

[rossberg1, 2017-06-01 09:27:07]

LGTM

[Clemens Hammacher, 2017-06-01 10:16:10]

> This particular CL fixes bugs (724972, 727560)  that were introduced in a
recent refactoring change, and affects GrowMemory calls through the JS API for
imported/exported memory. It makes sense for the runtime GrowMemory call to
behave the same way as well, but I'd prefer to keep refactoring/bug fixes in
separate CLs.

Agreed. Even though I am not sure whether we are talking about the same future
refactoring here.
LGTM.

[gdeepti, 2017-06-01 17:05:46]

The CQ bit was checked by gdeepti@chromium.org

[commit-bot: I haz the power, 2017-06-01 17:05:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 17:08:02]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496336746176790,
"parent_rev": "c25744c23858421cf4d6b4502ae266cd27713457", "commit_rev":
"5c0baf7127cae07e1aa7dd8349eab9c4641a3b4f"}

[commit-bot: I haz the power, 2017-06-01 17:08:09]

Description was changed from

==========
[wasm] Fix WasmMemoryObject constructor for when a module has no initial memory

BUG=chromium:724972

R=clemensh@chromium.org, rossberg@chromium.org
==========

to

==========
[wasm] Fix WasmMemoryObject constructor for when a module has no initial memory

BUG=chromium:724972

R=clemensh@chromium.org, rossberg@chromium.org

Review-Url: https://codereview.chromium.org/2917603002
Cr-Commit-Position: refs/heads/master@{#45665}
Committed:
https://chromium.googlesource.com/v8/v8/+/5c0baf7127cae07e1aa7dd8349eab9c4641...
==========

[commit-bot: I haz the power, 2017-06-01 17:08:10]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/5c0baf7127cae07e1aa7dd8349eab9c4641...