Move securityCheck out of V8WrapperInstantiationScope

third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h

Index: third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h b/third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h
index bc0ca27546ad2755bf250fb64e15a136b3ddf7e9..f833d717de3c966d57e73bb75be8a3a417f3814a 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h
+++ b/third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.h
@@ -31,10 +31,10 @@
 #ifndef V8DOMWrapper_h
 #define V8DOMWrapper_h
 
-#include "bindings/core/v8/BindingSecurity.h"
 #include "bindings/core/v8/DOMDataStore.h"
 #include "bindings/core/v8/ScriptWrappable.h"
 #include "bindings/core/v8/V8Binding.h"
+#include "bindings/core/v8/WrapperCreationSecurityCheck.h"
 #include "core/CoreExport.h"
 #include "v8/include/v8.h"
 #include "wtf/Compiler.h"
@@ -125,11 +125,11 @@ class V8WrapperInstantiationScope {
  public:
   V8WrapperInstantiationScope(v8::Local<v8::Object> creationContext,
                               v8::Isolate* isolate,
-                              bool withSecurityCheck)
+                              const WrapperTypeInfo* type)
       : m_didEnterContext(false),
         m_context(isolate->GetCurrentContext()),
         m_tryCatch(isolate),
-        m_convertExceptions(false) {
+        m_type(type) {
     // creationContext should not be empty. Because if we have an
     // empty creationContext, we will end up creating
     // a new object in the context currently entered. This is wrong.
@@ -141,11 +141,7 @@ class V8WrapperInstantiationScope {
     // context is different from the context that we are about to enter.
     if (contextForWrapper == m_context)
       return;
-    if (withSecurityCheck) {
-      securityCheck(isolate, contextForWrapper);
-    } else {
-      m_convertExceptions = true;
-    }
+
     m_context = v8::Local<v8::Context>::New(isolate, contextForWrapper);
     m_didEnterContext = true;
     m_context->Enter();
@@ -157,26 +153,26 @@ class V8WrapperInstantiationScope {
       return;
     }
     m_context->Exit();
-    // Rethrow any cross-context exceptions as security error.
-    if (m_tryCatch.HasCaught()) {
-      if (m_convertExceptions) {
-        m_tryCatch.Reset();
-        convertException();
-      }
+
+    v8::Isolate* isolate = m_context->GetIsolate();
+    v8::Local<v8::Value> caughtException = m_tryCatch.Exception();
+
+    m_tryCatch.Reset();
+    WrapperCreationSecurityCheck::securityCheck(

[Yuki, 2017/03/31 09:49:37]

This code seems expected to (re)throw an exception in a secure manner, rather
than just performing a security check.  Maybe better to change the name?

[adithyas, 2017/03/31 17:49:28]

OK, changed to a more descriptive name.

+        isolate, isolate->GetCurrentContext(), m_context, m_type,
+        caughtException);
+
+    if (m_tryCatch.HasCaught())

[Yuki, 2017/03/31 09:49:37]

You've reset m_tryCatch on line 160.  This is meaningless, isn't it?

[adithyas, 2017/03/31 17:49:28]

Hmm, does Reset() completely disable the TryCatch? securityCheck can throw an
exception, and if it does throw an exception, the tryCatch would catch it and
HasCaught() would return true right?

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

Ah, now I see the point.  Then, I'd prefer an early-exit here, too.

  if (!m_didEnterContext) {
    m_tryCatch.ReThrow();
    return;
  }
  m_context->Exit();

  if (!m_tryCatch.HasCaught())
    return;

  // A cross-context exception exists, so rethrow it.
  verifyContextAccessAndHandleCrossContextException();
  m_tryCatch.ReThrow();

Given the above logic, I'd feel that
  rethrowCrossContextException();
  m_tryCatch.ReThrow();
would be more straightforward because "(re)throw an exception" is the expected
behavior.  The following m_tryCatch.ReThrow() makes sense at a glance.

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

I think verifyContextAccessAndHandleCrossContextException needs to be called
regardless of whether a cross context exception is thrown or not. The security
checks (shouldAllowAccessToFrame/DetachedWindow) inside the method needs to be
run regardless of whether a cross context exception was thrown.

I could maybe split it up into two methods: verifyContextAccess and
rethrowCrossContextException

if (!m_didEnterContext) {
  m_tryCatch.ReThrow();
  return;
}
m_context->Exit();

crossContextException = m_tryCatch.Exception();
m_tryCatch.Reset();

if (!verifyContextAccess()) { // verifyContextAccess throws a security error
when it returns false
  m_tryCatch.ReThrow();
  return;
}

if (crossContextException.Empty())
  return;

rethrowCrossContextException(crossContextException);
m_tryCatch.ReThrow();

WDYT?

[Yuki, 2017/04/05 07:59:19]

I'm getting better understanding.  The original implementation was correct about
when to perform the security check.

It's too late to perform the security check in the destructor.  We need to
perform the security check *BEFORE* we're going to create a new wrapper object. 
Otherwise, there may be a security leak.  On this point, the original
implementation was correct.  However, when the security check failed, we
shouldn't even try to create a new wrapper object (the original implementation
ignores the failure of the security check, I don't like it.  We should abort the
wrapper creation in that case).

       m_tryCatch.ReThrow();
-    }
   }
 
   v8::Local<v8::Context> context() const { return m_context; }
 
  private:
-  void securityCheck(v8::Isolate*, v8::Local<v8::Context> contextForWrapper);
-  void convertException();
-
   bool m_didEnterContext;
   v8::Local<v8::Context> m_context;
   v8::TryCatch m_tryCatch;
-  bool m_convertExceptions;
+  const WrapperTypeInfo* m_type;
 };
 
 }  // namespace blink