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

src/runtime.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1131 matching lines @@
   Handle<Object> type_handle =
       isolate->factory()->NewStringFromAscii(CStrVector(type));
   Handle<Object> args[2] = { type_handle, name };
   Handle<Object> error =
       isolate->factory()->NewTypeError("redeclaration", HandleVector(args, 2));
   return isolate->Throw(*error);
 }
 
 
 RUNTIME_FUNCTION(MaybeObject*, Runtime_DeclareGlobals) {
-  ASSERT(args.length() == 4);
+  ASSERT(args.length() == 3);
   HandleScope scope(isolate);
   Handle<GlobalObject> global = Handle<GlobalObject>(
       isolate->context()->global());
 
   Handle<Context> context = args.at<Context>(0);
   CONVERT_ARG_CHECKED(FixedArray, pairs, 1);
-  bool is_eval = args.smi_at(2) == 1;
-  StrictModeFlag strict_mode = static_cast<StrictModeFlag>(args.smi_at(3));
-  ASSERT(strict_mode == kStrictMode || strict_mode == kNonStrictMode);
+  CONVERT_SMI_ARG_CHECKED(flags, 2);
+  bool is_eval = (flags & kDeclareGlobalsEvalFlag) != 0;

[Kevin Millikin (Chromium), 2011/09/01 07:58:24]

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.

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

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

+  StrictModeFlag strict_mode =
+      ((flags & kDeclareGlobalsStrictModeFlag) != 0) ? kStrictMode
+                                                     : kNonStrictMode;
+  bool is_native = (flags & kDeclareGlobalsNativeFlag) != 0;
 
   // Compute the property attributes. According to ECMA-262, section
   // 13, page 71, the property must be read-only and
   // non-deletable. However, neither SpiderMonkey nor KJS creates the
   // property as read-only, so we don't either.
   PropertyAttributes base = is_eval ? NONE : DONT_DELETE;
 
   // Traverse the name/value pairs and set the properties.
   int length = pairs->length();
   for (int i = 0; i < length; i += 2) {
     HandleScope scope(isolate);
     Handle<String> name(String::cast(pairs->get(i)));
     Handle<Object> value(pairs->get(i + 1), isolate);
 
     // We have to declare a global const property. To capture we only
     // assign to it when evaluating the assignment for "const x =
     // <expr>" the initial value is the hole.
     bool is_const_property = value->IsTheHole();
-
+    bool is_function_declaration = false;
     if (value->IsUndefined() || is_const_property) {
       // Lookup the property in the global object, and don't set the
       // value of the variable if the property is already there.
       LookupResult lookup;
       global->Lookup(*name, &lookup);
       if (lookup.IsProperty()) {
         // Determine if the property is local by comparing the holder
         // against the global object. The information will be used to
         // avoid throwing re-declaration errors when declaring
         // variables or constants that exist in the prototype chain.
@@ skipping 28 matching lines @@
           if (is_local && (is_read_only || is_const_property)) {
             const char* type = (is_read_only) ? "const" : "var";
             return ThrowRedeclarationError(isolate, type, name);
           }
           // The property already exists without conflicting: Go to
           // the next declaration.
           continue;
         }
       }
     } else {
+      is_function_declaration = true;
       // Copy the function and update its context. Use it as value.
       Handle<SharedFunctionInfo> shared =
           Handle<SharedFunctionInfo>::cast(value);
       Handle<JSFunction> function =
           isolate->factory()->NewFunctionFromSharedFunctionInfo(shared,
                                                                 context,
                                                                 TENURED);
       value = function;
     }
 
     LookupResult lookup;
     global->LocalLookup(*name, &lookup);
 
-    PropertyAttributes attributes = is_const_property
+    PropertyAttributes attributes =

[Kevin Millikin (Chromium), 2011/09/01 07:58:24]

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.

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

Good idea. Done!

+        (is_const_property || (is_native && is_function_declaration))
         ? static_cast<PropertyAttributes>(base | READ_ONLY)
         : base;
 
     // There's a local property that we need to overwrite because
     // we're either declaring a function or there's an interceptor
     // that claims the property is absent.
     //
     // Check for conflicting re-declarations. We cannot have
     // conflicting types in case of intercepted properties because
     // they are absent.
     if (lookup.IsProperty() &&
         (lookup.type() != INTERCEPTOR) &&
         (lookup.IsReadOnly() || is_const_property)) {
       const char* type = (lookup.IsReadOnly()) ? "const" : "var";
       return ThrowRedeclarationError(isolate, type, name);
     }
 
     // Safari does not allow the invocation of callback setters for
     // function declarations. To mimic this behavior, we do not allow
     // the invocation of setters for function values. This makes a
     // difference for global functions with the same names as event
     // handlers such as "function onload() {}". Firefox does call the
     // onload setter in those case and Safari does not. We follow
     // Safari for compatibility.
     if (value->IsJSFunction()) {
       // Do not change DONT_DELETE to false from true.
       if (lookup.IsProperty() && (lookup.type() != INTERCEPTOR)) {
         attributes = static_cast<PropertyAttributes>(

[Kevin Millikin (Chromium), 2011/09/01 07:58:24]

Here

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

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

Done.

             attributes | (lookup.GetAttributes() & DONT_DELETE));
       }
       RETURN_IF_EMPTY_HANDLE(isolate,

[Kevin Millikin (Chromium), 2011/09/01 07:58:24]

Here:

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

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

Done.

                              SetLocalPropertyIgnoreAttributes(global,
                                                               name,
                                                               value,
                                                               attributes));
     } else {
       RETURN_IF_EMPTY_HANDLE(isolate,
                              SetProperty(global,
                                          name,
                                          value,
                                          attributes,
@@ skipping 11665 matching lines @@
   } else {
     // Handle last resort GC and make sure to allow future allocations
     // to grow the heap without causing GCs (if possible).
     isolate->counters()->gc_last_resort_from_js()->Increment();
     isolate->heap()->CollectAllGarbage(false);
   }
 }
 
 
 } }  // namespace v8::internal