[wasm] Update WasmMemoryObject correctly when module memory is exported.

[wasm] Update WasmMemoryObject correctly when module memory is exported.

BUG=chromium:670683
R=titzer@chromium.org

Committed: https://crrev.com/0061089aa01d031c91373477dab422a35fb1d5fc
Cr-Commit-Position: refs/heads/master@{#41603}

[gdeepti, 2016-12-04 16:58:31]

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

[commit-bot: I haz the power, 2016-12-04 16:58:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gdeepti, 2016-12-04 17:19:01]

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

[commit-bot: I haz the power, 2016-12-04 17:19:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gdeepti, 2016-12-04 17:27:23]

gdeepti@chromium.org changed reviewers:
+ bradnelson@chromium.org, titzer@chromium.org
- titzer@cheomium.org

[gdeepti, 2016-12-04 17:27:45]

Description was changed from

==========
[wasm] Instance memory object should not be set when memory is exported.

BUG=chromium:670683

R=titzer@cheomium.org
==========

to

==========
[wasm] Instance memory object should not be set when memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

[gdeepti, 2016-12-04 17:27:45]

Description was changed from

==========
[wasm] Instance memory object should not be set when memory is exported.

BUG=chromium:670683

R=titzer@cheomium.org
==========

to

==========
[wasm] Instance memory object should not be set when memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

[commit-bot: I haz the power, 2016-12-04 17:33:23]

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

[commit-bot: I haz the power, 2016-12-04 17:33:24]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gdeepti, 2016-12-04 17:48:37]

https://codereview.chromium.org/2548223002/diff/20001/test/mjsunit/wasm/impor...
File test/mjsunit/wasm/import-memory.js (right):

https://codereview.chromium.org/2548223002/diff/20001/test/mjsunit/wasm/impor...
test/mjsunit/wasm/import-memory.js:384: assertEquals(kPageSize,
instance.exports.exported_mem.buffer.byteLength);
As per my understanding, this should be the correct behavior when memory is
exported as exports are returned at instantiation time. Ben, could you confirm?
Fails when guard pages are enabled, debugging bot failures.

[gdeepti, 2016-12-04 20:04:07]

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

[commit-bot: I haz the power, 2016-12-04 20:04:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gdeepti, 2016-12-04 20:07:05]

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

[commit-bot: I haz the power, 2016-12-04 20:07:18]

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, 2016-12-04 20:33:16]

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

[commit-bot: I haz the power, 2016-12-04 20:33:17]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-12-04 20:56:53]

Description was changed from

==========
[wasm] Instance memory object should not be set when memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

to

==========
[wasm] Instance memory object should not be set when memory is exported.
 - Exports are returned at instantiate, new memory object created when exports
are processed should not be set on the instance object. 
 - Use a different buffer for creating an exported memory object when guard
pages are enabled, so exported memory object does not have a handle to instance
memory. 

BUG=chromium:670683

R=titzer@chromium.org
==========

[gdeepti, 2016-12-04 20:56:53]

gdeepti@chromium.org changed reviewers:
+ eholk@chromium.org
- bradnelson@chromium.org

[titzer, 2016-12-05 09:40:54]

https://codereview.chromium.org/2548223002/diff/20001/test/mjsunit/wasm/impor...
File test/mjsunit/wasm/import-memory.js (right):

https://codereview.chromium.org/2548223002/diff/20001/test/mjsunit/wasm/impor...
test/mjsunit/wasm/import-memory.js:384: assertEquals(kPageSize,
instance.exports.exported_mem.buffer.byteLength);
On 2016/12/04 17:48:37, gdeepti wrote:
> As per my understanding, this should be the correct behavior when memory is
> exported as exports are returned at instantiation time. Ben, could you
confirm?
> Fails when guard pages are enabled, debugging bot failures. 

Yes, that's right.

[gdeepti, 2016-12-05 17:31:07]

https://codereview.chromium.org/2548223002/diff/20001/test/mjsunit/wasm/impor...
File test/mjsunit/wasm/import-memory.js (right):

https://codereview.chromium.org/2548223002/diff/20001/test/mjsunit/wasm/impor...
test/mjsunit/wasm/import-memory.js:384: assertEquals(kPageSize,
instance.exports.exported_mem.buffer.byteLength);
On 2016/12/05 09:40:54, titzer wrote:
> On 2016/12/04 17:48:37, gdeepti wrote:
> > As per my understanding, this should be the correct behavior when memory is
> > exported as exports are returned at instantiation time. Ben, could you
> confirm?
> > Fails when guard pages are enabled, debugging bot failures. 
> 
> Yes, that's right.

Thanks! Bot failures fixed with latest patch.

[Eric Holk, 2016-12-05 19:54:52]

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

