Remove duplication and unnecessary HandleScope from HasElement helper functions

Remove duplication and unnecessary HandleScope from HasElement helper functions

Committed: https://code.google.com/p/v8/source/detail?r=13736

[rossberg, 2013-02-25 10:46:25]

https://codereview.chromium.org/12328064/diff/1/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
src/objects.cc:3373: // Handle [] on String objects.
I'm not sure removing this is correct. This function is also called by
GetPropertyAttributeWithReceiver, and on that path, I don't think strings would
be handled otherwise.

[Michael Starzinger, 2013-02-25 14:29:08]

The removal of the HandleScope itself looks good.

https://codereview.chromium.org/12328064/diff/1/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
src/objects.cc:3373: // Handle [] on String objects.
On 2013/02/25 10:46:25, rossberg wrote:
> I'm not sure removing this is correct. This function is also called by
> GetPropertyAttributeWithReceiver, and on that path, I don't think strings
would
> be handled otherwise.

I agree with this comment. I think this can be triggered by checking "'0' in o"
where o has an interceptor and it's prototype is a string. But that is just
speculation.

[rossberg, 2013-02-25 14:43:40]

On 2013/02/25 14:29:08, Michael Starzinger wrote:
> The removal of the HandleScope itself looks good.
> 
> https://codereview.chromium.org/12328064/diff/1/src/objects.cc
> File src/objects.cc (left):
> 
> https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
> src/objects.cc:3373: // Handle [] on String objects.
> On 2013/02/25 10:46:25, rossberg wrote:
> > I'm not sure removing this is correct. This function is also called by
> > GetPropertyAttributeWithReceiver, and on that path, I don't think strings
> would
> > be handled otherwise.
> 
> I agree with this comment. I think this can be triggered by checking "'0' in
o"
> where o has an interceptor and it's prototype is a string. But that is just
> speculation.

You cannot actually have a string as a prototype. I think the example in
question would be:

  "0" in ""

[Michael Starzinger, 2013-02-25 14:51:35]

On 2013/02/25 14:43:40, rossberg wrote:
> On 2013/02/25 14:29:08, Michael Starzinger wrote:
> > The removal of the HandleScope itself looks good.
> > 
> > https://codereview.chromium.org/12328064/diff/1/src/objects.cc
> > File src/objects.cc (left):
> > 
> > https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
> > src/objects.cc:3373: // Handle [] on String objects.
> > On 2013/02/25 10:46:25, rossberg wrote:
> > > I'm not sure removing this is correct. This function is also called by
> > > GetPropertyAttributeWithReceiver, and on that path, I don't think strings
> > would
> > > be handled otherwise.
> > 
> > I agree with this comment. I think this can be triggered by checking "'0' in
> o"
> > where o has an interceptor and it's prototype is a string. But that is just
> > speculation.
> 
> You cannot actually have a string as a prototype. I think the example in
> question would be:
> 
>   "0" in ""

It is true that a string primitive cannot be used as a prototype, but a string
value wrapper can be. And there is a call-path that leads to this site through
the post-interceptor lookup on the prototype chain. So the following should
reproduce the issue ...

var s = new String('foobar');
var o = { __proto__:s };
print('0' in o);

... if "o" also has an interceptor. But then again, I am still speculating here,
and I am too lazy to write a cctest to prove my point. :)

[adamk, 2013-02-25 19:36:03]

https://codereview.chromium.org/12328064/diff/1/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
src/objects.cc:3373: // Handle [] on String objects.
On 2013/02/25 10:46:25, rossberg wrote:
> I'm not sure removing this is correct. This function is also called by
> GetPropertyAttributeWithReceiver, and on that path, I don't think strings
would
> be handled otherwise.

I've added a test case covering this case, and it still passes. The reason is
that, although I've removed the check here, the very next thing this code does
is call GetElementAttributeWithoutInterceptor, which itself does the
IsStringObjectWithCharacterAt check. Please take a look at the test case and see
if there's other stuff that I need to cover.

[rossberg, 2013-02-26 12:01:45]

On 2013/02/25 19:36:03, adamk wrote:
> https://codereview.chromium.org/12328064/diff/1/src/objects.cc
> File src/objects.cc (left):
> 
> https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
> src/objects.cc:3373: // Handle [] on String objects.
> On 2013/02/25 10:46:25, rossberg wrote:
> > I'm not sure removing this is correct. This function is also called by
> > GetPropertyAttributeWithReceiver, and on that path, I don't think strings
> would
> > be handled otherwise.
> 
> I've added a test case covering this case, and it still passes. The reason is
> that, although I've removed the check here, the very next thing this code does
> is call GetElementAttributeWithoutInterceptor, which itself does the
> IsStringObjectWithCharacterAt check. Please take a look at the test case and
see
> if there's other stuff that I need to cover.

Hm, I'm not sure what the simplest JS test is then, but I'm pretty certain that
the following API code will fail after the change:

  Handle<String> s = String::New("JS rocks so hard");
  ASSERT(s->Has(1));

[adamk, 2013-02-26 16:19:30]

