ScriptedIdleTaskController outlives its document |
|||||||||
Issue description
ScriptedIdleTaskController::registerCallback creates a ref-counted IdleRequestCallbackWrapper object, which it passes to WTF::bind and passes to the WebScheduler:
RefPtr<internal::IdleRequestCallbackWrapper> callbackWrapper = internal::IdleRequestCallbackWrapper::create(id, this);
m_scheduler->postIdleTask(BLINK_FROM_HERE, WTF::bind<double>(&internal::IdleRequestCallbackWrapper::idleTaskFired, callbackWrapper));
The IdleRequestCallbackWrapper has a Persistent pointer to the ScriptedIdleTaskController:
class IdleRequestCallbackWrapper {
...
RefPtrWillBePersistent<ScriptedIdleTaskController> m_controller;
}
Since WebScheduler tasks are not cancelled when a Document goes away, and the bound callback holds a reference to the IdleRequestCallbackWrapper and keeps it alive, the ScriptedTaskController will stay alive until the last idle callback runs.
This manifests as a DOM leak when running script in the blink test runner. For example:
<!DOCTYPE html>
<script>
if (window.testRunner)
testRunner.waitUntilDone();
requestIdleCallback(() => {
if (window.testRunner)
testRunner.notifyDone();
}, {timeout: 100});
</script>
... run through the test runner:
$ content_shell --run-layout-test --enable-leak-detection test.html
...
#LEAK - renderer pid 9954 ({"numberOfLiveActiveDOMObjects":[2,3]})
Note that {timeout: 100} is needed due to this unrelated bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=595152
---
This is blocking IntersectionObserver, which needs to use IdleRequestCallback.
,
Mar 16 2016
,
Mar 16 2016
,
Mar 16 2016
I see, so the problem is that the timeout callback is keeping the controller alive even after the idle callback has fired and thus the timeout callback would never do anything anyway. I think Elliott's suggested fix looks fine. I'll cook up a CL to fix this.
,
Mar 16 2016
It occurs to me that this may actually be related to: https://bugs.chromium.org/p/chromium/issues/detail?id=595152 ScriptedIdleTaskController::registerCallback schedules two tasks: one to be run at idle time, and another one to be run when the timeout expires. My hypothesis is that the idle task never runs, only the timeout ever runs. That would explain the behavior of that other bug, and it would also better explain this leak: the idle task is stuck in the WebScheduler and never actually fires.
,
Mar 16 2016
Actually, I'm now convinced that hypothesis is correct, because otherwise the patch I suggested would segfault :)
,
Mar 16 2016
I have a fix for this issue in https://codereview.chromium.org/1808643003/. I did wonder why the initial patch you suggested wouldn't segfault (I added code to avoid this in my patch), but that would explain it. I'm wondering if the issue is due to the fact you are running these as layout tests. The layout tests run without a threaded compositor, so they never get any idle periods and idle tasks don't get run. To work around this for rIC I added the rIC layout tests in virtual/threaded/, which explicitly turns on threaded composting. Does it work if you pass --enable-threaded-compositing? Sami: Can we do anything to get the non-threaded compositor configuration triggering idle periods in the scheduler?
,
Mar 16 2016
Not sure about short idle, but we might be able to make long idle periods work in single threaded mode.
,
Mar 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/82f18cd2a7b718c4d5356222017d9855a53eb6f3 commit 82f18cd2a7b718c4d5356222017d9855a53eb6f3 Author: rmcilroy <rmcilroy@chromium.org> Date: Wed Mar 16 17:49:35 2016 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 Review URL: https://codereview.chromium.org/1808643003 Cr-Commit-Position: refs/heads/master@{#381489} [modify] https://crrev.com/82f18cd2a7b718c4d5356222017d9855a53eb6f3/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp
,
Mar 16 2016
This is fixed, let's continue the conversation about threaded compositor layout tests on the other bug.
,
Mar 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/652dd3c14876e7b3618742e829d7b3c5522aaf2e commit 652dd3c14876e7b3618742e829d7b3c5522aaf2e Author: fgorski <fgorski@chromium.org> Date: Wed Mar 16 18:15:02 2016 Revert of RequestIdleCallback: Avoid retaining ScriptedIdleTaskController while a cancelled timeout is pending (patchset #2 id:20001 of https://codereview.chromium.org/1808643003/ ) Reason for revert: Closed tree. Original issue's description: > 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} TBR=esprehn@chromium.org,rmcilroy@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 595155 Review URL: https://codereview.chromium.org/1814443002 Cr-Commit-Position: refs/heads/master@{#381497} [modify] https://crrev.com/652dd3c14876e7b3618742e829d7b3c5522aaf2e/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp
,
Mar 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ac72812026ac388eb41d218a5ddf01747f301384 commit ac72812026ac388eb41d218a5ddf01747f301384 Author: rmcilroy <rmcilroy@chromium.org> Date: Thu Mar 17 15:13:36 2016 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} Review URL: https://codereview.chromium.org/1808643003 Cr-Commit-Position: refs/heads/master@{#381710} [modify] https://crrev.com/ac72812026ac388eb41d218a5ddf01747f301384/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp
,
Apr 26 2016
,
Jan 6 2017
Issue 676289 has been merged into this issue.
,
Jan 6 2017
Reopening this issue because if an idle task is untriggered when the document is gone, it still keeps the ScriptedIdleTaskController alive:
<!doctype html>
<script>
if (window.testRunner)
testRunner.waitUntilDone();
requestIdleCallback(() => {});
if (window.testRunner)
testRunner.notifyDone();
</script>
#LEAK - renderer pid 91979 ({"numberOfLiveSuspendableObjects":[2,3]})
I think IdleRequestCallbackWrapper should store a WeakPtr instead of a Persistent.
,
Jan 6 2017
In review: https://codereview.chromium.org/2618893002
,
Jan 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/114c62b99fd57ac1cf7c0da1150608fd5f68d3bf commit 114c62b99fd57ac1cf7c0da1150608fd5f68d3bf Author: xiaochengh <xiaochengh@chromium.org> Date: Fri Jan 06 13:15:57 2017 Fix memory leak with ScriptedIdleTaskController If there is an untriggered idle task when the document is gone, it introduces memory leak because IdleRequestCallbackWrapper keeps a reference to the ScriptedIdleTaskController, making it live longer than the document. This patch fixes the issue by letting IdleRequestCallbackWrapper store a WeakPersistent instead. BUG= 595155 Review-Url: https://codereview.chromium.org/2618893002 Cr-Commit-Position: refs/heads/master@{#441929} [add] https://crrev.com/114c62b99fd57ac1cf7c0da1150608fd5f68d3bf/third_party/WebKit/LayoutTests/idle-callback/untriggered-do-not-leak.html [modify] https://crrev.com/114c62b99fd57ac1cf7c0da1150608fd5f68d3bf/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp
,
Jan 10 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by szager@chromium.org
, Mar 15 2016Note that this patch, suggested by esprehn, appears to fix the problem: iff --git a/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp b/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp index b0bdb09..af93c90 100644 --- a/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp +++ b/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.cpp @@ -35,11 +35,13 @@ public: { // TODO(rmcilroy): Implement clamping of deadline in some form. callbackWrapper->controller()->callbackFired(callbackWrapper->id(), deadlineSeconds, IdleDeadline::CallbackType::CalledWhenIdle); + callbackWrapper->m_controller = nullptr; } static void timeoutFired(PassRefPtr<IdleRequestCallbackWrapper> callbackWrapper) { callbackWrapper->controller()->callbackFired(callbackWrapper->id(), monotonicallyIncreasingTime(), IdleDeadline::CallbackType::CalledByTimeout); + callbackWrapper->m_controller = nullptr; } ScriptedIdleTaskController::CallbackId id() const { return m_id; }