[wasm] Detach memory buffer only when GrowMemory is called from the JS API

[wasm] Detach memory buffer only when GrowMemory is called from the JS API

BUG=chromium:699485
R=ahaas@chromium.org, bradnelson@chromium.org

Review-Url: https://codereview.chromium.org/2772973002
Cr-Commit-Position: refs/heads/master@{#44166}
Committed: https://chromium.googlesource.com/v8/v8/+/c8b2656622faff4d0ee53c251c859deaa87f7e7a

[gdeepti, 2017-03-23 23:14:04]

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

[commit-bot: I haz the power, 2017-03-23 23:14:08]

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-03-23 23:32:17]

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

[commit-bot: I haz the power, 2017-03-23 23:32:18]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/23208)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/24867)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[gdeepti, 2017-03-23 23:52:50]

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

[commit-bot: I haz the power, 2017-03-23 23:53:29]

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-03-24 00:14:20]

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

[commit-bot: I haz the power, 2017-03-24 00:14:21]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/23212)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[gdeepti, 2017-03-24 00:34:09]

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

[commit-bot: I haz the power, 2017-03-24 00:34:13]

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-03-24 00:57:43]

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

[commit-bot: I haz the power, 2017-03-24 00:57:44]

Dry run: This issue passed the CQ dry run.

[bradnelson, 2017-03-24 03:49:46]

https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:2353: old_buffer->byte_length()->IsNumber()) {
Why is it ok to keep going below if length has been messed with?

https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:2435: old_buffer->byte_length()->IsNumber()) {
Same?

[ahaas, 2017-03-24 08:26:00]

For this CL it would have been interesting for me what the reason for the crash
was. Why should we detach the memory buffer only when GrowMemory is called from
the JS API.

https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:2353: old_buffer->byte_length()->IsNumber()) {
Could you add a comment why this IsNumber check is necessary?

[ahaas, 2017-03-24 08:42:09]

On 2017/03/24 at 08:26:00, ahaas wrote:
> For this CL it would have been interesting for me what the reason for the
crash was. Why should we detach the memory buffer only when GrowMemory is called
from the JS API.
> 
> https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:2353: old_buffer->byte_length()->IsNumber()) {
> Could you add a comment why this IsNumber check is necessary?

The code itself looks good to me, I just lack context.

[gdeepti, 2017-03-24 16:13:07]

On 2017/03/24 08:26:00, ahaas wrote:
> For this CL it would have been interesting for me what the reason for the
crash
> was. Why should we detach the memory buffer only when GrowMemory is called
from
> the JS API.
> 
> https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:2353: old_buffer->byte_length()->IsNumber()) {
> Could you add a comment why this IsNumber check is necessary?

When we start with an empty buffer as specified in the test (which I'm not sure
is really a use case in wasm, but can happen through JS), it crashes if
byte_length = NaN. We should Detach only from the JS API because that is as per
the Spec. The combination of the two was causing the test to crash.

[gdeepti, 2017-03-24 16:13:19]

https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2772973002/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:2353: old_buffer->byte_length()->IsNumber()) {
On 2017/03/24 08:26:00, ahaas wrote:
> Could you add a comment why this IsNumber check is necessary? 

GrowWebAssemblyMemory can be called through JS, i.e. with an ArrayBuffer with
size 0/NaN. In the case of byte_length = NaN, on some platforms the IsNumber()
check which is also a part of the Number() function fails, and causes a crash,
on other platforms it returns 0. If the IsNumber check fails, the byte_length is
forced to 0 (above), and Memory is Grown as if from no memory. This is mostly to
iron out platform specific behavior in terms of NaNs/Buffers with byte_length =
0, added comment in code as well. Same answer for question below.

[gdeepti, 2017-03-27 17:48:09]

On 2017/03/24 22:58:33, gdeepti wrote:

ptal

[bradnelson, 2017-03-27 22:35:50]

The CQ bit was checked by bradnelson@chromium.org

[bradnelson, 2017-03-27 22:35:51]

lgtm

[commit-bot: I haz the power, 2017-03-27 22:35:59]

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-03-27 22:59:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490654150608650,
"parent_rev": "b7e94287353ccb8adf76333840a2fc7bf1d00277", "commit_rev":
"c8b2656622faff4d0ee53c251c859deaa87f7e7a"}

[commit-bot: I haz the power, 2017-03-27 23:00:02]

Description was changed from

==========
[wasm] Detach memory buffer only when GrowMemory is called from the JS API

BUG=chromium:699485

R=ahaas@chromium.org, bradnelson@chromium.org
==========

to

==========
[wasm] Detach memory buffer only when GrowMemory is called from the JS API

BUG=chromium:699485

R=ahaas@chromium.org, bradnelson@chromium.org

Review-Url: https://codereview.chromium.org/2772973002
Cr-Commit-Position: refs/heads/master@{#44166}
Committed:
https://chromium.googlesource.com/v8/v8/+/c8b2656622faff4d0ee53c251c859deaa87...
==========

[commit-bot: I haz the power, 2017-03-27 23:00:03]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/c8b2656622faff4d0ee53c251c859deaa87...