Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552673002:
Eagerly dispose of ScheduledActions (reland.) (Closed)
DescriptionEagerly 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 #
Messages
Total messages: 34 (22 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. Seeing a number of crashes (across Opera releases) during ~ScheduledAction; theory is that it is due to too late release of v8 resources, but speculation. Reclaiming string source storage earlier is helpful, regardless.
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:64: ScheduledAction::~ScheduledAction() {} Ditto. https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:37: ScriptSourceCode::~ScriptSourceCode() {} Add a dcheck to check that dispose has been called?
https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptSourceCode.cpp:37: ScriptSourceCode::~ScriptSourceCode() {} On 2016/12/05 10:53:27, haraken wrote: > > Add a dcheck to check that dispose has been called? No, that wouldn't be appropriate -- ScriptSourceCode is mostly stack allocated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Eagerly dispose of ScheduleActions. 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= ========== to ========== 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= ==========
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/2552673002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:64: ScheduledAction::~ScheduledAction() {} On 2016/12/05 10:53:27, haraken wrote: > > Ditto. Added.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2552673002/#ps20001 (title: "Add assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480952740651830,
"parent_rev": "fbc1478c016bf13c0ea0496bea28d213f7dd2c2a", "commit_rev":
"796ae3ae9251d15ab3ea342c7e2aad70c9221784"}
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/11bd50343795ed1dc1977da91e9a1588687522fd Cr-Commit-Position: refs/heads/master@{#436298}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2568103002/ by sigbjornf@opera.com. The reason for reverting is: Speculative revert for reported perf decrease on system_health.memory_mobile, https://crbug.com/672098.
Message was sent while issue was closed.
Description was changed from ========== 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} ========== to ========== 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} ==========
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 to reland & retry. The revert is itself causing an (unexpected) perf regression; see same issue (673783)
Description was changed from ========== 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} ========== to ========== 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 Cr-Commit-Position: refs/heads/master@{#436298} ==========
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481718143671650,
"parent_rev": "081915f44caa2484c938b1cffe76a224f4090e5e", "commit_rev":
"beaaf29500fd2968200a6878d777ef05eb6dc824"}
Message was sent while issue was closed.
Description was changed from ========== 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 Cr-Commit-Position: refs/heads/master@{#436298} ========== to ========== 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 Cr-Commit-Position: refs/heads/master@{#436298} Review-Url: https://codereview.chromium.org/2552673002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== 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 Cr-Commit-Position: refs/heads/master@{#436298} Review-Url: https://codereview.chromium.org/2552673002 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4db292779177d5c3b666b2cb735a0f27b055c087 Cr-Commit-Position: refs/heads/master@{#438503} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
