[modules] Fix handling of uninitialized exports in namespace objects.

For namespace objects, [[GetOwnProperty]] on an uninitialized property
throws a ReferenceError. This was not implemented everywhere. This CL
fixes all such issues I'm aware of.

Bug: v8:7470
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: I5f024450005c4f4dcb3f41c844ef055f67a9a869
Reviewed-on: https://chromium-review.googlesource.com/937341Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51638}

src/keys.cc

@@ -653,6 +653,16 @@ Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
       enum_keys = GetOwnEnumPropertyDictionaryKeys(
           isolate_, mode_, this, object, object->property_dictionary());
     }
+    if (object->IsJSModuleNamespace()) {
+      // Simulate [[GetOwnProperty]] for establishing enumerability, which
+      // throws for uninitialized exports.
+      for (int i = 0, n = enum_keys->length(); i < n; ++i) {
+        Handle<String> key(String::cast(enum_keys->get(i)), isolate_);
+        if (Handle<JSModuleNamespace>::cast(object)->GetExport(key).is_null()) {
+          return Nothing<bool>();
+        }
+      }
+    }
     AddKeys(enum_keys, DO_NOT_CONVERT);
   } else {
     if (object->HasFastProperties()) {

src/keys.h

@@ -55,8 +55,11 @@ class KeyAccumulator final BASE_EMBEDDED {
 
   // Might return directly the object's enum_cache, copy the result before using
   // as an elements backing store for a JSObject.
+  // Does not throw for uninitialized exports in module namespace objects, so
+  // this has to be checked separately.
   static Handle<FixedArray> GetOwnEnumPropertyKeys(Isolate* isolate,
                                                    Handle<JSObject> object);
+
   void AddKey(Object* key, AddKeyConversion convert = DO_NOT_CONVERT);
   void AddKey(Handle<Object> key, AddKeyConversion convert = DO_NOT_CONVERT);
   void AddKeys(Handle<FixedArray> array, AddKeyConversion convert);

src/objects.cc

@@ -6146,6 +6146,11 @@ Maybe<PropertyAttributes> JSReceiver::GetPropertyAttributes(
       case LookupIterator::INTEGER_INDEXED_EXOTIC:
         return Just(ABSENT);
       case LookupIterator::ACCESSOR:
+        if (it->GetHolder<Object>()->IsJSModuleNamespace()) {
+          return JSModuleNamespace::GetPropertyAttributes(it);
+        } else {
+          return Just(it->property_attributes());
+        }
       case LookupIterator::DATA:
         return Just(it->property_attributes());
     }
@@ -7980,8 +7985,9 @@ Maybe<bool> JSReceiver::SetIntegrityLevel(Handle<JSReceiver> receiver,
   if (receiver->IsJSObject()) {
     Handle<JSObject> object = Handle<JSObject>::cast(receiver);
 
-    if (!object->HasSloppyArgumentsElements()) {  // Fast path.
-      // prevent memory leaks by not adding unnecessary transitions
+    if (!object->HasSloppyArgumentsElements() &&
+        !object->IsJSModuleNamespace()) {  // Fast path.
+      // Prevent memory leaks by not adding unnecessary transitions.
       Maybe<bool> test = JSObject::TestIntegrityLevel(object, level);
       MAYBE_RETURN(test, Nothing<bool>());
       if (test.FromJust()) return test;
@@ -8375,8 +8381,10 @@ Maybe<bool> JSObject::PreventExtensionsWithTransition(
     Handle<JSObject> object, ShouldThrow should_throw) {
   STATIC_ASSERT(attrs == NONE || attrs == SEALED || attrs == FROZEN);
 
-  // Sealing/freezing sloppy arguments should be handled elsewhere.
+  // Sealing/freezing sloppy arguments or namespace objects should be handled
+  // elsewhere.
   DCHECK(!object->HasSloppyArgumentsElements());
+  DCHECK_IMPLIES(object->IsJSModuleNamespace(), attrs == NONE);
 
   Isolate* isolate = object->GetIsolate();
   if (object->IsAccessCheckNeeded() &&

src/objects/dictionary.h

@@ -165,6 +165,8 @@ class BaseNameDictionary : public Dictionary<Derived, Shape> {
   static Handle<FixedArray> IterationIndices(Handle<Derived> dictionary);
 
   // Copies enumerable keys to preallocated fixed array.
+  // Does not throw for uninitialized exports in module namespace objects, so
+  // this has to be checked separately.
   static void CopyEnumKeysTo(Handle<Derived> dictionary,
                              Handle<FixedArray> storage, KeyCollectionMode mode,
                              KeyAccumulator* accumulator);

src/objects/module.cc

@@ -900,5 +900,28 @@ MaybeHandle<Object> JSModuleNamespace::GetExport(Handle<String> name) {
   return value;
 }
 
+Maybe<PropertyAttributes> JSModuleNamespace::GetPropertyAttributes(
+    LookupIterator* it) {
+  Handle<JSModuleNamespace> object = it->GetHolder<JSModuleNamespace>();
+  Handle<String> name = Handle<String>::cast(it->GetName());
+  DCHECK_EQ(it->state(), LookupIterator::ACCESSOR);
+
+  Isolate* isolate = name->GetIsolate();
+
+  Handle<Object> lookup(object->module()->exports()->Lookup(name), isolate);
+  if (lookup->IsTheHole(isolate)) {
+    return Just(ABSENT);
+  }
+
+  Handle<Object> value(Handle<Cell>::cast(lookup)->value(), isolate);
+  if (value->IsTheHole(isolate)) {
+    isolate->Throw(*isolate->factory()->NewReferenceError(
+        MessageTemplate::kNotDefined, name));
+    return Nothing<PropertyAttributes>();
+  }
+
+  return Just(it->property_attributes());
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/module.h

@@ -215,6 +215,12 @@ class JSModuleNamespace : public JSObject {
   // schedule an exception and return Nothing.
   MUST_USE_RESULT MaybeHandle<Object> GetExport(Handle<String> name);
 
+  // Return the (constant) property attributes for the referenced property,
+  // which is assumed to correspond to an export. If the export is
+  // uninitialized, schedule an exception and return Nothing.
+  static MUST_USE_RESULT Maybe<PropertyAttributes> GetPropertyAttributes(
+      LookupIterator* it);
+
   // In-object fields.
   enum {
     kToStringTagFieldIndex,

src/runtime/runtime-forin.cc

@@ -36,11 +36,12 @@ MaybeHandle<HeapObject> Enumerate(Handle<JSReceiver> receiver) {
     // Test again, since cache may have been built by GetKeys() calls above.
     if (!accumulator.is_receiver_simple_enum()) return keys;
   }
+  DCHECK(!receiver->IsJSModuleNamespace());
   return handle(receiver->map(), isolate);
 }
 
 // This is a slight modifcation of JSReceiver::HasProperty, dealing with
-// the oddities of JSProxy in for-in filter.
+// the oddities of JSProxy and JSModuleNamespace in for-in filter.
 MaybeHandle<Object> HasEnumerableProperty(Isolate* isolate,
                                           Handle<JSReceiver> receiver,
                                           Handle<Object> key) {
@@ -92,7 +93,14 @@ MaybeHandle<Object> HasEnumerableProperty(Isolate* isolate,
       case LookupIterator::INTEGER_INDEXED_EXOTIC:
         // TypedArray out-of-bounds access.
         return isolate->factory()->undefined_value();
-      case LookupIterator::ACCESSOR:
+      case LookupIterator::ACCESSOR: {
+        if (it.GetHolder<Object>()->IsJSModuleNamespace()) {
+          result = JSModuleNamespace::GetPropertyAttributes(&it);
+          if (result.IsNothing()) return MaybeHandle<Object>();
+          DCHECK_EQ(0, result.FromJust() & DONT_ENUM);
+        }
+        return it.GetName();
+      }
       case LookupIterator::DATA:
         return it.GetName();
     }

test/mjsunit/modules-namespace-getownproperty1.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// MODULE
+
+import * as ns from "./modules-namespace-getownproperty1.js";
+
+
+////////////////////////////////////////////////////////////////////////////////
+// There are three exports, a and b and c (all let-declared).  Variable b is
+// declared AFTER the first set of tests ran (see below).
+export let a = 1;
+export let c = 3;
+////////////////////////////////////////////////////////////////////////////////
+
+
+// for-in
+assertThrows(() => { for (let p in ns) {} }, ReferenceError);
+
+// Object.prototype.propertyIsEnumerable
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'a'));
+assertThrows(() => Object.prototype.propertyIsEnumerable.call(ns, 'b'),
+    ReferenceError);
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'c'));
+
+// Object.prototype.hasOwnProperty
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'a'));
+assertThrows(() => Object.prototype.hasOwnProperty.call(ns, 'b'),
+    ReferenceError);
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'c'));
+
+// Object.keys
+assertThrows(() => Object.keys(ns), ReferenceError);
+
+// Object.entries
+assertThrows(() => Object.entries(ns), ReferenceError);
+
+// Object.values
+assertThrows(() => Object.values(ns), ReferenceError);
+
+// Object.getOwnPropertyNames
+assertEquals(['a', 'b', 'c'], Object.getOwnPropertyNames(ns));
+
+// Object.getOwnPropertySymbols
+assertEquals([Symbol.toStringTag], Object.getOwnPropertySymbols(ns));
+
+// Reflect.ownKeys
+assertEquals(['a', 'b', 'c', Symbol.toStringTag], Reflect.ownKeys(ns));
+
+// Object.assign
+var copy = {};
+assertThrows(() => Object.assign(copy, ns), ReferenceError);
+assertEquals({a: 1}, copy);
+
+// Object.isFrozen
+assertFalse(Object.isFrozen(ns));
+
+// Object.isSealed
+assertThrows(() => Object.isSealed(ns), ReferenceError);
+
+// Object.freeze
+assertThrows(() => Object.freeze(ns), TypeError);
+
+// Object.seal
+assertThrows(() => Object.seal(ns), ReferenceError);
+
+// JSON.stringify
+assertThrows(() => JSON.stringify(ns), ReferenceError);
+
+// PropertyDefinition
+assertThrows(() => ({...copy} = ns), ReferenceError);
+
+// delete
+assertThrows(() => delete ns.b, TypeError);
+assertFalse(Reflect.deleteProperty(ns, 'b'));
+
+
+////////////////////////////////////////////////////////////////////////////////
+// Variable b is declared here.
+export let b = 2;
+////////////////////////////////////////////////////////////////////////////////
+
+
+// for-in
+var i = 1;
+for (let p in ns) {
+  assertEquals(i, ns[p]);
+  i++
+}
+assertEquals(i, 4);
+
+// Object.prototype.propertyIsEnumerable
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'a'));
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'b'));
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'c'));
+
+// Object.prototype.hasOwnProperty
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'a'));
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'b'));
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'c'));
+
+// Object.keys
+assertEquals(['a', 'b', 'c'], Object.keys(ns));
+
+// Object.entries
+assertEquals([['a', 1], ['b', 2], ['c', 3]], Object.entries(ns));
+
+// Object.values
+assertEquals([1, 2, 3], Object.values(ns));
+
+// Object.getOwnPropertyNames
+assertEquals(['a', 'b', 'c'], Object.getOwnPropertyNames(ns));
+
+// Object.getOwnPropertySymbols
+assertEquals([Symbol.toStringTag], Object.getOwnPropertySymbols(ns));
+
+// Reflect.ownKeys
+assertEquals(['a', 'b', 'c', Symbol.toStringTag], Reflect.ownKeys(ns));
+
+// Object.assign
+copy = {};
+Object.assign(copy, ns);
+assertEquals({a: 1, b:2, c:3}, copy);
+
+// Object.isFrozen
+assertFalse(Object.isFrozen(ns));
+
+// Object.isSealed
+assertTrue(Object.isSealed(ns));
+
+// Object.freeze
+assertThrows(() => Object.freeze(ns), TypeError);
+
+// Object.seal
+assertDoesNotThrow(() => Object.seal(ns));
+
+// JSON.stringify
+assertEquals('{"a":1,"b":2,"c":3}', JSON.stringify(ns));
+
+// PropertyDefinition
+copy = {};
+({...copy} = ns);
+assertEquals({a: 1, b:2, c:3}, copy);
+
+// delete
+assertThrows(() => delete ns.b, TypeError);
+assertFalse(Reflect.deleteProperty(ns, 'b'));

