Fix handling of foreign callbacks in DefineOwnProperty.

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 4308 matching lines @@
       } else {
         js_object->set_elements(*extended_dictionary);
       }
     }
     return *obj_value;
   }
 
   LookupResult result(isolate);
   js_object->LocalLookupRealNamedProperty(*name, &result);
 
-  // To be compatible with safari we do not change the value on API objects
-  // in defineProperty. Firefox disagrees here, and actually changes the value.
-  if (result.IsProperty() &&
-      (result.type() == CALLBACKS) &&
-      result.GetCallbackObject()->IsAccessorInfo()) {
-    return isolate->heap()->undefined_value();
+  // Special case for callback properties.
+  if (result.IsProperty() && result.type() == CALLBACKS) {
+    Object* callback = result.GetCallbackObject();
+    // To be compatible with safari we do not change the value on API objects
+    // in defineProperty. Firefox disagrees here, and actually changes the value.

[rossberg, 2011/12/19 16:47:44]

80 characters

[Michael Starzinger, 2011/12/20 08:50:16]

Done.

+    if (callback->IsAccessorInfo()) {
+      return isolate->heap()->undefined_value();
+    }
+    // Avoid redefining foreign callback as data property, just use the stored
+    // setter to update the value instead.
+    // TODO(mstarzinger): So far this only works if property attributes don't
+    // change, this should be fixed once we cleanup the underlying code.
+    if (callback->IsForeign() && result.GetAttributes() == attr) {
+      return js_object->SetPropertyWithCallback(callback,
+                                                *name,
+                                                *obj_value,
+                                                result.holder(),
+                                                kNonStrictMode);

[rossberg, 2011/12/19 16:47:44]

This should be KStrictMode, I think.

[Michael Starzinger, 2011/12/20 08:50:16]

Done. On this code path it doesn't actually matter. The mode is only considered
for JavaScript accessors, but not for API nor foreign callbacks.

+    }
   }
 
   // Take special care when attributes are different and there is already
   // a property. For simplicity we normalize the property which enables us
   // to not worry about changing the instance_descriptor and creating a new
   // map. The current version of SetObjectProperty does not handle attributes
   // correctly in the case where a property is a field and is reset with
   // new attributes.
   if (result.IsProperty() &&
       (attr != result.GetAttributes() || result.type() == CALLBACKS)) {
@@ skipping 9226 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(Heap::kNoGCFlags);
   }
 }
 
 
 } }  // namespace v8::internal