Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1808643003:
RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending (Closed)
|
Created:
4 years, 9 months ago by rmcilroy Modified:
4 years, 9 months ago Reviewers:
esprehn CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending
Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the
idle task has been triggered, but the document has gone away.
BUG=595155
Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3
Cr-Commit-Position: refs/heads/master@{#381489}
Committed: https://crrev.com/ac72812026ac388eb41d218a5ddf01747f301384
Cr-Commit-Position: refs/heads/master@{#381710}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Review comments #Patch Set 3 : Fix non-oilpan #Messages
Total messages: 28 (12 generated)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808643003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808643003/1
rmcilroy@chromium.org changed reviewers: + esprehn@chromium.org
Elliot, could you please take a look. This is basically the patch you suggested.
lgtm https://codereview.chromium.org/1808643003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1808643003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp:37: if (callbackWrapper->controller()) We normally assign to a local in the if to avoid the verbosity and make the scope clear. if (Foo* foo = thing->foo()) foo->...
I'm not sure it really leaks btw, it just lives as long as the timeout task.
Description was changed from ========== [RequestIdleCallback] Avoid leaking ScriptedIdleTaskController. Ensure that ScriptedIdleTaskController does not leak due to a timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 ========== to ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 ==========
Thanks for the review. On 2016/03/16 16:33:14, esprehn wrote: > I'm not sure it really leaks btw, it just lives as long as the timeout task. Yes agreed, I updated the description to make this more clear. https://codereview.chromium.org/1808643003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp (right): https://codereview.chromium.org/1808643003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp:37: if (callbackWrapper->controller()) On 2016/03/16 16:32:18, esprehn wrote: > We normally assign to a local in the if to avoid the verbosity and make the > scope clear. > > if (Foo* foo = thing->foo()) > foo->... Done.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1808643003/#ps20001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808643003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808643003/20001
Message was sent while issue was closed.
Description was changed from ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 ========== to ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 ========== to ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1814443002/ by fgorski@chromium.org. The reason for reverting is: Closed tree..
Message was sent while issue was closed.
On 2016/03/16 18:14:09, fgorski wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/1814443002/ by mailto:fgorski@chromium.org. > > The reason for reverting is: Closed tree.. Huh? The CQ landed this patch, what do you mean "closed tree" ? This shouldn't have been reverted.
Message was sent while issue was closed.
It seems the Oilpan bot isn't in the CQ and was broken: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20non-Oi... ../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp:37:41: error: no viable conversion from 'WTF::PassRefPtr<ScriptedIdleTaskController>' to 'blink::ScriptedIdleTaskController *' if (ScriptedIdleTaskController* controller = callbackWrapper->controller()) ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../third_party/WebKit/Source/wtf/PassRefPtr.h:90:5: note: candidate function operator UnspecifiedBoolType() const { return m_ptr ? &PassRefPtr::m_ptr : 0; } ^ rmcilroy@ I think you need to make this a RefPtrWillBeRawPtr<>, also we really need to ditch the WillBe stuff soon. :)
Message was sent while issue was closed.
It seems the Oilpan bot isn't in the CQ and was broken: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20non-Oi... ../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp:37:41: error: no viable conversion from 'WTF::PassRefPtr<ScriptedIdleTaskController>' to 'blink::ScriptedIdleTaskController *' if (ScriptedIdleTaskController* controller = callbackWrapper->controller()) ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../third_party/WebKit/Source/wtf/PassRefPtr.h:90:5: note: candidate function operator UnspecifiedBoolType() const { return m_ptr ? &PassRefPtr::m_ptr : 0; } ^ rmcilroy@ I think you need to make this a RefPtrWillBeRawPtr<>, also we really need to ditch the WillBe stuff soon. :)
Message was sent while issue was closed.
On 2016/03/16 18:21:27, esprehn wrote: > It seems the Oilpan bot isn't in the CQ and was broken: > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20non-Oi... > > ../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp:37:41: > error: no viable conversion from 'WTF::PassRefPtr<ScriptedIdleTaskController>' > to 'blink::ScriptedIdleTaskController *' > if (ScriptedIdleTaskController* controller = > callbackWrapper->controller()) > ^ > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../third_party/WebKit/Source/wtf/PassRefPtr.h:90:5: note: candidate function > operator UnspecifiedBoolType() const { return m_ptr ? &PassRefPtr::m_ptr : > 0; } > ^ > > rmcilroy@ I think you need to make this a RefPtrWillBeRawPtr<>, also we really > need to ditch the WillBe stuff soon. :) Yup that was the fix, thanks. Relanding with the fix.
Message was sent while issue was closed.
Description was changed from ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489} ========== to ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489} ==========
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1808643003/#ps40001 (title: "Fix non-oilpan")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808643003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808643003/40001
Message was sent while issue was closed.
Description was changed from ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489} ========== to ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489} ========== to ========== RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending Ensure that ScriptedIdleTaskController does not get unnecessarily retained by a cancelled timeout task if the idle task has been triggered, but the document has gone away. BUG=595155 Committed: https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3 Cr-Commit-Position: refs/heads/master@{#381489} Committed: https://crrev.com/ac72812026ac388eb41d218a5ddf01747f301384 Cr-Commit-Position: refs/heads/master@{#381710} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ac72812026ac388eb41d218a5ddf01747f301384 Cr-Commit-Position: refs/heads/master@{#381710} |
