bindings: Implement timers with V8Function

This fixes bug 866610 by using the IDL infrastructure to properly enter
the v8::Context before calling the registered callback.

Also ensure eager finalization of ScheduledAction in DOMTimer to
prevent a memory leak. Added two more effective DCHECKs to confirm.

Bug: 866610
Change-Id: I37d7bd05f035fe31856cfe68bae51aa0632cd3b1
Reviewed-on: https://chromium-review.googlesource.com/1220486Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593108}

third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/resources/url-entry-document-timer-frame.html

+<script>
+setTimeout(parent.timerTest, 10);
+</script>

third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url-entry-document.window.js

+async_test(t => {
+  const frame = document.body.appendChild(document.createElement("iframe"));
+  t.add_cleanup(() => frame.remove());
+  const frameURL = new URL("resources/url-entry-document-timer-frame.html", document.URL).href;
+  window.timerTest = t.step_func_done(() => {
+    assert_equals(frame.contentDocument.URL, frameURL);
+    assert_equals(frame.contentWindow.location.href, frameURL);
+
+    // In this case, the entry settings object was set when this function is
+    // executed in the timer task through Web IDL's "invoke a callback
+    // function" algorithm, to be the relevant settings object of this
+    // function. Therefore the URL of this document would be inherited.
+    assert_equals(frame.contentDocument.open(), frame.contentDocument);
+    assert_equals(frame.contentDocument.URL, document.URL);
+    assert_equals(frame.contentWindow.location.href, document.URL);
+  });
+  frame.src = frameURL;
+}, "document.open() changes document's URL to the entry settings object's responsible document's (through timeouts)");

third_party/WebKit/LayoutTests/external/wpt/lint.whitelist

