Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(52)

Issue 2552673002: Eagerly dispose of ScheduledActions (reland.) (Closed)

Created:
4 years ago by sof
Modified:
4 years ago
Reviewers:
oilpan-reviews, haraken
CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Eagerly dispose of ScheduledActions (reland.) 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 Committed: https://crrev.com/4db292779177d5c3b666b2cb735a0f27b055c087 Cr-Original-Commit-Position: refs/heads/master@{#436298} Cr-Commit-Position: refs/heads/master@{#438503}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Add assert #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -4 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h View 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp View 1 1 chunk +11 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.h View 2 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/DOMTimer.cpp View 3 chunks +8 lines, -1 line 0 comments Download

Messages

Total messages: 34 (22 generated)
sof
please take a look. Seeing a number of crashes (across Opera releases) during ~ScheduledAction; theory ...
4 years ago (2016-12-05 10:25:14 UTC) #4
haraken
LGTM https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp#newcode64 third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:64: ScheduledAction::~ScheduledAction() {} Ditto. https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp#newcode37 ...
4 years ago (2016-12-05 10:53:28 UTC) #6
sof
https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp#newcode37 third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:37: ScriptSourceCode::~ScriptSourceCode() {} On 2016/12/05 10:53:27, haraken wrote: > > ...
4 years ago (2016-12-05 10:54:51 UTC) #7
sof
https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp#newcode64 third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:64: ScheduledAction::~ScheduledAction() {} On 2016/12/05 10:53:27, haraken wrote: > > ...
4 years ago (2016-12-05 14:07:09 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2552673002/20001
4 years ago (2016-12-05 15:45:58 UTC) #18
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years ago (2016-12-05 15:51:03 UTC) #21
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/11bd50343795ed1dc1977da91e9a1588687522fd Cr-Commit-Position: refs/heads/master@{#436298}
4 years ago (2016-12-05 15:53:25 UTC) #23
sof
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2568103002/ by sigbjornf@opera.com. ...
4 years ago (2016-12-12 14:20:44 UTC) #24
sof
Indications are that this CL wasn't implicated, https://bugs.chromium.org/p/chromium/issues/detail?id=673783#c5 , so I'd like to ask permission ...
4 years ago (2016-12-14 10:52:54 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2552673002/20001
4 years ago (2016-12-14 12:22:36 UTC) #29
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years ago (2016-12-14 13:51:39 UTC) #32
commit-bot: I haz the power
4 years ago (2016-12-14 13:53:47 UTC) #34
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/4db292779177d5c3b666b2cb735a0f27b055c087
Cr-Commit-Position: refs/heads/master@{#438503}

Powered by Google App Engine
This is Rietveld 408576698