Create DOM exceptions in the correct context.

Create DOM exceptions in the correct context.

If access to a cross-origin property or method is allowed, but it ends
up throwing a DOM exception, arrange for the exception to be created
in the context of the target property/method, not that of the
caller/accessing context.

Addressed by having ExceptionState keep a creation context, which is
then passed along when creating the wrapper for the exception object.

R=haraken@chromium.org
BUG=235223
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162010

[sof, 2013-11-11 10:21:51]

Please have a look (if there people even better suited to review this
context-related change, then please let me know.)

[haraken, 2013-11-11 10:29:01]

LGTM.

We're using v8::Handle<v8::Object>() for a creation context in a lot of places
in V8 bindings. We should kill all of them :)

https://codereview.chromium.org/68563003/diff/1/Source/bindings/v8/V8ThrowExc...
File Source/bindings/v8/V8ThrowException.h (right):

https://codereview.chromium.org/68563003/diff/1/Source/bindings/v8/V8ThrowExc...
Source/bindings/v8/V8ThrowException.h:46: return createDOMException(ec,
String(), v8::Handle<v8::Object>(), isolate);

This method will end up creating a DOM exception in an unexpected context. We
might want to kill this method in a follow-up.

[sof, 2013-11-11 10:56:44]

https://codereview.chromium.org/68563003/diff/1/Source/bindings/v8/V8ThrowExc...
File Source/bindings/v8/V8ThrowException.h (right):

https://codereview.chromium.org/68563003/diff/1/Source/bindings/v8/V8ThrowExc...
Source/bindings/v8/V8ThrowException.h:46: return createDOMException(ec,
String(), v8::Handle<v8::Object>(), isolate);
On 2013/11/11 10:29:01, haraken wrote:
> 
> This method will end up creating a DOM exception in an unexpected context. We
> might want to kill this method in a follow-up.

Yes, let's do that if no one will miss having it around. It is currently unused.