@@ -163,6 +163,7 @@ SET TIMEOUT: html/semantics/embedded-content/the-img-element/*
 SET TIMEOUT: html/semantics/scripting-1/the-script-element/*
 SET TIMEOUT: html/webappapis/dynamic-markup-insertion/opening-the-input-stream/0*
 SET TIMEOUT: html/webappapis/dynamic-markup-insertion/opening-the-input-stream/resources/history-frame.html
+SET TIMEOUT: html/webappapis/dynamic-markup-insertion/opening-the-input-stream/resources/url-entry-document-timer-frame.html
 SET TIMEOUT: html/webappapis/dynamic-markup-insertion/opening-the-input-stream/tasks.window.js
 SET TIMEOUT: html/webappapis/scripting/event-loops/*
 SET TIMEOUT: html/webappapis/scripting/events/event-handler-processing-algorithm-error/*

third_party/blink/renderer/bindings/core/v8/scheduled_action.cc

@@ -36,6 +36,7 @@
 #include "third_party/blink/renderer/bindings/core/v8/script_value.h"
 #include "third_party/blink/renderer/bindings/core/v8/source_location.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
+#include "third_party/blink/renderer/bindings/core/v8/v8_function.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_gc_controller.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_script_runner.h"
 #include "third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.h"
@@ -52,9 +53,8 @@ namespace blink {
 
 ScheduledAction* ScheduledAction::Create(ScriptState* script_state,
                                          ExecutionContext* target,
-                                         const ScriptValue& handler,
+                                         V8Function* handler,
                                          const Vector<ScriptValue>& arguments) {
-  DCHECK(handler.IsFunction());
   if (!script_state->World().IsWorkerWorld()) {
     if (!BindingSecurity::ShouldAllowAccessToFrame(
             EnteredDOMWindow(script_state->GetIsolate()),
@@ -84,12 +84,14 @@ ScheduledAction* ScheduledAction::Create(ScriptState* script_state,
 
 ScheduledAction::~ScheduledAction() {
   // Verify that owning DOMTimer has eagerly disposed.
-  DCHECK(info_.IsEmpty());
+  DCHECK(arguments_.IsEmpty());
+  DCHECK(!function_);
+  DCHECK(!script_state_);
 }
 
 void ScheduledAction::Dispose() {
   code_ = String();
-  info_.Clear();
+  arguments_.clear();
   function_.Clear();
   script_state_->Reset();
   script_state_.Clear();
@@ -97,6 +99,7 @@ void ScheduledAction::Dispose() {
 
 void ScheduledAction::Trace(blink::Visitor* visitor) {
   visitor->Trace(script_state_);
+  visitor->Trace(function_);
 }
 
 void ScheduledAction::Execute(ExecutionContext* context) {
@@ -127,15 +130,11 @@ void ScheduledAction::Execute(ExecutionContext* context) {
 }
 
 ScheduledAction::ScheduledAction(ScriptState* script_state,
-                                 const ScriptValue& function,
+                                 V8Function* function,
                                  const Vector<ScriptValue>& arguments)
     : ScheduledAction(script_state) {
-  DCHECK(function.IsFunction());
-  function_.Set(script_state->GetIsolate(),
-                v8::Local<v8::Function>::Cast(function.V8Value()));
-  info_.ReserveCapacity(arguments.size());
-  for (const ScriptValue& argument : arguments)
-    info_.Append(argument.V8Value());
+  function_ = function;
+  arguments_ = arguments;
 }
 
 ScheduledAction::ScheduledAction(ScriptState* script_state, const String& code)
@@ -144,29 +143,34 @@ ScheduledAction::ScheduledAction(ScriptState* script_state, const String& code)
 }
 
 ScheduledAction::ScheduledAction(ScriptState* script_state)
-    : script_state_(ScriptStateProtectingContext::Create(script_state)),
-      info_(script_state->GetIsolate()) {}
+    : script_state_(ScriptStateProtectingContext::Create(script_state)) {}
 
+// https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers
 void ScheduledAction::Execute(LocalFrame* frame) {
   DCHECK(script_state_->ContextIsValid());
 
   TRACE_EVENT0("v8", "ScheduledAction::execute");
-  if (!function_.IsEmpty()) {
+  if (function_) {
     DVLOG(1) << "ScheduledAction::execute " << this << ": have function";
-    v8::Local<v8::Function> function =
-        function_.NewLocal(script_state_->GetIsolate());
-    ScriptState* script_state_for_func =
-        ScriptState::From(function->CreationContext());
-    if (!script_state_for_func->ContextIsValid()) {
-      DVLOG(1) << "ScheduledAction::execute " << this
-               << ": function's context is empty";
-      return;
-    }
-    Vector<v8::Local<v8::Value>> info;
-    CreateLocalHandlesForArgs(&info);
-    V8ScriptRunner::CallFunction(
-        function, frame->GetDocument(), script_state_->GetContext()->Global(),
-        info.size(), info.data(), script_state_->GetIsolate());
+
+    // The method context is set in
+    // https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-settimeout
+    // as "the object on which the method for which the algorithm is running is
+    // implemented (a Window or WorkerGlobalScope object)."
+    ScriptWrappable* method_context =
+        ToScriptWrappable(script_state_->GetContext()->Global());
+
+    // 7.2. Run the appropriate set of steps from the following list:
+    //
+    //      If the first method argument is a Function:
+    //
+    //        Invoke the Function. Use the third and subsequent method
+    //        arguments (if any) as the arguments for invoking the Function.
+    //        Use method context proxy as the callback this value.
+    //
+    // InvokeAndReportException() is responsible for converting the method
+    // context to the method context proxy, if needed.
+    function_->InvokeAndReportException(method_context, arguments_);
   } else {
     DVLOG(1) << "ScheduledAction::execute " << this
              << ": executing from source";
@@ -181,6 +185,7 @@ void ScheduledAction::Execute(LocalFrame* frame) {
   // released it.
 }
 
+// https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers
 void ScheduledAction::Execute(WorkerGlobalScope* worker) {
   DCHECK(worker->GetThread()->IsCurrentThread());
 
@@ -189,22 +194,25 @@ void ScheduledAction::Execute(WorkerGlobalScope* worker) {
     return;
   }
 
-  if (!function_.IsEmpty()) {
-    ScriptState::Scope scope(script_state_->Get());
-    v8::Local<v8::Function> function =
-        function_.NewLocal(script_state_->GetIsolate());
-    ScriptState* script_state_for_func =
-        ScriptState::From(function->CreationContext());
-    if (!script_state_for_func->ContextIsValid()) {
-      DVLOG(1) << "ScheduledAction::execute " << this
-               << ": function's context is empty";
-      return;
-    }
-    Vector<v8::Local<v8::Value>> info;
-    CreateLocalHandlesForArgs(&info);
-    V8ScriptRunner::CallFunction(
-        function, worker, script_state_->GetContext()->Global(), info.size(),
-        info.data(), script_state_->GetIsolate());
+  if (function_) {
+    // The method context is set in
+    // https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-settimeout
+    // as "the object on which the method for which the algorithm is running is
+    // implemented (a Window or WorkerGlobalScope object)."
+    ScriptWrappable* method_context =
+        ToScriptWrappable(script_state_->GetContext()->Global());
+
+    // 7.2. Run the appropriate set of steps from the following list:
+    //
+    //      If the first method argument is a Function:
+    //
+    //         Invoke the Function. Use the third and subsequent method
+    //         arguments (if any) as the arguments for invoking the Function.
+    //         Use method context proxy as the callback this value.
+    //
+    // InvokeAndReportException() is responsible for converting the method
+    // context to the method context proxy, if needed.
+    function_->InvokeAndReportException(method_context, arguments_);
   } else {
     worker->ScriptController()->Evaluate(
         ScriptSourceCode(code_,
@@ -213,12 +221,4 @@ void ScheduledAction::Execute(WorkerGlobalScope* worker) {
   }
 }
 
-void ScheduledAction::CreateLocalHandlesForArgs(
-    Vector<v8::Local<v8::Value>>* handles) {
-  wtf_size_t handle_count = SafeCast<wtf_size_t>(info_.Size());
-  handles->ReserveCapacity(handle_count);
-  for (wtf_size_t i = 0; i < handle_count; ++i)
-    handles->push_back(info_.Get(i));
-}
-
 }  // namespace blink

third_party/blink/renderer/bindings/core/v8/scheduled_action.h

@@ -31,29 +31,28 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_BINDINGS_CORE_V8_SCHEDULED_ACTION_H_
 #define THIRD_PARTY_BLINK_RENDERER_BINDINGS_CORE_V8_SCHEDULED_ACTION_H_
 
-#include "third_party/blink/renderer/bindings/core/v8/v8_persistent_value_vector.h"
-#include "third_party/blink/renderer/platform/bindings/scoped_persistent.h"
+#include "third_party/blink/renderer/bindings/core/v8/script_value.h"
+#include "third_party/blink/renderer/bindings/core/v8/v8_function.h"
+#include "third_party/blink/renderer/platform/bindings/name_client.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_member.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
-#include "third_party/blink/renderer/platform/wtf/forward.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
-#include "v8/include/v8.h"
 
 namespace blink {
 
-class LocalFrame;
 class ExecutionContext;
-class ScriptValue;
+class LocalFrame;
 class WorkerGlobalScope;
 
-class ScheduledAction final
-    : public GarbageCollectedFinalized<ScheduledAction> {
+class ScheduledAction final : public GarbageCollectedFinalized<ScheduledAction>,
+                              public NameClient {
   WTF_MAKE_NONCOPYABLE(ScheduledAction);
 
  public:
   static ScheduledAction* Create(ScriptState*,
                                  ExecutionContext* target,
-                                 const ScriptValue& handler,
+                                 V8Function* handler,
                                  const Vector<ScriptValue>& arguments);
   static ScheduledAction* Create(ScriptState*,
                                  ExecutionContext* target,
@@ -63,12 +62,13 @@ class ScheduledAction final
   void Dispose();
 
   void Trace(blink::Visitor*);
+  const char* NameInHeapSnapshot() const override { return "ScheduledAction"; }
 
   void Execute(ExecutionContext*);
 
  private:
   ScheduledAction(ScriptState*,
-                  const ScriptValue& handler,
+                  V8Function* handler,
                   const Vector<ScriptValue>& arguments);
   ScheduledAction(ScriptState*, const String& handler);
 
@@ -77,11 +77,10 @@ class ScheduledAction final
 
   void Execute(LocalFrame*);
   void Execute(WorkerGlobalScope*);
-  void CreateLocalHandlesForArgs(Vector<v8::Local<v8::Value>>* handles);
 
   Member<ScriptStateProtectingContext> script_state_;
-  ScopedPersistent<v8::Function> function_;
-  V8PersistentValueVector<v8::Value> info_;
+  TraceWrapperMember<V8Function> function_;
+  Vector<ScriptValue> arguments_;
   String code_;
 };
 

third_party/blink/renderer/core/frame/dom_timer.cc

@@ -174,6 +174,9 @@ void DOMTimer::Fired() {
 
   action->Execute(context);
 
+  // Eagerly clear out |action|'s resources.
+  action->Dispose();
+
   // ExecutionContext might be already gone when we executed action->execute().
   ExecutionContext* execution_context = GetExecutionContext();
   if (!execution_context)
@@ -182,8 +185,6 @@ void DOMTimer::Fired() {
   execution_context->Timers()->SetTimerNestingLevel(0);
   // Eagerly unregister as ExecutionContext observer.
   ClearContext();
-  // Eagerly clear out |action|'s resources.
-  action->Dispose();
 }
 
 scoped_refptr<base::SingleThreadTaskRunner> DOMTimer::TimerTaskRunner() const {

third_party/blink/renderer/core/frame/dom_timer.h

@@ -32,6 +32,8 @@
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/dom/user_gesture_indicator.h"
 #include "third_party/blink/renderer/core/frame/pausable_timer.h"
+#include "third_party/blink/renderer/platform/bindings/name_client.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_member.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 
 namespace blink {
@@ -39,7 +41,8 @@ namespace blink {
 class ExecutionContext;
 
 class CORE_EXPORT DOMTimer final : public GarbageCollectedFinalized<DOMTimer>,
-                                   public PausableTimer {
+                                   public PausableTimer,
+                                   public NameClient {
   USING_GARBAGE_COLLECTED_MIXIN(DOMTimer);
 
  public:
@@ -62,6 +65,7 @@ class CORE_EXPORT DOMTimer final : public GarbageCollectedFinalized<DOMTimer>,
   // already have been finalized & must not be accessed.
   EAGERLY_FINALIZE();
   void Trace(blink::Visitor*) override;
+  const char* NameInHeapSnapshot() const override { return "DOMTimer"; }
 
   void Stop() override;
 
@@ -87,7 +91,7 @@ class CORE_EXPORT DOMTimer final : public GarbageCollectedFinalized<DOMTimer>,
 
   int timeout_id_;
   int nesting_level_;
-  Member<ScheduledAction> action_;
+  TraceWrapperMember<ScheduledAction> action_;
   scoped_refptr<UserGestureToken> user_gesture_token_;
 };
 

third_party/blink/renderer/core/frame/dom_timer_coordinator.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/single_thread_task_runner.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_member.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/wtf/time.h"
 
@@ -59,7 +60,7 @@ class DOMTimerCoordinator {
  private:
   int NextID();
 
-  using TimeoutMap = HeapHashMap<int, Member<DOMTimer>>;
+  using TimeoutMap = HeapHashMap<int, TraceWrapperMember<DOMTimer>>;
   TimeoutMap timers_;
 
   int circular_sequential_id_;

third_party/blink/renderer/core/frame/dom_window_timers.cc

@@ -84,7 +84,7 @@ static bool IsAllowed(ScriptState* script_state,
 
 int setTimeout(ScriptState* script_state,
                EventTarget& event_target,
-               const ScriptValue& handler,
+               V8Function* handler,
                int timeout,
                const Vector<ScriptValue>& arguments) {
   ExecutionContext* execution_context = event_target.GetExecutionContext();
@@ -144,7 +144,7 @@ int setTimeoutFromString(ScriptState* script_state,
 
 int setInterval(ScriptState* script_state,
                 EventTarget& event_target,
-                const ScriptValue& handler,
+                V8Function* handler,
                 int timeout,
                 const Vector<ScriptValue>& arguments) {
   ExecutionContext* execution_context = event_target.GetExecutionContext();

third_party/blink/renderer/core/frame/dom_window_timers.h

@@ -43,11 +43,12 @@ class ExceptionState;
 class ScriptState;
 class ScriptValue;
 class StringOrTrustedScript;
+class V8Function;
 
 namespace DOMWindowTimers {
 int setTimeout(ScriptState*,
                EventTarget&,
-               const ScriptValue& handler,
+               V8Function* handler,
                int timeout,
                const Vector<ScriptValue>& arguments);
 int setTimeout(ScriptState*,
@@ -63,7 +64,7 @@ int setTimeoutFromString(ScriptState*,
                          const Vector<ScriptValue>&);
 int setInterval(ScriptState*,
                 EventTarget&,
-                const ScriptValue& handler,
+                V8Function* handler,
                 int timeout,
                 const Vector<ScriptValue>&);
 int setInterval(ScriptState*,

third_party/blink/renderer/core/frame/window_timers.idl

@@ -34,13 +34,14 @@
     NoInterfaceObject, // Always used on target of 'implements'
     Exposed=(Window,Worker)
 ] interface WindowTimers {
-    // TODO(yukishiino): Use TimerHandler (or Function at least) to implement
-    // setTimeout and setInterval.
+    // TODO(yukishiino): Use TimerHandler to implement setTimeout and
+    // setInterval, when the IDL code generator supports unions of a callback
+    // function and a string.
     // https://html.spec.whatwg.org/C/webappapis.html#windoworworkerglobalscope-mixin
-    [CallWith=ScriptState, RuntimeCallStatsCounter=WindowSetTimeout] long setTimeout(CallbackFunctionTreatedAsScriptValue handler, optional long timeout = 0, any... arguments);
+    [CallWith=ScriptState, RuntimeCallStatsCounter=WindowSetTimeout] long setTimeout(Function handler, optional long timeout = 0, any... arguments);
     [CallWith=ScriptState, RaisesException] long setTimeout(ScriptString handler, optional long timeout = 0, any... arguments);
     void clearTimeout(optional long handle = 0);
-    [CallWith=ScriptState] long setInterval(CallbackFunctionTreatedAsScriptValue handler, optional long timeout = 0, any... arguments);
+    [CallWith=ScriptState] long setInterval(Function handler, optional long timeout = 0, any... arguments);
     [CallWith=ScriptState, RaisesException] long setInterval(ScriptString handler, optional long timeout = 0, any... arguments);
     void clearInterval(optional long handle = 0);
 };