Implement WindowTimers.set{Timeout,Interval} without [Custom]

Source/core/frame/DOMWindowTimers.cpp

Index: Source/core/frame/DOMWindowTimers.cpp
diff --git a/Source/core/frame/DOMWindowTimers.cpp b/Source/core/frame/DOMWindowTimers.cpp
index 7f9aae6a3c4af84bcb2ef6da34038d909de23db0..2eb3692f19544447bae8a394f3b4bf23625fcd0a 100644
--- a/Source/core/frame/DOMWindowTimers.cpp
+++ b/Source/core/frame/DOMWindowTimers.cpp
@@ -33,21 +33,91 @@
 #include "config.h"
 #include "core/frame/DOMWindowTimers.h"
 
+#include "core/dom/Document.h"
+#include "core/dom/ExecutionContext.h"
 #include "core/events/EventTarget.h"
 #include "core/frame/DOMTimer.h"
+#include "core/frame/csp/ContentSecurityPolicy.h"
+#include "core/workers/WorkerGlobalScope.h"
 
 namespace blink {
 
 namespace DOMWindowTimers {
 
-int setTimeout(EventTarget& eventTarget, PassOwnPtr<ScheduledAction> action, int timeout)
+static bool isAllowed(ExecutionContext* executionContext, bool isEval)
 {
-    return DOMTimer::install(eventTarget.executionContext(), action, timeout, true);
+    if (executionContext->isDocument()) {
+        Document* document = static_cast<Document*>(executionContext);
+        if (isEval && document->contentSecurityPolicy()->allowEval())
+            return false;

[haraken, 2014/11/13 12:59:31]

Shouldn't this return true?

[Jens Widell, 2014/11/13 13:12:42]

I was intending to have

  if (!allowEval())
    return false;
  return true;

but lost the ! somewhere along the way. Since we only have one check for
documents, the other way works too. If we had multiple independent checks, "if
(!condition) return false; return true;" scales better.

[Jens Widell, 2014/11/13 13:26:48]

I've fixed the logic here to be what I initially intended.

+        return true;

[haraken, 2014/11/13 12:59:32]

return false?

+    }
+    if (executionContext->isWorkerGlobalScope()) {
+        WorkerGlobalScope* workerGlobalScope = static_cast<WorkerGlobalScope*>(executionContext);
+        if (!workerGlobalScope->script())
+            return false;
+        ContentSecurityPolicy* policy = workerGlobalScope->contentSecurityPolicy();
+        if (isEval && policy && !policy->allowEval())
+            return false;

[haraken, 2014/11/13 12:59:31]

I guess this should be:

if (isEval && policy && policy->allowEval())
  return true;
return false;

?

+        return true;
+    }
+    ASSERT_NOT_REACHED();
+    return false;
 }
 
-int setInterval(EventTarget& eventTarget, PassOwnPtr<ScheduledAction> action, int timeout)
+static PassOwnPtr<ScheduledAction> makeScheduledAction(ScriptState* scriptState, const ScriptValue& handler, const Vector<ScriptValue>& arguments)
 {
-    return DOMTimer::install(eventTarget.executionContext(), action, timeout, false);
+    ASSERT(handler.isFunction());
+    return adoptPtr(new ScheduledAction(scriptState, handler, arguments, scriptState->isolate()));
+}
+
+static PassOwnPtr<ScheduledAction> makeScheduledAction(ScriptState* scriptState, String handler)

[haraken, 2014/11/13 12:59:32]

Instead of adding makeScheduledAction(), we should add
SchedulerAction::create().

[Jens Widell, 2014/11/13 13:26:48]

Done.

+{
+    return adoptPtr(new ScheduledAction(scriptState, handler, KURL(), scriptState->isolate()));
+}
+
+int setTimeout(ScriptState* scriptState, EventTarget& eventTarget, const ScriptValue& handler, int timeout, const Vector<ScriptValue>& arguments)
+{
+    ExecutionContext* executionContext = eventTarget.executionContext();

[Jens Widell, 2014/11/13 12:37:47]

The old custom code in V8WindowCustom.cpp had an !impl->document() check. An
equivalent check here would be to check if executionContext is null, I guess.

[haraken, 2014/11/13 12:59:31]

I think we should check against scriptState->executionContext().
scriptState->executionContext() is more reliable because it retrieves a right
ExecutionContext from the current ScriptState.

[Jens Widell, 2014/11/13 13:12:42]

But isn't scriptState the calling script, whereas impl->executionContext() (or
eventTarget.executionContext() here) is the environment in which we should set
the timeout?

For "window.setTimeout(...)" they're the same, but for
"iframe.contentWindow.setTimeout(...)" they're not. I think.

+    if (!isAllowed(executionContext, false))

[haraken, 2014/11/13 12:59:32]

Do we need to call isAllowed() when isEval==false? If isEval==false, I guess
isAllowed() should always return true.

[Jens Widell, 2014/11/13 13:12:41]

I might be badly named for what I intended it to be. I meant to put all the
checks from the old custom code that caused us to bail out early in it. For
instance the "workerGlobalScope->script()" check, which is unrelated to whether
it's an "eval".

+        return 0;
+    OwnPtr<ScheduledAction> action = makeScheduledAction(scriptState, handler, arguments);
+    return DOMTimer::install(executionContext, action.release(), timeout, true);
+}
+
+int setTimeout(ScriptState* scriptState, EventTarget& eventTarget, String handler, int timeout, const Vector<ScriptValue>&)
+{
+    ExecutionContext* executionContext = eventTarget.executionContext();
+    if (!isAllowed(executionContext, true))
+        return 0;
+    // Don't allow setting timeouts to run empty functions!
+    // (Bug 1009597)
+    if (handler.isEmpty())
+        return 0;
+    OwnPtr<ScheduledAction> action = makeScheduledAction(scriptState, handler);
+    return DOMTimer::install(executionContext, action.release(), timeout, true);
+}
+
+int setInterval(ScriptState* scriptState, EventTarget& eventTarget, const ScriptValue& handler, int timeout, const Vector<ScriptValue>& arguments)
+{
+    ExecutionContext* executionContext = eventTarget.executionContext();
+    if (!isAllowed(executionContext, false))
+        return 0;
+    OwnPtr<ScheduledAction> action = makeScheduledAction(scriptState, handler, arguments);
+    return DOMTimer::install(executionContext, action.release(), timeout, false);
+}
+
+int setInterval(ScriptState* scriptState, EventTarget& eventTarget, String handler, int timeout, const Vector<ScriptValue>&)
+{
+    ExecutionContext* executionContext = eventTarget.executionContext();
+    if (!isAllowed(executionContext, true))
+        return 0;
+    // Don't allow setting timeouts to run empty functions!
+    // (Bug 1009597)
+    if (handler.isEmpty())
+        return 0;
+    OwnPtr<ScheduledAction> action = makeScheduledAction(scriptState, handler);
+    return DOMTimer::install(executionContext, action.release(), timeout, false);
 }
 
 void clearTimeout(EventTarget& eventTarget, int timeoutID)