[wasm] Do a proper HasProperty() check in the memory and table setup.

[wasm] Do a proper HasProperty() check in the memory and table setup.

The WebAssembly spec requires a HasProperty() check for the maximum
property of the descriptor object which is used to set up a
WebAssembly.Memory object or a WebAssembly.Table object.

The original implementation only approximated the HasProperty() check.
It used Get() to get the value of the maximum property of the descriptor
object and compared the resulting value to {undefined}. However, this
approximation is incorrect if the property exists but its value is
{undefined}.

R=titzer@chromium.org, franzih@chromium.org
BUG=chromium:649461
TEST=mjsunit/wasm/memory

Committed: https://crrev.com/7bffaaac2c2d3c638475ade4adba60f1b01dd66f
Cr-Commit-Position: refs/heads/master@{#39722}

[ahaas, 2016-09-23 13:50:37]

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

[commit-bot: I haz the power, 2016-09-23 13:50:50]

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

[Franzi, 2016-09-23 14:06:49]

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

https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode313
src/wasm/wasm-js.cc:313: v8::MaybeLocal<v8::Value> maybe = object->Get(context,
property);
You're still doing Object::Get(), which I believe can modify the object. Why
don't you use Object::Has()?

[commit-bot: I haz the power, 2016-09-23 15:02:26]

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

[commit-bot: I haz the power, 2016-09-23 15:02:27]

Dry run: This issue passed the CQ dry run.

[Franzi, 2016-09-23 17:23:28]

LGTM with nits and I'd like to see a test that fails without the patch. 

I find the commit message a bit confusing. How about something like this for the
last paragraph: 

If GetProperty() returns undefined, coerce undefined to 0 rather than treating
the property as absent.

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

https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode313
src/wasm/wasm-js.cc:313: v8::MaybeLocal<v8::Value> maybe = object->Get(context,
property);
On 2016/09/23 at 14:06:49, Franzi wrote:
> You're still doing Object::Get(), which I believe can modify the object. Why
don't you use Object::Has()?

Never mind, I found the Has() :P

https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode375
src/wasm/wasm-js.cc:375: bool has_maximum;
I'd prefer to inline maybe_has_max and is_just. 
I think you can do

Maybe<bool> has_maximum = desc->Has();
if (has_maximum.IsNothing()) return;
if (has_maximum.FromJust()) {
  // get integer property
}

https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode430
src/wasm/wasm-js.cc:430: bool is_just = maybe_has_maximum.To(&has_maximum);
Same as above.

https://codereview.chromium.org/2367673003/diff/1/test/mjsunit/wasm/memory.js
File test/mjsunit/wasm/memory.js (right):

https://codereview.chromium.org/2367673003/diff/1/test/mjsunit/wasm/memory.js...
test/mjsunit/wasm/memory.js:45: (function TestInitialIsUndefined() {
I think this test passes without your change as well. Can we add a test for
{maximum: undefined} which should now behave like {maximum: 0} and not anymore
like {}? 

And, just to be safe, we could add one, where initial and maximum are not an own
property but on the prototype.

If you want to go all out, write a test for an accessor. This way we know if we
call HasProperty() according to the spec. Something like this:

var a = true;
var desc = {};
Object.defineProperty(desc, 'maximum', {get: function() {
  if (a) {a = false; return 32766;}
  else {return -1;} // change the return value on the second call so it throws
}}); 
let memory = new WebAssembly.Memory(desc);

[ahaas, 2016-09-26 09:06:24]

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

[commit-bot: I haz the power, 2016-09-26 09:06:31]

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

[ahaas, 2016-09-26 09:16:52]

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

[commit-bot: I haz the power, 2016-09-26 09:16:58]

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

[ahaas, 2016-09-26 09:19:12]

PTAL, I added several tests which fail on ToT at the moment but work with this
CL.

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

https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode375
src/wasm/wasm-js.cc:375: bool has_maximum;
On 2016/09/23 at 17:23:28, Franzi wrote:
> I'd prefer to inline maybe_has_max and is_just. 
> I think you can do
> 
> Maybe<bool> has_maximum = desc->Has();
> if (has_maximum.IsNothing()) return;
> if (has_maximum.FromJust()) {
>   // get integer property
> }

Thanks, done.

https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode430
src/wasm/wasm-js.cc:430: bool is_just = maybe_has_maximum.To(&has_maximum);
On 2016/09/23 at 17:23:28, Franzi wrote:
> Same as above.

Done.

https://codereview.chromium.org/2367673003/diff/1/test/mjsunit/wasm/memory.js
File test/mjsunit/wasm/memory.js (right):

https://codereview.chromium.org/2367673003/diff/1/test/mjsunit/wasm/memory.js...
test/mjsunit/wasm/memory.js:45: (function TestInitialIsUndefined() {
On 2016/09/23 at 17:23:28, Franzi wrote:
> I think this test passes without your change as well. Can we add a test for
{maximum: undefined} which should now behave like {maximum: 0} and not anymore
like {}? 
> 
> And, just to be safe, we could add one, where initial and maximum are not an
own property but on the prototype.
> 
> If you want to go all out, write a test for an accessor. This way we know if
we call HasProperty() according to the spec. Something like this:
> 
> var a = true;
> var desc = {};
> Object.defineProperty(desc, 'maximum', {get: function() {
>   if (a) {a = false; return 32766;}
>   else {return -1;} // change the return value on the second call so it throws
> }}); 
> let memory = new WebAssembly.Memory(desc);

Done, and I also added many more tests.

[Franzi, 2016-09-26 09:25:22]

On 2016/09/26 at 09:19:12, ahaas wrote:
> PTAL, I added several tests which fail on ToT at the moment but work with this
CL.
> 
> https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc
> File src/wasm/wasm-js.cc (right):
> 
>
https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode375
> src/wasm/wasm-js.cc:375: bool has_maximum;
> On 2016/09/23 at 17:23:28, Franzi wrote:
> > I'd prefer to inline maybe_has_max and is_just. 
> > I think you can do
> > 
> > Maybe<bool> has_maximum = desc->Has();
> > if (has_maximum.IsNothing()) return;
> > if (has_maximum.FromJust()) {
> >   // get integer property
> > }
> 
> Thanks, done.
> 
>
https://codereview.chromium.org/2367673003/diff/1/src/wasm/wasm-js.cc#newcode430
> src/wasm/wasm-js.cc:430: bool is_just = maybe_has_maximum.To(&has_maximum);
> On 2016/09/23 at 17:23:28, Franzi wrote:
> > Same as above.
> 
> Done.
> 
> https://codereview.chromium.org/2367673003/diff/1/test/mjsunit/wasm/memory.js
> File test/mjsunit/wasm/memory.js (right):
> 
>
https://codereview.chromium.org/2367673003/diff/1/test/mjsunit/wasm/memory.js...
> test/mjsunit/wasm/memory.js:45: (function TestInitialIsUndefined() {
> On 2016/09/23 at 17:23:28, Franzi wrote:
> > I think this test passes without your change as well. Can we add a test for
{maximum: undefined} which should now behave like {maximum: 0} and not anymore
like {}? 
> > 
> > And, just to be safe, we could add one, where initial and maximum are not an
own property but on the prototype.
> > 
> > If you want to go all out, write a test for an accessor. This way we know if
we call HasProperty() according to the spec. Something like this:
> > 
> > var a = true;
> > var desc = {};
> > Object.defineProperty(desc, 'maximum', {get: function() {
> >   if (a) {a = false; return 32766;}
> >   else {return -1;} // change the return value on the second call so it
throws
> > }}); 
> > let memory = new WebAssembly.Memory(desc);
> 
> Done, and I also added many more tests.

LGTM

[commit-bot: I haz the power, 2016-09-26 09:40:43]

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

[commit-bot: I haz the power, 2016-09-26 09:40:44]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-09-26 13:04:35]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2016-09-26 13:04:42]

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-09-26 13:07:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-26 13:08:08]

Description was changed from

==========
[wasm] Do a proper HasProperty() check in the memory and table setup.

The WebAssembly spec requires a HasProperty() check for the maximum
property of the descriptor object which is used to set up a
WebAssembly.Memory object or a WebAssembly.Table object.

The original implementation only approximated the HasProperty() check.
It used Get() to get the value of the maximum property of the descriptor
object and compared the resulting value to {undefined}. However, this
approximation is incorrect if the property exists but its value is
{undefined}.


R=titzer@chromium.org, franzih@chromium.org
BUG=chromium:649461
TEST=mjsunit/wasm/memory
==========

to

==========
[wasm] Do a proper HasProperty() check in the memory and table setup.

The WebAssembly spec requires a HasProperty() check for the maximum
property of the descriptor object which is used to set up a
WebAssembly.Memory object or a WebAssembly.Table object.

The original implementation only approximated the HasProperty() check.
It used Get() to get the value of the maximum property of the descriptor
object and compared the resulting value to {undefined}. However, this
approximation is incorrect if the property exists but its value is
{undefined}.

R=titzer@chromium.org, franzih@chromium.org
BUG=chromium:649461
TEST=mjsunit/wasm/memory

Committed: https://crrev.com/7bffaaac2c2d3c638475ade4adba60f1b01dd66f
Cr-Commit-Position: refs/heads/master@{#39722}
==========

[commit-bot: I haz the power, 2016-09-26 13:08:09]

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