test/mjsunit/modules-namespace-getownproperty2.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// MODULE
+
+import * as ns from "./modules-namespace-getownproperty2.js";
+
+// This tests the same as modules-namespace-getownproperty1.js except that here
+// variable a doesn't exist. This means that the late-declared variable b is the
+// (alphabetically) first property of the namespace object, which makes a
+// difference for some operations.
+
+
+////////////////////////////////////////////////////////////////////////////////
+// There are two exports, b and c (both let-declared).  Variable b is
+// declared AFTER the first set of tests ran (see below).
+export let c = 3;
+////////////////////////////////////////////////////////////////////////////////
+
+
+// for-in
+assertThrows(() => { for (let p in ns) {} }, ReferenceError);
+
+// Object.prototype.propertyIsEnumerable
+assertThrows(() => Object.prototype.propertyIsEnumerable.call(ns, 'b'),
+    ReferenceError);
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'c'));
+
+// Object.prototype.hasOwnProperty
+assertThrows(() => Object.prototype.hasOwnProperty.call(ns, 'b'),
+    ReferenceError);
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'c'));
+
+// Object.keys
+assertThrows(() => Object.keys(ns), ReferenceError);
+
+// Object.entries
+assertThrows(() => Object.entries(ns), ReferenceError);
+
+// Object.values
+assertThrows(() => Object.values(ns), ReferenceError);
+
+// Object.getOwnPropertyNames
+assertEquals(['b', 'c'], Object.getOwnPropertyNames(ns));
+
+// Object.getOwnPropertySymbols
+assertEquals([Symbol.toStringTag], Object.getOwnPropertySymbols(ns));
+
+// Reflect.ownKeys
+assertEquals(['b', 'c', Symbol.toStringTag], Reflect.ownKeys(ns));
+
+// Object.assign
+var copy = {};
+assertThrows(() => Object.assign(copy, ns), ReferenceError);
+assertEquals({}, copy);
+
+// Object.isFrozen
+assertThrows(() => Object.isFrozen(ns), ReferenceError);
+
+// Object.isSealed
+assertThrows(() => Object.isSealed(ns), ReferenceError);
+
+// Object.freeze
+assertThrows(() => Object.freeze(ns), ReferenceError);
+
+// Object.seal
+assertThrows(() => Object.seal(ns), ReferenceError);
+
+// JSON.stringify
+assertThrows(() => JSON.stringify(ns), ReferenceError);
+
+// PropertyDefinition
+assertThrows(() => ({...copy} = ns), ReferenceError);
+
+// delete
+assertThrows(() => delete ns.b, TypeError);
+assertFalse(Reflect.deleteProperty(ns, 'b'));
+
+
+////////////////////////////////////////////////////////////////////////////////
+// Variable b is declared here.
+export let b = 2;
+////////////////////////////////////////////////////////////////////////////////
+
+
+// for-in
+var i = 2;
+for (let p in ns) {
+  assertEquals(i, ns[p]);
+  i++
+}
+assertEquals(i, 4);
+
+// Object.prototype.propertyIsEnumerable
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'b'));
+assertTrue(Object.prototype.propertyIsEnumerable.call(ns, 'c'));
+
+// Object.prototype.hasOwnProperty
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'b'));
+assertTrue(Object.prototype.hasOwnProperty.call(ns, 'c'));
+
+// Object.keys
+assertEquals(['b', 'c'], Object.keys(ns));
+
+// Object.entries
+assertEquals([['b', 2], ['c', 3]], Object.entries(ns));
+
+// Object.values
+assertEquals([2, 3], Object.values(ns));
+
+// Object.getOwnPropertyNames
+assertEquals(['b', 'c'], Object.getOwnPropertyNames(ns));
+
+// Object.getOwnPropertySymbols
+assertEquals([Symbol.toStringTag], Object.getOwnPropertySymbols(ns));
+
+// Reflect.ownKeys
+assertEquals(['b', 'c', Symbol.toStringTag], Reflect.ownKeys(ns));
+
+// Object.assign
+copy = {};
+Object.assign(copy, ns);
+assertEquals({b:2, c:3}, copy);
+
+// Object.isFrozen
+assertFalse(Object.isFrozen(ns));
+
+// Object.isSealed
+assertTrue(Object.isSealed(ns));
+
+// Object.freeze
+assertThrows(() => Object.freeze(ns), TypeError);
+
+// Object.seal
+assertDoesNotThrow(() => Object.seal(ns));
+
+// JSON.stringify
+assertEquals('{"b":2,"c":3}', JSON.stringify(ns));
+
+// PropertyDefinition
+copy = {};
+({...copy} = ns);
+assertEquals({b:2, c:3}, copy);
+
+// delete
+assertThrows(() => delete ns.b, TypeError);
+assertFalse(Reflect.deleteProperty(ns, 'b'));

test/test262/test262.status

@@ -458,11 +458,6 @@
   'language/expressions/call/eval-spread-empty-leading': [FAIL],
   'language/expressions/call/eval-spread-empty-trailing': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=7470
-  'language/module-code/namespace/internals/enumerate-binding-uninit': [FAIL],
-  'language/module-code/namespace/internals/object-keys-binding-uninit': [FAIL],
-  'language/module-code/namespace/internals/object-propertyIsEnumerable-binding-uninit': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=7471
   'intl402/DateTimeFormat/prototype/format/time-clip-near-time-boundaries': [FAIL],
   'intl402/DateTimeFormat/prototype/format/time-clip-to-integer': [FAIL],