Commit c0418c74 authored by sigbjornf's avatar sigbjornf Committed by Commit bot

Revert of Eagerly dispose of ScheduledActions. (patchset #2 id:20001 of...

Revert of Eagerly dispose of ScheduledActions. (patchset #2 id:20001 of https://codereview.chromium.org/2552673002/ )

Reason for revert:
Speculative revert for reported perf decrease on system_health.memory_mobile, https://crbug.com/672098

Original issue's description:
> Eagerly dispose of ScheduledActions.
>
> The DOMTimer's ScheduledAction hold on to the script source and
> state needed to execute the timer action. Let go of ShceduledAction's
> resource early.
>
> Apart from reducing the lifetime of script source, this is a speculative
> fix for crashes reported in v8::PersistentValueVector::Clear() during
> lazy sweeping of ScheduledAction objects.
>
> R=
> BUG=
>
> Committed: https://crrev.com/11bd50343795ed1dc1977da91e9a1588687522fd
> Cr-Commit-Position: refs/heads/master@{#436298}

TBR=oilpan-reviews@chromium.org,haraken@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=

Review-Url: https://codereview.chromium.org/2568103002
Cr-Commit-Position: refs/heads/master@{#437889}
parent b559a094
...@@ -61,17 +61,7 @@ DEFINE_TRACE(ScheduledAction) { ...@@ -61,17 +61,7 @@ DEFINE_TRACE(ScheduledAction) {
visitor->trace(m_code); visitor->trace(m_code);
} }
ScheduledAction::~ScheduledAction() { ScheduledAction::~ScheduledAction() {}
// Verify that owning DOMTimer has eagerly disposed.
DCHECK(m_info.IsEmpty());
}
void ScheduledAction::dispose() {
m_code.dispose();
m_info.Clear();
m_function.clear();
m_scriptState.clear();
}
void ScheduledAction::execute(ExecutionContext* context) { void ScheduledAction::execute(ExecutionContext* context) {
if (context->isDocument()) { if (context->isDocument()) {
......
...@@ -56,8 +56,6 @@ class ScheduledAction final ...@@ -56,8 +56,6 @@ class ScheduledAction final
static ScheduledAction* create(ScriptState*, const String& handler); static ScheduledAction* create(ScriptState*, const String& handler);
~ScheduledAction(); ~ScheduledAction();
void dispose();
DECLARE_TRACE(); DECLARE_TRACE();
void execute(ExecutionContext*); void execute(ExecutionContext*);
......
...@@ -36,13 +36,6 @@ ScriptSourceCode::ScriptSourceCode(ScriptStreamer* streamer, ...@@ -36,13 +36,6 @@ ScriptSourceCode::ScriptSourceCode(ScriptStreamer* streamer,
ScriptSourceCode::~ScriptSourceCode() {} ScriptSourceCode::~ScriptSourceCode() {}
void ScriptSourceCode::dispose() {
m_source = String();
m_resource = nullptr;
m_streamer = nullptr;
m_url = KURL();
}
DEFINE_TRACE(ScriptSourceCode) { DEFINE_TRACE(ScriptSourceCode) {
visitor->trace(m_resource); visitor->trace(m_resource);
visitor->trace(m_streamer); visitor->trace(m_streamer);
......
...@@ -56,7 +56,6 @@ class CORE_EXPORT ScriptSourceCode final { ...@@ -56,7 +56,6 @@ class CORE_EXPORT ScriptSourceCode final {
ScriptSourceCode(ScriptStreamer*, ScriptResource*); ScriptSourceCode(ScriptStreamer*, ScriptResource*);
~ScriptSourceCode(); ~ScriptSourceCode();
void dispose();
DECLARE_TRACE(); DECLARE_TRACE();
bool isEmpty() const { return m_source.isEmpty(); } bool isEmpty() const { return m_source.isEmpty(); }
...@@ -66,13 +65,13 @@ class CORE_EXPORT ScriptSourceCode final { ...@@ -66,13 +65,13 @@ class CORE_EXPORT ScriptSourceCode final {
bool isNull() const { return m_source.isNull(); } bool isNull() const { return m_source.isNull(); }
const String& source() const { return m_source; } const String& source() const { return m_source; }
ScriptResource* resource() const { return m_resource; } ScriptResource* resource() const { return m_resource.get(); }
const KURL& url() const; const KURL& url() const;
int startLine() const { return m_startPosition.m_line.oneBasedInt(); } int startLine() const { return m_startPosition.m_line.oneBasedInt(); }
const TextPosition& startPosition() const { return m_startPosition; } const TextPosition& startPosition() const { return m_startPosition; }
String sourceMapUrl() const; String sourceMapUrl() const;
ScriptStreamer* streamer() const { return m_streamer; } ScriptStreamer* streamer() const { return m_streamer.get(); }
private: private:
void treatNullSourceAsEmpty(); void treatNullSourceAsEmpty();
......
...@@ -107,10 +107,7 @@ DOMTimer::DOMTimer(ExecutionContext* context, ...@@ -107,10 +107,7 @@ DOMTimer::DOMTimer(ExecutionContext* context,
startRepeating(intervalMilliseconds, BLINK_FROM_HERE); startRepeating(intervalMilliseconds, BLINK_FROM_HERE);
} }
DOMTimer::~DOMTimer() { DOMTimer::~DOMTimer() {}
if (m_action)
m_action->dispose();
}
void DOMTimer::stop() { void DOMTimer::stop() {
InspectorInstrumentation::asyncTaskCanceled(getExecutionContext(), this); InspectorInstrumentation::asyncTaskCanceled(getExecutionContext(), this);
...@@ -118,8 +115,6 @@ void DOMTimer::stop() { ...@@ -118,8 +115,6 @@ void DOMTimer::stop() {
// Need to release JS objects potentially protected by ScheduledAction // Need to release JS objects potentially protected by ScheduledAction
// because they can form circular references back to the ExecutionContext // because they can form circular references back to the ExecutionContext
// which will cause a memory leak. // which will cause a memory leak.
if (m_action)
m_action->dispose();
m_action = nullptr; m_action = nullptr;
SuspendableTimer::stop(); SuspendableTimer::stop();
} }
...@@ -177,8 +172,6 @@ void DOMTimer::fired() { ...@@ -177,8 +172,6 @@ void DOMTimer::fired() {
executionContext->timers()->setTimerNestingLevel(0); executionContext->timers()->setTimerNestingLevel(0);
// Eagerly unregister as ExecutionContext observer. // Eagerly unregister as ExecutionContext observer.
clearContext(); clearContext();
// Eagerly clear out |action|'s resources.
action->dispose();
} }
WebTaskRunner* DOMTimer::timerTaskRunner() const { WebTaskRunner* DOMTimer::timerTaskRunner() const {
......
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