[asm.js] Ensure lookups of imports are non-observable.

This makes sure that property lookups on the provided imports object are
non-observable to JavaScript. It allows instantiation failures to fall
back to JavaScript proper without accidentally calling accessors twice.
Also accessors might invalidate previous checks done during linking or
throw exceptions.

R=clemensh@chromium.org
TEST=mjsunit/regress/regress-crbug-719384
BUG=chromium:719384

Change-Id: I3db2672d2a496110f705d02b82878e70cd5d701f
Reviewed-on: https://chromium-review.googlesource.com/509552Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45481}

src/asmjs/asm-js.cc

@@ -286,16 +286,6 @@ MaybeHandle<Object> AsmJs::InstantiateAsmWasm(Isolate* isolate,
     }
   }
 
-  // Create the ffi object for foreign functions {"": foreign}.
-  Handle<JSObject> ffi_object;
-  if (!foreign.is_null()) {
-    Handle<JSFunction> object_function = Handle<JSFunction>(
-        isolate->native_context()->object_function(), isolate);
-    ffi_object = isolate->factory()->NewJSObject(object_function);
-    JSObject::AddProperty(ffi_object, isolate->factory()->empty_string(),
-                          foreign, NONE);
-  }
-
   // Check that a valid heap buffer is provided if required.
   if (stdlib_use_of_typed_array_present) {
     if (memory.is_null()) {
@@ -313,8 +303,9 @@ MaybeHandle<Object> AsmJs::InstantiateAsmWasm(Isolate* isolate,
 
   wasm::ErrorThrower thrower(isolate, "AsmJs::Instantiate");
   MaybeHandle<Object> maybe_module_object =
-      wasm::SyncInstantiate(isolate, &thrower, module, ffi_object, memory);
+      wasm::SyncInstantiate(isolate, &thrower, module, foreign, memory);
   if (maybe_module_object.is_null()) {
+    DCHECK(!isolate->has_pending_exception());
     thrower.Reset();  // Ensure exceptions do not propagate.
     ReportInstantiationFailure(script, position, "Internal wasm failure");
     return MaybeHandle<Object>();

src/runtime/runtime-compiler.cc

@@ -88,9 +88,9 @@ RUNTIME_FUNCTION(Runtime_InstantiateAsmJs) {
   if (args[1]->IsJSReceiver()) {
     stdlib = args.at<JSReceiver>(1);
   }
-  Handle<JSObject> foreign;
-  if (args[2]->IsJSObject()) {
-    foreign = args.at<JSObject>(2);
+  Handle<JSReceiver> foreign;
+  if (args[2]->IsJSReceiver()) {
+    foreign = args.at<JSReceiver>(2);
   }
   Handle<JSArrayBuffer> memory;
   if (args[3]->IsJSArrayBuffer()) {

src/wasm/wasm-module.cc

@@ -1620,6 +1620,44 @@ class InstantiationHelper {
     return result;
   }
 
+  // Look up an import value in the {ffi_} object specifically for linking an
+  // asm.js module. This only performs non-observable lookups, which allows
+  // falling back to JavaScript proper (and hence re-executing all lookups) if
+  // module instantiation fails.
+  MaybeHandle<Object> LookupImportAsm(uint32_t index,
+                                      Handle<String> import_name) {
+    // Check that a foreign function interface object was provided.
+    if (ffi_.is_null()) {
+      return ReportLinkError("missing imports object", index, import_name);
+    }
+
+    // Perform lookup of the given {import_name} without causing any observable
+    // side-effect. We only accept accesses that resolve to data properties,
+    // which is indicated by the asm.js spec in section 7 ("Linking") as well.
+    Handle<Object> result;
+    LookupIterator it =
+        LookupIterator::PropertyOrElement(isolate_, ffi_, import_name);
+    switch (it.state()) {
+      case LookupIterator::ACCESS_CHECK:
+      case LookupIterator::INTEGER_INDEXED_EXOTIC:
+      case LookupIterator::INTERCEPTOR:
+      case LookupIterator::JSPROXY:
+      case LookupIterator::ACCESSOR:
+      case LookupIterator::TRANSITION:
+        return ReportLinkError("not a data property", index, import_name);
+      case LookupIterator::NOT_FOUND:
+        // Accepting missing properties as undefined does not cause any
+        // observable difference from JavaScript semantics, we are lenient.
+        result = isolate_->factory()->undefined_value();
+        break;
+      case LookupIterator::DATA:
+        result = it.GetDataValue();
+        break;
+    }
+
+    return result;
+  }
+
   uint32_t EvalUint32InitExpr(const WasmInitExpr& expr) {
     switch (expr.kind) {
       case WasmInitExpr::kI32Const:
@@ -1704,7 +1742,8 @@ class InstantiationHelper {
       if (!maybe_import_name.ToHandle(&import_name)) return -1;
 
       MaybeHandle<Object> result =
-          LookupImport(index, module_name, import_name);
+          module_->is_asm_js() ? LookupImportAsm(index, import_name)
+                               : LookupImport(index, module_name, import_name);
       if (thrower_->error()) return -1;
       Handle<Object> value = result.ToHandleChecked();
 

test/mjsunit/asm/global-imports.js

@@ -72,3 +72,8 @@ Run("var x = foreign.x",        "x(dbl) | 0",               { x:p }, 17, true);
 Run("var x = foreign.x",        "(x = fround, x(dbl)) | 0", { x:p }, 4,  false);
 Run("var x = stdlib.Math.E",    "(x = 3.1415, 1) | 0",      {},      1,  false);
 Run("var x = stdlib.Math.imul", "(x = fround, 1) | 0",      {},      1,  false);
+
+// Imports missing or causing side-effects during lookup.
+Run("var x = +foreign.x", "+x", { no_x_present:0 },        NaN, true);
+Run("var x = +foreign.x", "+x", { get x() { return 23 } }, 23,  false);
+Run("var x = +foreign.x", "+x", new Proxy({ x:42 }, {}),   42,  false);

test/mjsunit/regress/regress-crbug-719384.js

+// Copyright 2017 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.
+
+// Flags: --allow-natives-syntax
+
+(function TestThrowingObserver() {
+  function Module(stdlib, foreign) {
+    "use asm";
+    var x = foreign.x | 0;
+    function f() {}
+    return { f:f };
+  }
+  var observer = { get x() { throw new Error() } };
+  assertThrows(() => Module(this, observer));
+  assertFalse(%IsAsmWasmCode(Module));
+})();
+
+(function TestMutatingObserver() {
+  function Module(stdlib, foreign) {
+    "use asm";
+    var x = +foreign.x;
+    var PI = stdlib.Math.PI;
+    function f() {
+      return +(PI + x);
+    }
+    return { f:f };
+  }
+  var stdlib = { Math : { PI : Math.PI } };
+  var observer = { get x() { stdlib.Math.PI = 23; return 42; } };
+  var m = Module(stdlib, observer);
+  assertFalse(%IsAsmWasmCode(Module));
+  assertEquals(65, m.f());
+})();

test/mjsunit/wasm/asm-wasm.js

@@ -1145,9 +1145,6 @@ function TestForeignVariables() {
   TestCase({baz: 345.7}, 0, NaN, 345, 345.7);
   // Check that undefined values are converted to proper defaults.
   TestCase({qux: 999}, 0, NaN, 0, NaN);
-  // Check that an undefined ffi is ok.
-  // TODO(v8:6127): Fix handling of this case and re-enable.
-//  TestCase(undefined, 0, NaN, 0, NaN);
   // Check that true values are converted properly.
   TestCase({foo: true, bar: true, baz: true}, 1, 1.0, 1, 1.0);
   // Check that false values are converted properly.
@@ -1164,20 +1161,6 @@ function TestForeignVariables() {
   TestCase({foo: [], bar: [], baz: []}, 0, 0, 0, 0);
   // Check that object values are converted properly.
   TestCase({foo: {}, bar: {}, baz: {}}, 0, NaN, 0, NaN);
-  // Check that getter object values are converted properly.
-  var o = {
-    get foo() {
-      return 123.4;
-    }
-  };
-  TestCase({foo: o.foo, bar: o.foo, baz: o.foo}, 123, 123.4, 123, 123.4);
-  // Check that getter object values are converted properly.
-  var o = {
-    get baz() {
-      return 123.4;
-    }
-  };
-  TestCase(o, 0, NaN, 123, 123.4);
   // Check that objects with valueOf are converted properly.
   var o = {
     valueOf: function() { return 99; }
@@ -1185,9 +1168,6 @@ function TestForeignVariables() {
   TestCase({foo: o, bar: o, baz: o}, 99, 99, 99, 99);
   // Check that function values are converted properly.
   TestCase({foo: TestCase, bar: TestCase, qux: TestCase}, 0, NaN, 0, NaN);
-  // Check that a missing ffi object is safe.
-  // TODO(v8:6127): Fix handling of this case and re-enable.
-//  TestCase(undefined, 0, NaN, 0, NaN);
 }
 
 print("TestForeignVariables...");