Move securityCheck out of V8WrapperInstantiationScope

third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp

Index: third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
index 516ef4dff5498d95b703de67ff7f23e7350fcaca..fdb3aad09cc2a1a04267eff1d61e66f2095df5a0 100644
--- a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
@@ -32,6 +32,8 @@
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/V8Binding.h"
+#include "bindings/core/v8/V8Location.h"
+#include "bindings/core/v8/WrapperCreationSecurityCheck.h"
 #include "core/dom/Document.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/LocalFrame.h"
@@ -252,6 +254,56 @@ bool BindingSecurity::shouldAllowNamedAccessTo(const DOMWindow* accessingWindow,
   return true;
 }
 
+void BindingSecurity::wrapperCreationSecurityCheck(
+    v8::Isolate* isolate,
+    v8::Local<v8::Context> creationContext,
+    const WrapperTypeInfo* type,
+    v8::Local<v8::Value> crossContextException) {
+  ExceptionState exceptionState(isolate, ExceptionState::ConstructionContext,
+                                type->interfaceName);
+
+  // According to
+  // https://html.spec.whatwg.org/multipage/browsers.html#security-location,
+  // cross-origin script access to a few properties of Location is allowed.
+  // Location already implements the necessary security checks.
+  if (type->equals(&V8Location::wrapperTypeInfo)) {
+    if (crossContextException.IsEmpty())

[Yuki, 2017/04/03 08:29:26]

I meant that we can do an early-exit at the beginning.

void BindingSecurity::wrapperCreationSecurityCheck(...) {
  if (crossContextException.IsEmpty())
    return;

  ExceptionState exceptionState(...);
  ...
}

However, now I think that it's better to do this early-exit on the call site. 
See my reply at:
https://codereview.chromium.org/2745313003/diff/120001/third_party/WebKit/Sou...

Given that we do the early-exit on the call site, we can write:

void BindingSecurity::wrapperCreationSecurityCheck(...) {
  DCHECK(!crossContextException.IsEmpty());

  ...
}

And then, I think that the expected behavior of this function will be "rethrow
an exception", and I'd prefer "rethrowCrossContextException" or something.  It's
a bit strange that fooCheck() returns void.

[adithyas, 2017/04/03 15:20:54]

Based on my reply on your earlier comment, I was thinking this could be 2
functions instead:

bool BindingSecurity::checkCreationContextAccess(creationContext, type) {
  if (type == V8Location)
    return true;
  frame = toLocalFrameIfNotDetached(creationContext)
  if (!frame) {
    return shouldAllowAccessToDetachedWindow(...); // could throw security error
  } else {
    return shouldAllowAccessToFrame(...); // could throw security error
  }
}

void BindingSecurity::rethrowCrossContextException(crossContextException, type,
creationContext) {
  if (type == V8Location) {
    // convert to security error and throw
  } else {
    // rethrow crossContextException
}

+      return;
+    // Convert cross-context exception to security error
+    LocalDOMWindow* callingWindow = currentDOMWindow(isolate);
+    LocalDOMWindow* targetWindow = toLocalDOMWindow(creationContext);
+    exceptionState.throwSecurityError(
+        targetWindow->sanitizedCrossDomainAccessErrorMessage(callingWindow),
+        targetWindow->crossDomainAccessErrorMessage(callingWindow));
+    return;
+  }
+
+  bool hasAccess = false;
+  LocalFrame* frame = toLocalFrameIfNotDetached(creationContext);
+
+  if (!frame) {
+    // Sandbox detached frames - they can't create cross origin objects.
+    LocalDOMWindow* callingWindow = currentDOMWindow(isolate);
+    LocalDOMWindow* targetWindow = toLocalDOMWindow(creationContext);
+
+    hasAccess = shouldAllowAccessToDetachedWindow(callingWindow, targetWindow,
+                                                  exceptionState);
+  } else {
+    const DOMWrapperWorld& currentWorld =
+        DOMWrapperWorld::world(isolate->GetCurrentContext());
+    CHECK_EQ(currentWorld.worldId(),
+             DOMWrapperWorld::world(creationContext).worldId());
+
+    hasAccess = !currentWorld.isMainWorld() ||
+                shouldAllowAccessToFrame(currentDOMWindow(isolate), frame,
+                                         exceptionState);
+  }
+
+  if (hasAccess && !crossContextException.IsEmpty()) {
+    exceptionState.rethrowV8Exception(crossContextException);
+  }

[Yuki, 2017/04/03 08:29:26]

You may want:
  DCHECK(exceptionState.hadException());

+}
+
 void BindingSecurity::failedAccessCheckFor(v8::Isolate* isolate,
                                            const Frame* target) {
   // TODO(dcheng): See if this null check can be removed or hoisted to a