Make functions on the built-in object non-writable.

Make functions on the built-in object non-writable.

Committed: http://code.google.com/p/v8/source/detail?r=9096

[Kevin Millikin (Chromium), 2011-09-01 07:58:23]

LGTM if the comments below are addressed.

http://codereview.chromium.org/7736018/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7736018/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:777: __ mov(r1, Operand(Smi::FromInt(is_eval() ? 1 :
0)));
Don't you have to remove this argument?

http://codereview.chromium.org/7736018/diff/1/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/7736018/diff/1/src/messages.js#newcode32
src/messages.js:32: const TYPE_NATIVE = 0;
Is this change necessary?

If it is, can you instead define them as const macros in macros.py?  Our
implementation of const is slower than var, prevents optimization, and the
semantics of global const is somewhat up in the air and not something we should
rely on.

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1160
src/runtime.cc:1160: bool is_eval = (flags & kDeclareGlobalsEvalFlag) != 0;
These flags and 'base' below are declared a away from where they're used and
this is a big function.  You might consider moving them down toward their use.

If you keep the declarations up here, can you make them const so the reader
knows he doesn't have to look at all the intervening code between definition and
use?

I also think it's clearer to make strict_mode a bool and only convert it to the
enum at its single use.

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1246
src/runtime.cc:1246: PropertyAttributes attributes =
Here, you could just get rid of base and have the more direct:

int attr = NONE;
if (!is_eval) attr |= DONT_DELETE;
if (is_const_property || (is_native && is_function_declaration)) attr |=
READ_ONLY;

You can also move it down below the next check that doesn't use the attributes.

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1275
src/runtime.cc:1275: attributes = static_cast<PropertyAttributes>(
Here

attr |= (lookup.GetAttributes() & DONT_DELETE);

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1278
src/runtime.cc:1278: RETURN_IF_EMPTY_HANDLE(isolate,
Here:

SetLocalPropertyIgnoreAttributes(global,
                                 ...
                                 static_cast<PropertyAttributes>(attr));

http://codereview.chromium.org/7736018/diff/1/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/7736018/diff/1/src/v8globals.h#newcode485
src/v8globals.h:485: kNonStrictMode = 0,
I couldn't find a site where we relied on a specific values for kNonStrictMode
or kStrictMode, so I'd rather not make this change.

It suggests the reader here that we already *do* rely on a specific value and so
encourages him to make use of that fact.  There doesn't seem a good reason to
introduce that dependency.

[Lasse Reichstein, 2011-09-01 09:26:58]

http://codereview.chromium.org/7736018/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7736018/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:777: __ mov(r1, Operand(Smi::FromInt(is_eval() ? 1 :
0)));
Absolutely. I'm amazed that it seemed to work.
Fixed on Mips too.

http://codereview.chromium.org/7736018/diff/1/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/7736018/diff/1/src/messages.js#newcode32
src/messages.js:32: const TYPE_NATIVE = 0;
Nothing here is *necessary*, but it's an attempt to lock down the builtins
object to prevent tampering in case someone gets a hold of it (i.e., security in
depth).
Making them macros.py-macros is a better idea.

I do need some constants that can't be moved to macros.py, though (two of the
vars below have been changed in another CL). Is there a better way to make them
const than using "const" declarations? E.g., using %SetProperty(builtins, ...) ?

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1160
src/runtime.cc:1160: bool is_eval = (flags & kDeclareGlobalsEvalFlag) != 0;
On 2011/09/01 07:58:24, Kevin Millikin wrote:
> These flags and 'base' below are declared a away from where they're used and
> this is a big function.  You might consider moving them down toward their use.

Done.


> I also think it's clearer to make strict_mode a bool and only convert it to
the
> enum at its single use.

The entire declaration of strict_mode is moved to just before it's used.

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1246
src/runtime.cc:1246: PropertyAttributes attributes =
Good idea. Done!

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1275
src/runtime.cc:1275: attributes = static_cast<PropertyAttributes>(
Done.

http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1278
src/runtime.cc:1278: RETURN_IF_EMPTY_HANDLE(isolate,
Done.

http://codereview.chromium.org/7736018/diff/1/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/7736018/diff/1/src/v8globals.h#newcode485
src/v8globals.h:485: kNonStrictMode = 0,
Good catch. It's an artifact of a preliminary change that didn't make it.

[Kevin Millikin (Chromium), 2011-09-01 09:53:14]

http://codereview.chromium.org/7736018/diff/1/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/7736018/diff/1/src/messages.js#newcode32
src/messages.js:32: const TYPE_NATIVE = 0;
I think something like that, even if you have to implement a custom function,
might be better.

If we match Safari's behavior, we'll have surprising stuff like consts whose
value can change (a global const declaration overrides a pre-existing one).