New LayoutTests for inert reveal existing memory leak |
|||||||||||
Issue descriptionThese three tests: fast/dom/inert/inert-node-is-uneditable.html fast/dom/inert/inert-inlines.html fast/dom/inert/inert-label-focus.html added in https://codereview.chromium.org/2088453002/, caused a leak to be detected: https://bugs.chromium.org/p/chromium/issues/detail?id=725815 However, running these tests on a clean build without the other changes in that patch set indicates that they have revealed one or more existing leaks, rather than the new code introducing a leak.
,
May 26 2017
,
May 26 2017
Editing isn't related to this leak.
,
May 26 2017
,
May 26 2017
https://bugs.chromium.org/p/chromium/issues/detail?id=678879 might be related. lanwei@, could you take a look?
,
May 30 2017
,
May 31 2017
We have used pointerActionSequence in layout tests, and we did not see memory leak, but I will take a look.
,
Jul 26 2017
Minimized test case to cause leaks:
<!DOCTYPE html>
<script>
var pointerActions = [{
source: "mouse",
actions: [
{ name: "pointerMove", x: 0, y: 0 },
]
}];
for (var i = 0; i < 3; i++) {
chrome.gpuBenchmarking.pointerActionSequence(pointerActions);
}
</script>
,
Jul 26 2017
I investigated this and found this is probably not a real leak. * chrome.gpuBenchmarking.pointerActionSequence enqueues a gesture and its callback at RenderWidget::QueueSyntheticGesture. * Callbacks are enqueued to RenderWiedget::pending_synthetic_gesture_callbacks_. * Callbacks refers its V8 context, that refers a document. * Callbacks are dequeued when InputMsg_SyntheticGestureCompleted. * The leak detector tries to detect before all the callbacks are consumed. Actually, when GC is delayed by like 1 second, this leak doesn't happen. Thus, I think we should fix the leak detector to wait all the callbacks are consumed.
,
Sep 26 2017
I got a change reverted because I hit this leak. Presubmit enforces the use of pointerActionSequence instead of eventSender for interactive tests which is unfortunate as long as this issue is not fixed.
,
Sep 26 2017
,
Sep 26 2017
rune@: As this leak is false positive, you can add your test to LeakExpectations and go ahead if you need.
,
Sep 26 2017
I looked at these tests and they aren't awaiting for the promise to be resolved which is causing the leak (and actually failing to actually test anything). You should either use an async test. Or await on the promise.
,
Sep 26 2017
Appears you can't await inside a sync test; so really these should be async tests. You may need to daisy chain them as you don't want them all running at once since you are clicking on items.
,
Sep 26 2017
You should be able to use a promise_test instead of a test and return the promise from the method.
,
Sep 26 2017
moving to Blink>DOM since this is an issue with the layouttests
,
Sep 27 2017
,
Sep 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7cf59fe731788560f890ce965036302ddb6369cb commit 7cf59fe731788560f890ce965036302ddb6369cb Author: Rune Lillesveen <rune@opera.com> Date: Wed Sep 27 18:12:41 2017 Properly await promises in inert-tests. I did not find a way to await each forEach or map in inert-inlines, so I fell back on a plain old for-loop. Bug: 726218 , 769347 Change-Id: I196ff128ce95319b7ca297254745aec842ef6cff Reviewed-on: https://chromium-review.googlesource.com/686814 Commit-Queue: Rune Lillesveen <rune@opera.com> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#504733} [modify] https://crrev.com/7cf59fe731788560f890ce965036302ddb6369cb/third_party/WebKit/LayoutTests/LeakExpectations [modify] https://crrev.com/7cf59fe731788560f890ce965036302ddb6369cb/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/7cf59fe731788560f890ce965036302ddb6369cb/third_party/WebKit/LayoutTests/fast/dom/inert/inert-inlines.html [modify] https://crrev.com/7cf59fe731788560f890ce965036302ddb6369cb/third_party/WebKit/LayoutTests/fast/dom/inert/inert-label-focus.html [modify] https://crrev.com/7cf59fe731788560f890ce965036302ddb6369cb/third_party/WebKit/LayoutTests/fast/dom/inert/inert-node-is-uneditable.html
,
Sep 27 2017
,
Sep 28 2017
Awesome, thank you for fixing! |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by kouhei@chromium.org
, May 25 2017Components: Blink>Editing
Owner: hajimehoshi@chromium.org