external/wpt/html/browsers/the-window-object/security-window/window-security.https.html and 20 other(s) in webkit_layout_tests failing on chromium.webkit/WebKit Linux Trusty Leak |
|||||
Issue descriptionFiled by sheriff-o-matic@appspot.gserviceaccount.com on behalf of hongjunchoi@google.com external/wpt/html/browsers/the-window-object/security-window/window-security.https.html and 20 other(s) in webkit_layout_tests failing on chromium.webkit/WebKit Linux Trusty Leak Builders failed on: - WebKit Linux Trusty Leak: https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20Leak
,
Nov 19
,
Nov 19
I don't know what WebKit_Linux_Trusty_Leak does. I vaguely recall that layout tests have their own approach for detecting leaks. I am guessing that this approach might not be compatible with OOPIFs. I think I'll just try to fix this forward by adding a [ Leak Pass ] expectation for the affected tests.
,
Nov 19
Yeah - it seems that this is what people have done in the past - e.g. here: https://crrev.com/c/967511/
,
Nov 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03cf59212a842c0c265aac64374a1ed40aca50e2 commit 03cf59212a842c0c265aac64374a1ed40aca50e2 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Mon Nov 19 23:04:47 2018 Leaks expected when running layout tests with site-per-process. These are most likely not recent regressions, but long-standing issues that got recently discovered because after r609379 made site-per-process the default mode for running layout tests. Using No-Try because no code is changed - only the test expectations (and we want to cycle WebKit Linux Trusty Leak bot green ASAP). Bug: 906809 Change-Id: I72c2da48fc0459b49c180843db34837eecfd0891 No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/1343063 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#609486} [modify] https://crrev.com/03cf59212a842c0c265aac64374a1ed40aca50e2/third_party/WebKit/LayoutTests/LeakExpectations
,
Nov 19
The CL above should be included in https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20Leak/26584 and hopefully this build will be green (at which point we can remove this bug from the sheriffing queue).
,
Nov 19
,
Nov 19
Repro steps: 1. Build blink_tests target 2. Run layout tests with --enable-leak-detection flag - for example: $ DISPLAY=:20 third_party/WebKit/Tools/Scripts/run-webkit-tests -t rel --no-retry --enable-leak-detection external/wpt/html/browsers/the-window-object/security-window/window-security.https.html
,
Nov 19
The leaked frame is allocated via the following callstack: #1 0x7f392b80bdb6 blink::Frame::Frame() #2 0x7f392b86f073 blink::RemoteFrame::RemoteFrame() #3 0x7f392b86ef85 blink::RemoteFrame::Create() #4 0x7f392b76cb09 blink::WebRemoteFrameImpl::InitializeCoreFrame() #5 0x7f392b752a81 blink::WebFrame::Swap() #6 0x7f39311a012b content::RenderFrameImpl::OnSwapOut()
,
Nov 20
https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20Leak/26586 is green.
,
Nov 20
Apparently the leaked RemoteFrame is kept alive by FocusController::focused_frame_
,
Nov 20
We clear the focused frame when detaching local frames... but not remote frames. Oops. https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/local_frame.cc?rcl=42fe4045e7b7cc0dc2fad03a4e4ab7f5f8c9ba6b&l=430 We could make it a WeakMember, but we should probably just be actively clearing it on detach.
,
Nov 20
Issue 906810 has been merged into this issue.
,
Nov 20
WIP CL with a real fix: https://crrev.com/c/1344254
,
Nov 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03a0497eb7828b3c0b289533bb0aa710c41e642b commit 03a0497eb7828b3c0b289533bb0aa710c41e642b Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Wed Nov 21 00:42:22 2018 Clear FocusController::focused_frame_ when RemoteFrame is detached. Before this CL, after RemoteFrame is detached, it would be kept alive by FocusController::focused_frame_. Before this CL, LocalFrame::DetachImpl would clear FocusController::focused_frame_ if needed. After this CL this is done from Frame::Detach (and so covers both LocalFrame and RemoteFrame). Before and after this CL, FocusController::focused_frame_ stores either a LocalFrame or RemoteFrame, but FocusController::FocusedFrame only exposes LocalFrame. This CL preserves this approach of handling RemoteFrame internally within FocusController, by introducing FocusController::FrameDetached(Frame* detached_frame) method which can directly compare |detached_frame| with |focused_frame_| (without going through local-vs-remote considerations and/or casts). Bug: 906809 Change-Id: I47f3a725db87f3a887ad96c7a4739b4fc02496ed Reviewed-on: https://chromium-review.googlesource.com/c/1344254 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#609870} [modify] https://crrev.com/03a0497eb7828b3c0b289533bb0aa710c41e642b/third_party/WebKit/LayoutTests/LeakExpectations [modify] https://crrev.com/03a0497eb7828b3c0b289533bb0aa710c41e642b/third_party/blink/renderer/core/frame/frame.cc [modify] https://crrev.com/03a0497eb7828b3c0b289533bb0aa710c41e642b/third_party/blink/renderer/core/frame/local_frame.cc [modify] https://crrev.com/03a0497eb7828b3c0b289533bb0aa710c41e642b/third_party/blink/renderer/core/page/focus_controller.cc [modify] https://crrev.com/03a0497eb7828b3c0b289533bb0aa710c41e642b/third_party/blink/renderer/core/page/focus_controller.h
,
Nov 21
https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Linux%20Trusty%20Leak/ seems mostly green - marking the bug as fixed. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by hongjunchoi@chromium.org
, Nov 19