New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 906809 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

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

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Nov 19

Issue description

Filed 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


 
Owner: lukasza@chromium.org
Hello Lukasz, 

https://chromium-review.googlesource.com/c/chromium/src/+/1318750 seems to be culprit for webkit layout test failures. Could you please take a look? 

Thanks!
Status: Started (was: Available)
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.
Yeah - it seems that this is what people have done in the past - e.g. here: https://crrev.com/c/967511/
Project Member

Comment 5 by bugdroid1@chromium.org, 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

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).
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





Cc: alex...@chromium.org dcheng@chromium.org
Components: Internals>Sandbox>SiteIsolation
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()

Apparently the leaked RemoteFrame is kept alive by FocusController::focused_frame_
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.
 Issue 906810  has been merged into this issue.
WIP CL with a real fix: https://crrev.com/c/1344254
Project Member

Comment 16 by bugdroid1@chromium.org, 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

Labels: Type-Bug
Status: Fixed (was: Started)
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