Issue metadata
Sign in to add a comment
|
wpt test module/crossorigin.html is leaking |
||||||||||||||||||||||
Issue description
third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/crossorigin.html fails with a memory leak when run with leak-detection.
Leak log:
({"numberOfLiveDocuments":[1,2],"numberOfLiveFrames":[1,2],"numberOfLiveNodes":[4,22],"numberOfLiveResources":[0,1],"numberOfLiveSuspendableObjects":[2,3]})
Builders failed on:
- WebKit Linux Trusty Leak:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak
,
May 3 2017
,
May 3 2017
Thanks for handling! My CL enables the feature (ES6 module scripts, behind a flag) that the layout test is testing. Assigning to me for initial investigation.
,
May 3 2017
,
May 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cce51892785f3fcb3ec019197517c2c99bf38cc4 commit cce51892785f3fcb3ec019197517c2c99bf38cc4 Author: adithyas <adithyas@chromium.org> Date: Wed May 03 21:21:53 2017 Add leak expectation for module WPT This leak is caused by http://crrev.com/2781713003. I tried reverting manually, but it is a non-trivial revert, so I'm adding in a leak expectation for now. TBR=hajimehoshi, hiroshige BUG= 718114 Review-Url: https://codereview.chromium.org/2857253002 Cr-Commit-Position: refs/heads/master@{#469130} [modify] https://crrev.com/cce51892785f3fcb3ec019197517c2c99bf38cc4/third_party/WebKit/LayoutTests/LeakExpectations
,
May 4 2017
,
May 10 2017
,
May 10 2017
,
May 17 2017
https://codereview.chromium.org/2860993002 was also reverted due to leaks.
,
May 17 2017
Hmm. A flaky but reproducible case:
<!doctype html>
<html>
<head>
<title>html-script-module-crossOrigin</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
</head>
<body>
<script type="module">export var foo = {};</script>
<script>
var t = async_test("");
window.onload = function() {
setTimeout(function(){t.done();}, 100);
};
</script>
</body>
</html>
"export var foo" and "setTimeout" makes this to fail more frequently, but still leaks without those.
,
May 17 2017
Looks like the failure and flakiness is because of ScriptedIdleTaskController? Sometimes ScriptedIdleTaskController is created before leak detector counts the number of suspendable objects (== fail, because the leak detector thinks the newly created ScriptedIdleTaskController is leaking), or after the leak detector counts the number (== success).
,
May 17 2017
The same leak is observed even with the following HTML (with low probability, once per 100 runs?):
<!doctype html>
<script>
testRunner.waitUntilDone();
testRunner.dumpAsText();
window.onload = function() {
setTimeout(function(){testRunner.notifyDone();}, 100);
};
</script>
Related to Issue 595155 ?
,
May 17 2017
#11: ScriptedIdleTaskController shouldn't be created without calling requestIdleCallback(). It's only created here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?rcl=233e1b3593cd4a9565e2ffef1a9ec97207d740e2&l=6204 Did I misunderstand something?
,
May 17 2017
Document::RequestIdleCallback() is called from IdleSpellCheckCallback::ColdModeTimerFired() in this case.
,
May 17 2017
I see. The timeline is: 1. Document loaded 2. IdleSpellCheckCallback registered a timer 3. testRunner.nodifyDone() called 4. IdleSpellCheckCallback's timer fired, which calls requestIdleCallback, which creats ScriptedIdleTaskController 5. Leak detector counts number of suspendable objects and reports leak We should call IdleSpellCheckCallback::Deactivate() in SpellChecker::PrepareForLeakDetection()
,
May 17 2017
Thanks for suggestion! I'll try that and if it works upload a CL.
,
May 17 2017
Thanks for fixing my implementation! Btw, this is kind of a first-aid but doesn't fix the root cause. Ideally, registering an idle task after testRunner.notifyDone() shouldn't cause leak, just like registering a timeout. But it happens. I guess this issue got overlooked because IdleSpellCheckCallback is the only class that registers idle tasks internally? Anyway, I don't prioritize fixing it before there is a second internal user of idle tasks.
,
May 20 2017
Another related wpt test was just imported which has a similar leak: Example test results: https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Trusty_Leak/4841/layout-test-results/results.html Test: external/wpt/html/semantics/scripting-1/the-script-element/module/import-subgraph-404.html Leak log: ({"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,21],"numberOfLiveResources":[0,2],"numberOfLiveSuspendableObjects":[2,3]}) See bug 724818 ; that could be duped into this one if it's related.
,
May 22 2017
,
May 22 2017
Still leaking after applying the suggestion (Comment #15). It suppresses new tasks in or after Step 5, but ScriptedIdleTaskController is still created (because it is before PrepareForLeakDetection). Perhaps kouhei@ is taking another look?
,
May 22 2017
claiming OWNER while taking a look
,
May 26 2017
As for leak without module scripts (Comment #12) is quite rare, taking several hours to check.
- The leak occurs about once per 2000 runs without [1].
- I'm checking whether it occurs with [1] and will report the result tomorrow.
[1] Call Deactivate(), according to the suggestion in Comment #15.
https://codereview.chromium.org/2904993002/
,
May 26 2017
No leak for ~18000 runs after [1], so [1] seems to fix the leak without module scripts. Command line (leak.html is the HTML in Comment #12): ASAN_OPTIONS="detect_leaks=1 symbolize=1 external_symbolizer_path=./third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer" ./third_party/WebKit/Tools/Scripts/run-webkit-tests --no-retry-failures --enable-leak-detection --iterations=1000000 --batch-size=1 --exit-after-n-failures=1 third_party/WebKit/LayoutTests/fast/dom/leak.html Note: leak with module scripts is still a valid leak bug.
,
May 26 2017
,
May 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b01675740f7b8841d4b59c4a35b517bb19dd401 commit 7b01675740f7b8841d4b59c4a35b517bb19dd401 Author: hiroshige <hiroshige@chromium.org> Date: Mon May 29 07:01:33 2017 Call IdleSpellCheckCallback::Deactivate() in PrepareForLeakDetection() IdleSpellCheckCallback's cold mode timer can be fired and thus ScriptedIdleTaskController can be created after PrepareForLeakDetection() before leak detection, causing ScriptedIdleTaskController to be reported as leaking. This CL fixes this by calling Deactivate(). Tests for this issue is not included because this is quite timing-dependent and is hard to reproduce reliably. BUG= 718114 Review-Url: https://codereview.chromium.org/2904993002 Cr-Commit-Position: refs/heads/master@{#475302} [modify] https://crrev.com/7b01675740f7b8841d4b59c4a35b517bb19dd401/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
,
May 30 2017
,
May 30 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by adithyas@chromium.org
, May 3 2017