Propagate exceptions thrown by access check interceptors.

When v8 fails an access check, it invokes a helper to try to see if it
can service the request via an access check interceptor. Invoking the
access check interceptor can throw an exception (e.g. a SecurityError).

Unfortunately, the failed access check property helpers and the
interceptor helpers don't agree on how to propagate the exception: if
the interceptor helper detects a scheduled exception, it promotes the
exception to a pending exception and returns to the failed access check
property helper.

The failed access check property helper also has an early return in
case of a scheduled exception. However, this doesn't work, as the
previously thrown exception is no longer scheduled, as it's been
promoted to a pending exception. Thus, the failed access check property
helper always end up calling the failed access check callback as well.
Since Blink's implementation of the failed access check callback also
throws an exception, this conflicts with the previously-thrown,
already-pending exception.

With this patch, the failed access check property helpers check for a
pending exception rather than a scheduled exception after invoking the
interceptor, so the exception can be propagated correctly.

BUG=v8:5715
R=yangguo@chromium.org,jochen@chromium.org

Review-Url: https://codereview.chromium.org/2550423002
Cr-Commit-Position: refs/heads/master@{#41556}

src/objects.cc

@@ -1811,11 +1811,13 @@ MaybeHandle<Object> JSObject::GetPropertyWithFailedAccessCheck(
                                  GetPropertyWithInterceptor(it, &done), Object);
       if (done) return result;
     }
+
   } else {
-    MaybeHandle<Object> result;
+    Handle<Object> result;
     bool done;
-    result = GetPropertyWithInterceptorInternal(it, interceptor, &done);
-    RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object);
+    ASSIGN_RETURN_ON_EXCEPTION(
+        isolate, result,
+        GetPropertyWithInterceptorInternal(it, interceptor, &done), Object);
     if (done) return result;
   }
 
@@ -1851,7 +1853,7 @@ Maybe<PropertyAttributes> JSObject::GetPropertyAttributesWithFailedAccessCheck(
   } else {
     Maybe<PropertyAttributes> result =
         GetPropertyAttributesWithInterceptorInternal(it, interceptor);
-    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<PropertyAttributes>());
+    if (isolate->has_pending_exception()) return Nothing<PropertyAttributes>();
     if (result.FromMaybe(ABSENT) != ABSENT) return result;
   }
   isolate->ReportFailedAccessCheck(checked);
@@ -1887,10 +1889,9 @@ Maybe<bool> JSObject::SetPropertyWithFailedAccessCheck(
   } else {
     Maybe<bool> result = SetPropertyWithInterceptorInternal(
         it, interceptor, should_throw, value);
-    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
+    if (isolate->has_pending_exception()) return Nothing<bool>();
     if (result.IsJust()) return result;
   }
-
   isolate->ReportFailedAccessCheck(checked);
   RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
   return Just(true);

test/cctest/test-access-checks.cc

@@ -102,6 +102,29 @@ void IndexedEnumerator(const v8::PropertyCallbackInfo<v8::Array>& info) {
   info.GetReturnValue().Set(names);
 }
 
+void NamedGetterThrowsException(
+    v8::Local<v8::Name> property,
+    const v8::PropertyCallbackInfo<v8::Value>& info) {
+  info.GetIsolate()->ThrowException(v8_str("exception"));
+}
+
+void NamedSetterThrowsException(
+    v8::Local<v8::Name> property, v8::Local<v8::Value> value,
+    const v8::PropertyCallbackInfo<v8::Value>& info) {
+  info.GetIsolate()->ThrowException(v8_str("exception"));
+}
+
+void IndexedGetterThrowsException(
+    uint32_t index, const v8::PropertyCallbackInfo<v8::Value>& info) {
+  info.GetIsolate()->ThrowException(v8_str("exception"));
+}
+
+void IndexedSetterThrowsException(
+    uint32_t index, v8::Local<v8::Value> value,
+    const v8::PropertyCallbackInfo<v8::Value>& info) {
+  info.GetIsolate()->ThrowException(v8_str("exception"));
+}
+
 bool AccessCheck(v8::Local<v8::Context> accessing_context,
                  v8::Local<v8::Object> accessed_object,
                  v8::Local<v8::Value> data) {
@@ -181,6 +204,56 @@ void CheckCrossContextAccess(v8::Isolate* isolate,
                "[\"7\",\"cross_context_int\"]");
 }
 
+void CheckCrossContextAccessWithException(
+    v8::Isolate* isolate, v8::Local<v8::Context> accessing_context,
+    v8::Local<v8::Object> accessed_object) {
+  v8::HandleScope handle_scope(isolate);
+  accessing_context->Global()
+      ->Set(accessing_context, v8_str("other"), accessed_object)
+      .FromJust();
+  v8::Context::Scope context_scope(accessing_context);
+
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("this.other.should_throw");
+    CHECK(try_catch.HasCaught());
+    CHECK(try_catch.Exception()->IsString());
+    CHECK(v8_str("exception")
+              ->Equals(accessing_context, try_catch.Exception())
+              .FromJust());
+  }
+
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("this.other.should_throw = 8");
+    CHECK(try_catch.HasCaught());
+    CHECK(try_catch.Exception()->IsString());
+    CHECK(v8_str("exception")
+              ->Equals(accessing_context, try_catch.Exception())
+              .FromJust());
+  }
+
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("this.other[42]");
+    CHECK(try_catch.HasCaught());
+    CHECK(try_catch.Exception()->IsString());
+    CHECK(v8_str("exception")
+              ->Equals(accessing_context, try_catch.Exception())
+              .FromJust());
+  }
+
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("this.other[42] = 8");
+    CHECK(try_catch.HasCaught());
+    CHECK(try_catch.Exception()->IsString());
+    CHECK(v8_str("exception")
+              ->Equals(accessing_context, try_catch.Exception())
+              .FromJust());
+  }
+}
+
 void Ctor(const v8::FunctionCallbackInfo<v8::Value>& info) {
   CHECK(info.IsConstructCall());
 }
@@ -215,6 +288,32 @@ TEST(AccessCheckWithInterceptor) {
   CheckCrossContextAccess(isolate, context1, context0->Global());
 }
 
+TEST(AccessCheckWithExceptionThrowingInterceptor) {
+  v8::Isolate* isolate = CcTest::isolate();
+  isolate->SetFailedAccessCheckCallbackFunction([](v8::Local<v8::Object> target,
+                                                   v8::AccessType type,
+                                                   v8::Local<v8::Value> data) {
+    CHECK(false);  // This should never be called.
+  });
+
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::ObjectTemplate> global_template =
+      v8::ObjectTemplate::New(isolate);
+  global_template->SetAccessCheckCallbackAndHandler(
+      AccessCheck, v8::NamedPropertyHandlerConfiguration(
+                       NamedGetterThrowsException, NamedSetterThrowsException),
+      v8::IndexedPropertyHandlerConfiguration(IndexedGetterThrowsException,
+                                              IndexedSetterThrowsException));
+
+  // Create two contexts.
+  v8::Local<v8::Context> context0 =
+      v8::Context::New(isolate, nullptr, global_template);
+  v8::Local<v8::Context> context1 =
+      v8::Context::New(isolate, nullptr, global_template);
+
+  CheckCrossContextAccessWithException(isolate, context1, context0->Global());
+}
+
 TEST(NewRemoteContext) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::HandleScope scope(isolate);