https://codereview.chromium.org/2548223002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1794: if (!buffer.is_null() &&
buffer->has_guard_region()) {
We talked about this offline, but I'm summarizing here.

It's probably best to do this branch using wasm::NewArrayBuffer. To match the
old behavior, I think the right thing to do is to copy the old buffer into a
newly allocated buffer. Otherwise we'll end up with inconsistencies where once
the buffer grows sometimes it looks like the old size and sometimes it looks
like the new size.

[gdeepti, 2016-12-06 07:58:43]

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

[commit-bot: I haz the power, 2016-12-06 07:58:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gdeepti, 2016-12-06 08:12:51]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-12-06 08:13:19]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[gdeepti, 2016-12-06 08:14:37]

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

[commit-bot: I haz the power, 2016-12-06 08:14:49]

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, 2016-12-06 08:29:18]

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

[commit-bot: I haz the power, 2016-12-06 08:29:19]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gdeepti, 2016-12-07 06:07:12]

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

[commit-bot: I haz the power, 2016-12-07 06:07:19]

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, 2016-12-07 07:01:12]

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

[commit-bot: I haz the power, 2016-12-07 07:01:14]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-12-07 07:02:15]

Description was changed from

==========
[wasm] Instance memory object should not be set when memory is exported.
 - Exports are returned at instantiate, new memory object created when exports
are processed should not be set on the instance object. 
 - Use a different buffer for creating an exported memory object when guard
pages are enabled, so exported memory object does not have a handle to instance
memory. 

BUG=chromium:670683

R=titzer@chromium.org
==========

to

==========
[wasm] Update WasmMemoryObject correctly when memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

[gdeepti, 2016-12-07 07:02:40]

Description was changed from

==========
[wasm] Update WasmMemoryObject correctly when memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

to

==========
[wasm] Update WasmMemoryObject correctly when module memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

[gdeepti, 2016-12-07 07:10:34]

PTAL

Previous fix was incorrect, fixed now. Updated issue subject, test.

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

https://codereview.chromium.org/2548223002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1794: if (!buffer.is_null() &&
buffer->has_guard_region()) {
On 2016/12/05 19:54:52, Eric Holk wrote:
> We talked about this offline, but I'm summarizing here.
> 
> It's probably best to do this branch using wasm::NewArrayBuffer. To match the
> old behavior, I think the right thing to do is to copy the old buffer into a
> newly allocated buffer. Otherwise we'll end up with inconsistencies where once
> the buffer grows sometimes it looks like the old size and sometimes it looks
> like the new size.


Summarizing offline discussions, previous interpretation of the spec was
incorrect, exported memory objects reference instance memory, and should reflect
new size when grow_memory is called. Removed this block of code.

[Eric Holk, 2016-12-07 18:25:50]

lgtm from my standpoint.

[gdeepti, 2016-12-08 01:36:02]

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

[commit-bot: I haz the power, 2016-12-08 01:36: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, 2016-12-08 02:21:09]

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

[commit-bot: I haz the power, 2016-12-08 02:21:10]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-12-08 07:38:29]

Ping Brad/Ben for owners review.

[gdeepti, 2016-12-08 20:26:38]

On 2016/12/08 07:38:29, gdeepti wrote:
> Ping Brad/Ben for owners review.

Ping++ as chromium:671783, chromium:671761 and the issue referenced above are
because of the same root cause.

[bradnelson, 2016-12-08 20:28:11]

bradnelson@chromium.org changed reviewers:
+ bradnelson@chromium.org

[bradnelson, 2016-12-08 20:28:11]

The CQ bit was checked by bradnelson@chromium.org

[bradnelson, 2016-12-08 20:28:12]

lgtm

[bradnelson, 2016-12-08 20:28:12]

The patchset sent to the CQ was uploaded after l-g-t-m from eholk@chromium.org
Link to the patchset: https://codereview.chromium.org/2548223002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-08 20:28:21]

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, 2016-12-08 20:30:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481228891845380,
"parent_rev": "6b65acbbbe762f4a1cd77461a114aa17bb701768", "commit_rev":
"41ade8087df12be76155a17c8b4cba8f1574f38d"}

[commit-bot: I haz the power, 2016-12-08 20:30:25]

Description was changed from

==========
[wasm] Update WasmMemoryObject correctly when module memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

to

==========
[wasm] Update WasmMemoryObject correctly when module memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

[commit-bot: I haz the power, 2016-12-08 20:30:26]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-12-08 20:31:07]

Description was changed from

==========
[wasm] Update WasmMemoryObject correctly when module memory is exported.

BUG=chromium:670683

R=titzer@chromium.org
==========

to

==========
[wasm] Update WasmMemoryObject correctly when module memory is exported.

BUG=chromium:670683

R=titzer@chromium.org

Committed: https://crrev.com/0061089aa01d031c91373477dab422a35fb1d5fc
Cr-Commit-Position: refs/heads/master@{#41603}
==========

[commit-bot: I haz the power, 2016-12-08 20:31:08]

Patchset 7 (id:??) landed as
https://crrev.com/0061089aa01d031c91373477dab422a35fb1d5fc
Cr-Commit-Position: refs/heads/master@{#41603}