[commit-bot: I haz the power, 2013-11-11 10:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/68563003/1

[commit-bot: I haz the power, 2013-11-13 15:15:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/68563003/1

[commit-bot: I haz the power, 2013-11-13 15:15:53]

Failed to apply patch for Source/bindings/tests/results/V8TestObject.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/tests/results/V8TestObject.cpp
  Hunk #11 succeeded at 3001 with fuzz 2.
  Hunk #14 succeeded at 3128 (offset -2 lines).
  Hunk #15 succeeded at 3202 (offset -4 lines).
  Hunk #16 succeeded at 3228 (offset -4 lines).
  Hunk #17 succeeded at 3293 (offset -4 lines).
  Hunk #18 succeeded at 4141 (offset -57 lines).
  Hunk #19 FAILED at 4220.
  Hunk #20 succeeded at 4181 (offset -57 lines).
  Hunk #21 FAILED at 4349.
  Hunk #22 succeeded at 4336 (offset -57 lines).
  Hunk #23 succeeded at 4870 (offset -57 lines).
  Hunk #24 succeeded at 4886 (offset -57 lines).
  2 out of 24 hunks FAILED -- saving rejects to file
Source/bindings/tests/results/V8TestObject.cpp.rej

Patch:       Source/bindings/tests/results/V8TestObject.cpp
Index: Source/bindings/tests/results/V8TestObject.cpp
diff --git a/Source/bindings/tests/results/V8TestObject.cpp
b/Source/bindings/tests/results/V8TestObject.cpp
index
b882b1c4776d7fe4801a51ba604e0860ab87f456..9cf74c55bb81bedcf09c81ec206c4ff20d3d4cf5
100644
--- a/Source/bindings/tests/results/V8TestObject.cpp
+++ b/Source/bindings/tests/results/V8TestObject.cpp
@@ -851,7 +851,7 @@ static void
typedArrayAttrAttributeSetterCallback(v8::Local<v8::String>, v8::Loc
 static void attrWithGetterExceptionAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     int jsValue = imp->attrWithGetterException(es);
     if (UNLIKELY(es.throwIfNeeded()))
         return;
@@ -896,7 +896,7 @@ static void
attrWithSetterExceptionAttributeSetter(v8::Local<v8::Value> jsValue,
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
     V8TRYCATCH_VOID(int, cppValue, toInt32(jsValue));
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     imp->setAttrWithSetterException(cppValue, es);
     es.throwIfNeeded();
 }
@@ -911,7 +911,7 @@ static void
attrWithSetterExceptionAttributeSetterCallback(v8::Local<v8::String>
 static void stringAttrWithGetterExceptionAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     String jsValue = imp->stringAttrWithGetterException(es);
     if (UNLIKELY(es.throwIfNeeded()))
         return;
@@ -956,7 +956,7 @@ static void
stringAttrWithSetterExceptionAttributeSetter(v8::Local<v8::Value> js
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
     V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue,
jsValue);
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     imp->setStringAttrWithSetterException(cppValue, es);
     es.throwIfNeeded();
 }
@@ -1078,7 +1078,7 @@ static void
withActiveWindowAndFirstWindowAttributeAttributeSetterCallback(v8::L
 static void withScriptStateAttributeRaisesAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     ScriptState* currentState = ScriptState::current();
     if (!currentState)
         return v8Undefined();
@@ -1123,7 +1123,7 @@ static void
withScriptStateAttributeRaisesAttributeSetterCallback(v8::Local<v8::
 static void withExecutionContextAttributeRaisesAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     ExecutionContext* scriptContext = getExecutionContext();
     RefPtr<TestObj> jsValue =
imp->withExecutionContextAttributeRaises(scriptContext, es);
     if (UNLIKELY(es.throwIfNeeded()))
@@ -1195,7 +1195,7 @@ static void
withExecutionContextAndScriptStateAttributeAttributeSetterCallback(v
 static void
withExecutionContextAndScriptStateAttributeRaisesAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     ScriptState* currentState = ScriptState::current();
     if (!currentState)
         return v8Undefined();
@@ -1817,7 +1817,7 @@ static void
messagePortArrayAttributeSetterCallback(v8::Local<v8::String>, v8::L
 static void contentDocumentAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     if (!BindingSecurity::shouldAllowAccessToNode(imp->contentDocument(), es))
{
         v8SetReturnValueNull(info);
         es.throwIfNeeded();
@@ -2074,7 +2074,7 @@ static void
nullableLongSettableAttributeAttributeSetterCallback(v8::Local<v8::S
 static void nullableStringValueAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     bool isNull = false;
     int jsValue = imp->nullableStringValue(isNull, es);
     if (isNull) {
@@ -2803,7 +2803,7 @@ static void
locationWithExceptionAttributeSetter(v8::Local<v8::Value> jsValue, c
     if (!imp)
         return;
     V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue,
jsValue);
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     imp->setHrefThrows(cppValue, es);
     es.throwIfNeeded();
 }
@@ -3001,7 +3001,7 @@ static void methodThatRequiresAllArgsAndThrowsMethod(const
v8::FunctionCallbackI
         return;
     }
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, strArg, info[0]);
     V8TRYCATCH_VOID(TestObj*, objArg, V8TestObject::HasInstance(info[1],
info.GetIsolate(), worldType(info.GetIsolate())) ?
V8TestObject::toNative(v8::Handle<v8::Object>::Cast(info[1])) : 0);
     RefPtr<TestObj> result = imp->methodThatRequiresAllArgsAndThrows(strArg,
objArg, es);
@@ -3068,7 +3068,7 @@ static void optionsObjectMethodCallback(const
v8::FunctionCallbackInfo<v8::Value
 static void methodWithExceptionMethod(const
v8::FunctionCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     imp->methodWithException(es);
     if (es.throwIfNeeded())
         return;
@@ -3100,7 +3100,7 @@ static void addEventListenerMethod(const
v8::FunctionCallbackInfo<v8::Value>& in
 {
     EventTarget* impl = V8TestObject::toNative(info.Holder());
     if (DOMWindow* window = impl->toDOMWindow()) {
-        ExceptionState es(info.GetIsolate());
+        ExceptionState es(info.Holder(), info.GetIsolate());
         if (!BindingSecurity::shouldAllowAccessToFrame(window->frame(), es)) {
             es.throwIfNeeded();
             return;
@@ -3130,7 +3130,7 @@ static void removeEventListenerMethod(const
v8::FunctionCallbackInfo<v8::Value>&
 {
     EventTarget* impl = V8TestObject::toNative(info.Holder());
     if (DOMWindow* window = impl->toDOMWindow()) {
-        ExceptionState es(info.GetIsolate());
+        ExceptionState es(info.Holder(), info.GetIsolate());
         if (!BindingSecurity::shouldAllowAccessToFrame(window->frame(), es)) {
             es.throwIfNeeded();
             return;
@@ -3206,7 +3206,7 @@ static void withScriptStateObjMethodCallback(const
v8::FunctionCallbackInfo<v8::
 static void withScriptStateVoidExceptionMethod(const
v8::FunctionCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     ScriptState* currentState = ScriptState::current();
     if (!currentState)
         return;
@@ -3232,7 +3232,7 @@ static void
withScriptStateVoidExceptionMethodCallback(const v8::FunctionCallbac
 static void withScriptStateObjExceptionMethod(const
v8::FunctionCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     ScriptState* currentState = ScriptState::current();
     if (!currentState)
         return;
@@ -3297,7 +3297,7 @@ static void
withExecutionContextAndScriptStateMethodCallback(const v8::FunctionC
 static void withExecutionContextAndScriptStateObjExceptionMethod(const
v8::FunctionCallbackInfo<v8::Value>& info)
 {
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     ScriptState* currentState = ScriptState::current();
     if (!currentState)
         return;
@@ -4198,7 +4198,7 @@ static void stringArrayFunctionMethod(const
v8::FunctionCallbackInfo<v8::Value>&
         return;
     }
     TestObj* imp = V8TestObject::toNative(info.Holder());
-    ExceptionState es(info.GetIsolate());
+    ExceptionState es(info.Holder(), info.GetIsolate());
     V8TRYCATCH_VOID(Vector<String>, values, toNativeArray<String>(info[0], 1,
info.GetIsolate()));
     Vector<String> result = imp->stringArrayFunction(values, es);
     if (es.throwIfNeeded())
@@ -4220,7 +4220,7 @@ static void domStringListFunctionMethod(const
v8::FunctionCallbackInfo<v8::…
(message too large)

[sof, 2013-11-13 16:44:47]

Commit lingered in the cq for too long, causing conflicts when being re-applied.

Possible to look over the rebase? Sorry about this.

[sof, 2013-11-14 06:22:42]

Hmm, why is a change like 919a1ab82ccd019e1b325ed96c7b249d55e8c356 not held back
while this is in the queue? Totally kills this patch.

[abarth-chromium, 2013-11-14 07:22:41]

On 2013/11/14 06:22:42, sof wrote:
> Hmm, why is a change like 919a1ab82ccd019e1b325ed96c7b249d55e8c356 not held
back
> while this is in the queue? Totally kills this patch.

Generally, we don't hold off on changes because they conflict with other
changes.  We just land them and resolve the conflicts afterwards.

[sof, 2013-11-14 07:25:40]

On 2013/11/14 07:22:41, abarth wrote:
> On 2013/11/14 06:22:42, sof wrote:
> > Hmm, why is a change like 919a1ab82ccd019e1b325ed96c7b249d55e8c356 not held
> back
> > while this is in the queue? Totally kills this patch.
> 
> Generally, we don't hold off on changes because they conflict with other
> changes.  We just land them and resolve the conflicts afterwards.

Sure. But this prior rebase needed a slight lgtm to have avoided such overhead.

[sof, 2013-11-14 08:16:14]

If someone could look over the patchset #3 rebase, I'd be grateful.

[marja, 2013-11-14 08:43:07]

On 2013/11/14 08:16:14, sof wrote:
> If someone could look over the patchset #3 rebase, I'd be grateful.

Was the rebase mechanical or was there anything non-trivial in it? If it was
purely mechanical, it should be ok to commit this since haraken has already l g
t m d.

(Removing myself from the reviewer list since haraken is already on this.)

[haraken, 2013-11-14 08:51:00]

Still LGTM.

[sof, 2013-11-14 08:52:41]

On 2013/11/14 08:43:07, marja wrote:
> On 2013/11/14 08:16:14, sof wrote:
> > If someone could look over the patchset #3 rebase, I'd be grateful.
> 
> Was the rebase mechanical or was there anything non-trivial in it? If it was
> purely mechanical, it should be ok to commit this since haraken has already l
g
> t m d.
> 

Thanks, very useful to know. I had to manually resolve conflicts to most files,
more so with patchset #3, which is why I'm asking for someone to quickly look
the end result over for slip-ups, if possible.

Local testing is fine, fwiw (trybot access is something I don't have.)

[commit-bot: I haz the power, 2013-11-14 08:53:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/68563003/400001

[commit-bot: I haz the power, 2013-11-14 12:09:01]

Change committed as 162010