Revert "bindings: Implement timers with V8Function"

This reverts commit 254369a5.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> 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/1220486
> Reviewed-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}

TBR=peria@chromium.org,yukishiino@chromium.org,haraken@chromium.org,japhet@chromium.org,timothygu@chromium.org

Change-Id: Ifaccc3374466b851fc28b10c63ed1397bdae635e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 866610
Reviewed-on: https://chromium-review.googlesource.com/1239216Reviewed-by: Frank Liberato <liberato@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593260}

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,7 +163,6 @@ 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,7 +36,6 @@
 #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"
@@ -53,8 +52,9 @@ namespace blink {
 
 ScheduledAction* ScheduledAction::Create(ScriptState* script_state,
                                          ExecutionContext* target,
-                                         V8Function* handler,
+                                         const ScriptValue& handler,
                                          const Vector<ScriptValue>& arguments) {
+  DCHECK(handler.IsFunction());
   if (!script_state->World().IsWorkerWorld()) {
     if (!BindingSecurity::ShouldAllowAccessToFrame(
             EnteredDOMWindow(script_state->GetIsolate()),
@@ -84,14 +84,12 @@ ScheduledAction* ScheduledAction::Create(ScriptState* script_state,
 
 ScheduledAction::~ScheduledAction() {
   // Verify that owning DOMTimer has eagerly disposed.
-  DCHECK(arguments_.IsEmpty());
-  DCHECK(!function_);
-  DCHECK(!script_state_);
+  DCHECK(info_.IsEmpty());
 }
 
 void ScheduledAction::Dispose() {
   code_ = String();
-  arguments_.clear();
+  info_.Clear();
   function_.Clear();
   script_state_->Reset();
   script_state_.Clear();
@@ -99,7 +97,6 @@ void ScheduledAction::Dispose() {
 
 void ScheduledAction::Trace(blink::Visitor* visitor) {
   visitor->Trace(script_state_);
-  visitor->Trace(function_);
 }
 
 void ScheduledAction::Execute(ExecutionContext* context) {
@@ -130,11 +127,15 @@ void ScheduledAction::Execute(ExecutionContext* context) {
 }
 
 ScheduledAction::ScheduledAction(ScriptState* script_state,
-                                 V8Function* function,
+                                 const ScriptValue& function,
                                  const Vector<ScriptValue>& arguments)
     : ScheduledAction(script_state) {
-  function_ = function;
-  arguments_ = arguments;
+  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());
 }
 
 ScheduledAction::ScheduledAction(ScriptState* script_state, const String& code)
@@ -143,34 +144,29 @@ ScheduledAction::ScheduledAction(ScriptState* script_state, const String& code)
 }
 
 ScheduledAction::ScheduledAction(ScriptState* script_state)
-    : script_state_(ScriptStateProtectingContext::Create(script_state)) {}
+    : script_state_(ScriptStateProtectingContext::Create(script_state)),
+      info_(script_state->GetIsolate()) {}
 
-// 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_) {
+  if (!function_.IsEmpty()) {
     DVLOG(1) << "ScheduledAction::execute " << this << ": have 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_);
+    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());
   } else {
     DVLOG(1) << "ScheduledAction::execute " << this
              << ": executing from source";
@@ -185,7 +181,6 @@ 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());
 
@@ -194,25 +189,22 @@ void ScheduledAction::Execute(WorkerGlobalScope* worker) {
     return;
   }
 
-  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_);
+  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());
   } else {
     worker->ScriptController()->Evaluate(
         ScriptSourceCode(code_,
@@ -221,4 +213,12 @@ 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,28 +31,29 @@
 #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/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/bindings/core/v8/v8_persistent_value_vector.h"
+#include "third_party/blink/renderer/platform/bindings/scoped_persistent.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 ExecutionContext;
 class LocalFrame;
+class ExecutionContext;
+class ScriptValue;
 class WorkerGlobalScope;
 
-class ScheduledAction final : public GarbageCollectedFinalized<ScheduledAction>,
-                              public NameClient {
+class ScheduledAction final
+    : public GarbageCollectedFinalized<ScheduledAction> {
   WTF_MAKE_NONCOPYABLE(ScheduledAction);
 
  public:
   static ScheduledAction* Create(ScriptState*,
                                  ExecutionContext* target,
-                                 V8Function* handler,
+                                 const ScriptValue& handler,
                                  const Vector<ScriptValue>& arguments);
   static ScheduledAction* Create(ScriptState*,
                                  ExecutionContext* target,
@@ -62,13 +63,12 @@ class ScheduledAction final : public GarbageCollectedFinalized<ScheduledAction>,
   void Dispose();
 
   void Trace(blink::Visitor*);
-  const char* NameInHeapSnapshot() const override { return "ScheduledAction"; }
 
   void Execute(ExecutionContext*);
 
  private:
   ScheduledAction(ScriptState*,
-                  V8Function* handler,
+                  const ScriptValue& handler,
                   const Vector<ScriptValue>& arguments);
   ScheduledAction(ScriptState*, const String& handler);
 
@@ -77,10 +77,11 @@ class ScheduledAction final : public GarbageCollectedFinalized<ScheduledAction>,
 
   void Execute(LocalFrame*);
   void Execute(WorkerGlobalScope*);
+  void CreateLocalHandlesForArgs(Vector<v8::Local<v8::Value>>* handles);
 
   Member<ScriptStateProtectingContext> script_state_;
-  TraceWrapperMember<V8Function> function_;
-  Vector<ScriptValue> arguments_;
+  ScopedPersistent<v8::Function> function_;
+  V8PersistentValueVector<v8::Value> info_;
   String code_;
 };
 

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

@@ -174,9 +174,6 @@ 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)
@@ -185,6 +182,8 @@ 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,8 +32,6 @@
 #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 {
@@ -41,8 +39,7 @@ namespace blink {
 class ExecutionContext;
 
 class CORE_EXPORT DOMTimer final : public GarbageCollectedFinalized<DOMTimer>,
-                                   public PausableTimer,
-                                   public NameClient {
+                                   public PausableTimer {
   USING_GARBAGE_COLLECTED_MIXIN(DOMTimer);
 
  public:
@@ -65,7 +62,6 @@ 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;
 
@@ -91,7 +87,7 @@ class CORE_EXPORT DOMTimer final : public GarbageCollectedFinalized<DOMTimer>,
 
   int timeout_id_;
   int nesting_level_;
-  TraceWrapperMember<ScheduledAction> action_;
+  Member<ScheduledAction> action_;
   scoped_refptr<UserGestureToken> user_gesture_token_;
 };
 

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

@@ -9,7 +9,6 @@
 
 #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"
 
@@ -60,7 +59,7 @@ class DOMTimerCoordinator {
  private:
   int NextID();
 
-  using TimeoutMap = HeapHashMap<int, TraceWrapperMember<DOMTimer>>;
+  using TimeoutMap = HeapHashMap<int, Member<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,
-               V8Function* handler,
+               const ScriptValue& 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,
-                V8Function* handler,
+                const ScriptValue& handler,
                 int timeout,
                 const Vector<ScriptValue>& arguments) {
   ExecutionContext* execution_context = event_target.GetExecutionContext();

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

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

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

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