On 2013/02/26 12:01:45, rossberg wrote:
> On 2013/02/25 19:36:03, adamk wrote:
> > https://codereview.chromium.org/12328064/diff/1/src/objects.cc
> > File src/objects.cc (left):
> > 
> > https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
> > src/objects.cc:3373: // Handle [] on String objects.
> > On 2013/02/25 10:46:25, rossberg wrote:
> > > I'm not sure removing this is correct. This function is also called by
> > > GetPropertyAttributeWithReceiver, and on that path, I don't think strings
> > would
> > > be handled otherwise.
> > 
> > I've added a test case covering this case, and it still passes. The reason
is
> > that, although I've removed the check here, the very next thing this code
does
> > is call GetElementAttributeWithoutInterceptor, which itself does the
> > IsStringObjectWithCharacterAt check. Please take a look at the test case and
> see
> > if there's other stuff that I need to cover.
> 
> Hm, I'm not sure what the simplest JS test is then, but I'm pretty certain
that
> the following API code will fail after the change:
> 
>   Handle<String> s = String::New("JS rocks so hard");
>   ASSERT(s->Has(1));

I assume you mean StringObject here, not String? Anyway, that works fine (I can
upload a test case when I get in; looks like StringObject doesn't have any API
coverage).

Perhaps there's some confusion here? The only way the code I'm removing could
have an impact is if, somehow, you managed to have a JSValue representing a
String that _also_ managed to have elements for the same indices as the String
(in the same Object, not in a prototype), and those elements had different
attributes. But I don't think this is possible, since it's not possible to add
elements to a JSValue _before_ creation, and after creation those indices are
read-only.

[rossberg, 2013-02-26 16:36:22]

On 2013/02/26 16:19:30, adamk wrote:
> On 2013/02/26 12:01:45, rossberg wrote:
> > On 2013/02/25 19:36:03, adamk wrote:
> > > https://codereview.chromium.org/12328064/diff/1/src/objects.cc
> > > File src/objects.cc (left):
> > > 
> > > https://codereview.chromium.org/12328064/diff/1/src/objects.cc#oldcode3373
> > > src/objects.cc:3373: // Handle [] on String objects.
> > > On 2013/02/25 10:46:25, rossberg wrote:
> > > > I'm not sure removing this is correct. This function is also called by
> > > > GetPropertyAttributeWithReceiver, and on that path, I don't think
strings
> > > would
> > > > be handled otherwise.
> > > 
> > > I've added a test case covering this case, and it still passes. The reason
> is
> > > that, although I've removed the check here, the very next thing this code
> does
> > > is call GetElementAttributeWithoutInterceptor, which itself does the
> > > IsStringObjectWithCharacterAt check. Please take a look at the test case
and
> > see
> > > if there's other stuff that I need to cover.
> > 
> > Hm, I'm not sure what the simplest JS test is then, but I'm pretty certain
> that
> > the following API code will fail after the change:
> > 
> >   Handle<String> s = String::New("JS rocks so hard");
> >   ASSERT(s->Has(1));
> 
> I assume you mean StringObject here, not String? Anyway, that works fine (I
can
> upload a test case when I get in; looks like StringObject doesn't have any API
> coverage).

Yes, sorry, I managed to forget that wrapping has to be done manually outside JS
land :). So more like:

  Handle<String> s = String::New("JS rocks so hard");
  Handle<StringObject> o = StringObject::New(s);
  ASSERT(o->Has(1));

> Perhaps there's some confusion here? The only way the code I'm removing could
> have an impact is if, somehow, you managed to have a JSValue representing a
> String that _also_ managed to have elements for the same indices as the String
> (in the same Object, not in a prototype), and those elements had different
> attributes. But I don't think this is possible, since it's not possible to add
> elements to a JSValue _before_ creation, and after creation those indices are
> read-only.

I'm not sure I follow. AFAICS, JSReceiver::HasElement calls
JSObject::GetElementAttributeWithReceiver, which is where you removed the
special case for string objects. And the call to Has in the example above should
take that path, shouldn't it? If not, I'm indeed confused, where on earth is it
going?

[adamk, 2013-02-26 16:47:15]

[resending to capture this on the codereview]

The call sequence is:

JSReceiver::HasElement
JSObject::GetElementAttributeWithReceiver
JSObject::GetElementAttributeWithoutInterceptor

GetElementAttributeWithoutInterceptor() does two things:

1. Check if the index is present in the element backing store
2. If not, check if this is a string object with a character at that index

The call sequence for "'0' in o" is:

JSReceiver::HasProperty
JSObject::GetPropertyAttribute
JSObject::GetPropertyAttributeWithReceiver
JSObject::GetElementAttributeWithReceiver
JSObject::GetElementAttributeWithoutInterceptor

I think I shot myself in the foot here by removing the HandleScope from
GetElementAttributeWithoutInterceptor in the same change, which obscures the
fact that its call to IsStringObjectWithCharacterAt remains unchanged.

[rossberg, 2013-02-26 17:00:15]

On 2013/02/26 16:47:15, adamk wrote:
> [resending to capture this on the codereview]
> 
> The call sequence is:
> 
> JSReceiver::HasElement
> JSObject::GetElementAttributeWithReceiver
> JSObject::GetElementAttributeWithoutInterceptor
> 
> GetElementAttributeWithoutInterceptor() does two things:
> 
> 1. Check if the index is present in the element backing store
> 2. If not, check if this is a string object with a character at that index
> 
> The call sequence for "'0' in o" is:
> 
> JSReceiver::HasProperty
> JSObject::GetPropertyAttribute
> JSObject::GetPropertyAttributeWithReceiver
> JSObject::GetElementAttributeWithReceiver
> JSObject::GetElementAttributeWithoutInterceptor
> 
> I think I shot myself in the foot here by removing the HandleScope from
> GetElementAttributeWithoutInterceptor in the same change, which obscures the
> fact that its call to IsStringObjectWithCharacterAt remains unchanged.

Oh. You are right, I _was_ confused. :) In that case, LGTM