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

Source/core/frame/DOMWindowTimers.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2013 Samsung Electronics. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
@@ skipping 15 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #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)
 {
     if (ExecutionContext* context = eventTarget.executionContext())
         DOMTimer::removeByID(context, timeoutID);
 }
 
 void clearInterval(EventTarget& eventTarget, int timeoutID)
 {
     if (ExecutionContext* context = eventTarget.executionContext())
         DOMTimer::removeByID(context, timeoutID);
 }
 
 } // namespace DOMWindowTimers
 
 } // namespace blink