Add flag to make __defineGetter__ & co. behave as strict functions

When --harmony-strict-legacy-accessor-builtins is enabled, it brings V8's behavior in line with the spec and more recent versions of SpiderMonkey and JSC: - No implicit receiver coercion - Attempting to redefine a non-configurable property throws Bug: v8:5070 Change-Id: I82b927538604136c0c550e19bcc606fbfea1377e Reviewed-on: https://chromium-review.googlesource.com/478312 Reviewed-by: Daniel Ehrenberg <littledan@chromium.org> Commit-Queue: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/heads/master@{#44703}
2017-04-14 12:08:22 -07:00 · 2017-04-14 12:08:22 -07:00 · cd76322817
commit cd76322817
parent 54271c21e2
8 changed files with 84 additions and 15 deletions
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@ -3835,6 +3835,7 @@ EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_object_rest_spread)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_dynamic_import)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_template_escapes)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_restrict_constructor_return)
+EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_strict_legacy_accessor_builtins)

 void InstallPublicSymbol(Factory* factory, Handle<Context> native_context,
                         const char* name, Handle<Symbol> value) {
--- a/src/builtins/builtins-object.cc
+++ b/src/builtins/builtins-object.cc
@ -86,8 +86,11 @@ Object* ObjectDefineAccessor(Isolate* isolate, Handle<Object> object,
                             Handle<Object> name, Handle<Object> accessor) {
  // 1. Let O be ? ToObject(this value).
  Handle<JSReceiver> receiver;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, receiver,
-                                     Object::ConvertReceiver(isolate, object));
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, receiver,
+      FLAG_harmony_strict_legacy_accessor_builtins
+          ? Object::ToObject(isolate, object)
+          : Object::ConvertReceiver(isolate, object));
  // 2. If IsCallable(getter) is false, throw a TypeError exception.
  if (!accessor->IsCallable()) {
    MessageTemplate::Template message =
@ -114,7 +117,9 @@ Object* ObjectDefineAccessor(Isolate* isolate, Handle<Object> object,
  // To preserve legacy behavior, we ignore errors silently rather than
  // throwing an exception.
  Maybe<bool> success = JSReceiver::DefineOwnProperty(
-      isolate, receiver, name, &desc, Object::DONT_THROW);
+      isolate, receiver, name, &desc,
+      FLAG_harmony_strict_legacy_accessor_builtins ? Object::THROW_ON_ERROR
+                                                   : Object::DONT_THROW);
  MAYBE_RETURN(success, isolate->heap()->exception());
  if (!success.FromJust()) {
    isolate->CountUsage(v8::Isolate::kDefineGetterOrSetterWouldThrow);
@ -125,8 +130,11 @@ Object* ObjectDefineAccessor(Isolate* isolate, Handle<Object> object,

 Object* ObjectLookupAccessor(Isolate* isolate, Handle<Object> object,
                             Handle<Object> key, AccessorComponent component) {
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, object,
-                                     Object::ConvertReceiver(isolate, object));
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, object,
+      FLAG_harmony_strict_legacy_accessor_builtins
+          ? Object::ToObject(isolate, object)
+          : Object::ConvertReceiver(isolate, object));
  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, key,
                                     Object::ToPropertyKey(isolate, key));
  bool success = false;
--- a/src/flag-definitions.h
+++ b/src/flag-definitions.h
@ -203,6 +203,8 @@ DEFINE_IMPLICATION(es_staging, harmony)
  V(harmony_async_iteration, "harmony async iteration")                 \
  V(harmony_dynamic_import, "harmony dynamic import")                   \
  V(harmony_promise_finally, "harmony Promise.prototype.finally")       \
+  V(harmony_strict_legacy_accessor_builtins,                            \
+    "treat __defineGetter__ and related functions as strict")           \
  V(harmony_restrict_constructor_return,                                \
    "harmony disallow non undefined primitive return value from class " \
    "constructor")
--- a/test/mjsunit/global-accessors.js
+++ b/test/mjsunit/global-accessors.js
@ -26,6 +26,8 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

 // Test accessors on the global object.
+//
+// Flags: --no-harmony-strict-legacy-accessor-builtins

 var x_ = 0;

--- a/test/mjsunit/harmony/global-accessors-strict.js
+++ b/test/mjsunit/harmony/global-accessors-strict.js
@ -0,0 +1,54 @@
+// Copyright 2010 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
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Test accessors on the global object.
+//
+// Flags: --harmony-strict-legacy-accessor-builtins
+
+var x_ = 0;
+
+this.__defineSetter__('x', function(x) { x_ = x; });
+this.__defineGetter__('x', function() { return x_; });
+
+this.__defineSetter__('y', function(x) { });
+this.__defineGetter__('y', function() { return 7; });
+
+function f(a) {
+  x = x + a;
+  return x;
+}
+
+function g(a) {
+  y = y + a;
+  return y;
+}
+
+assertEquals(1, f(1));
+assertEquals(3, f(2));
+
+assertEquals(7, g(1));
+assertEquals(7, g(2));
--- a/test/mjsunit/indexed-accessors.js
+++ b/test/mjsunit/indexed-accessors.js
@ -101,7 +101,7 @@ assertEquals(q1.b, 17);
 // Reported by nth10sd.

 a = function() {};
-__defineSetter__("0", function() {});
+this.__defineSetter__("0", function() {});
 if (a |= '') {};
 assertThrows('this[a].__parent__');
 assertEquals(a, 0);
--- a/test/mjsunit/regress/regress-1240.js
+++ b/test/mjsunit/regress/regress-1240.js
@ -33,7 +33,9 @@ var a = {};
 Object.defineProperty(a, 'b',
                      { get: function () { return 42; }, configurable: false });
 // Do not allow us to redefine b on a.
-a.__defineGetter__('b', function _b(){ return 'foo'; });
+try {
+  a.__defineGetter__('b', function _b(){ return 'foo'; });
+} catch (e) {}
 assertEquals(42, a.b);
 var desc = Object.getOwnPropertyDescriptor(a, 'b');
 assertFalse(desc.configurable);
--- a/test/test262/test262.status
+++ b/test/test262/test262.status
@ -278,14 +278,14 @@
  'built-ins/TypedArrays/internals/Set/conversion-operation-consistent-nan': [PASS, FAIL],

  # https://bugs.chromium.org/p/v8/issues/detail?id=5070
-  'annexB/built-ins/Object/prototype/__defineGetter__/define-non-configurable': [FAIL],
-  'annexB/built-ins/Object/prototype/__defineGetter__/define-non-extensible': [FAIL],
-  'annexB/built-ins/Object/prototype/__defineGetter__/this-non-obj': [FAIL],
-  'annexB/built-ins/Object/prototype/__defineSetter__/define-non-configurable': [FAIL],
-  'annexB/built-ins/Object/prototype/__defineSetter__/define-non-extensible': [FAIL],
-  'annexB/built-ins/Object/prototype/__defineSetter__/this-non-obj': [FAIL],
-  'annexB/built-ins/Object/prototype/__lookupGetter__/this-non-obj': [FAIL],
-  'annexB/built-ins/Object/prototype/__lookupSetter__/this-non-obj': [FAIL],
+  'annexB/built-ins/Object/prototype/__defineGetter__/define-non-configurable': ['--harmony-strict-legacy-accessor-builtins'],
+  'annexB/built-ins/Object/prototype/__defineGetter__/define-non-extensible': ['--harmony-strict-legacy-accessor-builtins'],
+  'annexB/built-ins/Object/prototype/__defineGetter__/this-non-obj': ['--harmony-strict-legacy-accessor-builtins'],
+  'annexB/built-ins/Object/prototype/__defineSetter__/define-non-configurable': ['--harmony-strict-legacy-accessor-builtins'],
+  'annexB/built-ins/Object/prototype/__defineSetter__/define-non-extensible': ['--harmony-strict-legacy-accessor-builtins'],
+  'annexB/built-ins/Object/prototype/__defineSetter__/this-non-obj': ['--harmony-strict-legacy-accessor-builtins'],
+  'annexB/built-ins/Object/prototype/__lookupGetter__/this-non-obj': ['--harmony-strict-legacy-accessor-builtins'],
+  'annexB/built-ins/Object/prototype/__lookupSetter__/this-non-obj': ['--harmony-strict-legacy-accessor-builtins'],

  # https://bugs.chromium.org/p/v8/issues/detail?id=4451
  # https://github.com/tc39/ecma262/issues/753