Fix issues with name and length on poison pill function

Fix issues with name and length on poison pill function

In ES6 function name and length are configurable. However, the length
and name properties of the poison pill function must not be
configurable.

BUG=v8:4011
LOG=N
R=adamk@chromium.org, rossberg@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

Committed: https://crrev.com/79be74364a40db15d831f4578617819bb391e465
Cr-Commit-Position: refs/heads/master@{#27855}

[arv (Not doing code reviews), 2015-04-07 19:47:26]

Use SetOwnPropertyIgnoreAttributes instead

[arv (Not doing code reviews), 2015-04-07 19:48:51]

arv@chromium.org changed reviewers:
+ caitpotter88@gmail.com

[arv (Not doing code reviews), 2015-04-07 19:48:51]

PTAL

[caitp (gmail), 2015-04-07 20:00:10]

https://codereview.chromium.org/1061393002/diff/20001/test/mjsunit/es6/throw-...
File test/mjsunit/es6/throw-type-error-function-restrictions.js (right):

https://codereview.chromium.org/1061393002/diff/20001/test/mjsunit/es6/throw-...
test/mjsunit/es6/throw-type-error-function-restrictions.js:7:
Object.getOwnPropertyDescriptor(f, 'arguments').get;
hmmm --- can this wait on crrev.com/1027283004 ? otherwise this assertion will
have to change and it will be touching more tests unrelated to it.

[adamk, 2015-04-07 20:02:58]

Mostly just some questions

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:630: JSReceiver::DeleteProperty(function,
factory()->name_string()).Assert();
So this'll put this into "slow" mode, but we don't really care since no one will
ever look at properties on this object?

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:634: CHECK_EQ(LookupIterator::ACCESSOR, it.state());
Seems like this could be a DCHECK_EQ just as well.

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:637: JSObject::SetOwnPropertyIgnoreAttributes(
So does this end up getting rid of the accessor? But it's OK because it will
never change?

[caitp (gmail), 2015-04-07 20:03:08]

On 2015/04/07 20:00:10, caitp wrote:
>
https://codereview.chromium.org/1061393002/diff/20001/test/mjsunit/es6/throw-...
> File test/mjsunit/es6/throw-type-error-function-restrictions.js (right):
> 
>
https://codereview.chromium.org/1061393002/diff/20001/test/mjsunit/es6/throw-...
> test/mjsunit/es6/throw-type-error-function-restrictions.js:7:
> Object.getOwnPropertyDescriptor(f, 'arguments').get;
> hmmm --- can this wait on crrev.com/1027283004 ? otherwise this assertion will
> have to change and it will be touching more tests unrelated to it.

otherwise lgtm

[arv (Not doing code reviews), 2015-04-07 20:10:17]

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:630: JSReceiver::DeleteProperty(function,
factory()->name_string()).Assert();
On 2015/04/07 20:02:58, adamk wrote:
> So this'll put this into "slow" mode, but we don't really care since no one
will
> ever look at properties on this object?

Yes. But that is fine since the would have to extract the accessor and use its
properties. There is no valid reason to do that.

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:634: CHECK_EQ(LookupIterator::ACCESSOR, it.state());
On 2015/04/07 20:02:58, adamk wrote:
> Seems like this could be a DCHECK_EQ just as well.

It had to be CHECK before when I was using lower level primitives. I'll change
it (if it works).

https://codereview.chromium.org/1061393002/diff/20001/src/bootstrapper.cc#new...
src/bootstrapper.cc:637: JSObject::SetOwnPropertyIgnoreAttributes(
On 2015/04/07 20:02:57, adamk wrote:
> So does this end up getting rid of the accessor? But it's OK because it will
> never change?

Yes. But let me expand the test to make sure that writing to it fails.

https://codereview.chromium.org/1061393002/diff/20001/test/mjsunit/es6/throw-...
File test/mjsunit/es6/throw-type-error-function-restrictions.js (right):

https://codereview.chromium.org/1061393002/diff/20001/test/mjsunit/es6/throw-...
test/mjsunit/es6/throw-type-error-function-restrictions.js:7:
Object.getOwnPropertyDescriptor(f, 'arguments').get;
On 2015/04/07 20:00:09, caitp wrote:
> hmmm --- can this wait on crrev.com/1027283004 ? otherwise this assertion will
> have to change and it will be touching more tests unrelated to it.

Yes. I was going to wait for your more substantial change to land.

[arv (Not doing code reviews), 2015-04-07 20:14:43]

No need to use a LookupIterator any more

[arv (Not doing code reviews), 2015-04-07 20:15:13]

PTAL

[adamk, 2015-04-07 21:02:47]

lgtm

[arv (Not doing code reviews), 2015-04-15 15:26:06]

Caitlin, can you take another look?

I refactored this a bit to work better with your CL.

One issue I found though is that we generate 2 different %ThrowTypeError%
functions so we can report different errors. The spec requires these to be the
same function. How important is the error message here?

[caitp (gmail), 2015-04-15 15:28:04]

On 2015/04/15 15:26:06, arv wrote:
> Caitlin, can you take another look?
> 
> I refactored this a bit to work better with your CL.
> 
> One issue I found though is that we generate 2 different %ThrowTypeError%
> functions so we can report different errors. The spec requires these to be the
> same function. How important is the error message here?

I think it's better if they use specific error messages.

What if we used some hack with IdentityHash() to make sure they look like the
same object?

[arv (Not doing code reviews), 2015-04-15 15:29:02]

On 2015/04/15 15:28:04, caitp wrote:
> On 2015/04/15 15:26:06, arv wrote:
> > Caitlin, can you take another look?
> > 
> > I refactored this a bit to work better with your CL.
> > 
> > One issue I found though is that we generate 2 different %ThrowTypeError%
> > functions so we can report different errors. The spec requires these to be
the
> > same function. How important is the error message here?
> 
> I think it's better if they use specific error messages.
> 
> What if we used some hack with IdentityHash() to make sure they look like the
> same object?

Let me file a new bug since this is not really important for this CL.

[caitp (gmail), 2015-04-15 15:33:30]

On 2015/04/15 15:29:02, arv wrote:
> On 2015/04/15 15:28:04, caitp wrote:
> > On 2015/04/15 15:26:06, arv wrote:
> > > Caitlin, can you take another look?
> > > 
> > > I refactored this a bit to work better with your CL.
> > > 
> > > One issue I found though is that we generate 2 different %ThrowTypeError%
> > > functions so we can report different errors. The spec requires these to be
> the
> > > same function. How important is the error message here?
> > 
> > I think it's better if they use specific error messages.
> > 
> > What if we used some hack with IdentityHash() to make sure they look like
the
> > same object?
> 
> Let me file a new bug since this is not really important for this CL.

The rest still looks good to me

[arv (Not doing code reviews), 2015-04-15 15:36:55]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2015-04-15 15:36:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
caitpotter88@gmail.com, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1061393002/#ps60001
(title: "git rebase and refactor")

[commit-bot: I haz the power, 2015-04-15 15:37:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061393002/60001

[commit-bot: I haz the power, 2015-04-15 17:15:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-15 17:15:52]

Patchset 4 (id:??) landed as
https://crrev.com/79be74364a40db15d831f4578617819bb391e465
Cr-Commit-Position: refs/heads/master@{#27855}