Skip to content

GitLab

T
tangled
Commit 892067a4 authored by Timothy Gu's avatar Timothy Gu Committed by Commit Bot
Browse files
Options

Revert "Reland "bindings: Implement timers with V8Function"" 

This reverts commit 17f4e7d2.

Reason for revert: Still breaks ASAN

Original change's description:
> Reland "bindings: Implement timers with V8Function"
> 
> This is a reland of 254369a5. It addresses bug
> 888025 by adding ASAN test expectations, as the relevant V8 feature does not
> yet support running on ASAN builds.
> 
> 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=japhet@chromium.org
> 
> Bug: 866610, 888025
> Change-Id: Iee5c1d6917ad7770383e06a425f96000835a663a
> Reviewed-on: https://chromium-review.googlesource.com/c/1239624
> Reviewed-by: Nate Chapin <japhet@chromium.org>
> Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Commit-Queue: Timothy Gu <timothygu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601830}

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

Change-Id: Ie4f45dfcc1adcc2ac3469eab99dba813723288f4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 866610, 888025
Reviewed-on: https://chromium-review.googlesource.com/c/1296057
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601864}
parent 26111c26
Hide whitespace changes
Inline Side-by-side
Showing with 78 additions and 129 deletions
third_party/WebKit/LayoutTests/ASANExpectations
View file @ 892067a4
......@@ -88,25 +88,3 @@ crbug.com/838057 [ Linux ] virtual/outofblink-cors/external/wpt/service-workers/
# Disabled by sheriff due to test crash
crbug.com/896068 [ Linux ] webaudio/AudioBuffer/huge-buffer.html [ Crash ]
crbug.com/896068 [ Linux ] webaudio/dom-exceptions.html [ Pass Crash ]
# V8's incumbent context tracking does not work with ASAN yet
crbug.com/888867 external/wpt/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html [ Timeout ]
crbug.com/888867 external/wpt/html/browsers/browsing-the-web/unloading-documents/unload/007.html [ Timeout ]
crbug.com/888867 external/wpt/html/browsers/the-window-object/garbage-collection-and-browsing-contexts/discard_iframe_history_3.html [ Timeout ]
crbug.com/888867 external/wpt/html/browsers/the-window-object/garbage-collection-and-browsing-contexts/discard_iframe_history_4.html [ Timeout ]
crbug.com/888867 external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url-entry-document.window.html [ Timeout ]
crbug.com/888867 external/wpt/xhr/open-url-multi-window-2.htm [ Timeout ]
crbug.com/888867 external/wpt/xhr/open-url-multi-window-3.htm [ Timeout ]
crbug.com/888867 fast/forms/calendar-picker/calendar-picker-type-change-onchange.html [ Timeout ]
crbug.com/888867 http/tests/appcache/crash-when-navigating-away-then-back.html [ Timeout ]
crbug.com/888867 http/tests/devtools/sources/debugger-pause/debugger-resume-button-in-overlay.js [ Timeout ]
crbug.com/888867 http/tests/loading/onbeforeunload-detach.html [ Timeout ]
crbug.com/888867 http/tests/loading/onreadystatechange-detach.html [ Timeout ]
crbug.com/888867 http/tests/misc/delete-frame-during-readystatechange-with-gc-after-video-removal.html [ Timeout ]
crbug.com/888867 http/tests/misc/delete-frame-during-readystatechange.html [ Timeout ]
crbug.com/888867 http/tests/misc/frame-detached-in-animationstart-event.html [ Timeout ]
crbug.com/888867 http/tests/workers/shared-worker-lifecycle.html [ Timeout ]
crbug.com/888867 virtual/outofblink-cors/external/wpt/xhr/open-url-multi-window-2.htm [ Timeout ]
crbug.com/888867 virtual/outofblink-cors/external/wpt/xhr/open-url-multi-window-3.htm [ Timeout ]
crbug.com/888867 virtual/outofblink-cors-ns/external/wpt/xhr/open-url-multi-window-2.htm [ Timeout ]
crbug.com/888867 virtual/outofblink-cors-ns/external/wpt/xhr/open-url-multi-window-3.htm [ Timeout ]
third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url-entry-document.window.js deleted 100644 → 0
View file @ 26111c26
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
View file @ 892067a4
......@@ -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
View file @ 892067a4
......@@ -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"
......@@ -54,8 +53,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()),
......@@ -85,14 +85,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();
......@@ -100,7 +98,6 @@ void ScheduledAction::Dispose() {
void ScheduledAction::Trace(blink::Visitor* visitor) {
visitor->Trace(script_state_);
visitor->Trace(function_);
}
void ScheduledAction::Execute(ExecutionContext* context) {
......@@ -131,11 +128,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)
......@@ -144,34 +145,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";
......@@ -190,7 +186,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());
......@@ -199,25 +194,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 {
// We're using |kSharableCrossOrigin| to keep the existing behavior, but
// this causes failures on
......@@ -230,4 +222,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
View file @ 892067a4
......@@ -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
View file @ 892067a4
......@@ -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
View file @ 892067a4
......@@ -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
View file @ 892067a4
......@@ -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
View file @ 892067a4
......@@ -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
View file @ 892067a4
......@@ -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
View file @ 892067a4
......@@ -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);
};
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment