New issue
Advanced search Search tips

Issue 599297 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Mar 2016
Cc:
jochen@chromium.org
lukasza@chromium.org
sa...@chromium.org
wfh@chromium.org
creis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 477150



Sign in to add a comment

Crashes in content::BlinkTestRunner::TestFinished when Layout Tests run with --site-per-process

Project Member Reported by lukasza@chromium.org, Mar 30 2016 
The linux_site_isolation FYI bot shows 100 crashes: https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/builds/8598/steps/webkit_tests/logs/stdio

I suspect that the code added in https://codereview.chromium.org/1836203002 crashes when Layout Tests run with --site-per-process - the newly added code dereferences render_view()->GetMainRenderFrame() which can be null if the main frame is a remote frame that lives in another renderer process.

Repro (the important bit is --additional-drt-flag=--site-per-process):

$ DISPLAY=:20 ~/src/chromium4/src/third_party/WebKit/Tools/Scripts/run-webkit-tests -t Release -v --additional-drt-flag=--site-per-process --no-retry-failures --additional-drt-flag=--no-sandbox http/tests/security/cross-frame-access-delete.html

Comment 1 by lukasza@chromium.org, Mar 30 2016 

In addition to OOPIF-incompatible usage of GetMainRenderFrame, I am not sure if BlinkTestRunner::TestFinished is the right place for the kind of cleanup intended (because TestFinished is not necessarily called in all the renderers - only in the renderer that finishes the test [i.e. by calling testRunner.notifyDone] and in the renderer that hosts the main frame of the test).

I think BlinkTestRunner::Reset might be a better place for in-between-tests clean-up.  I am not sure what place might be good for leak detection (mentioned in the comment next to the new code in BlinkTestRunner::TestFinished).  Adding jochen@ in case he has any suggestions.

Comment 2 by lukasza@chromium.org, Mar 30 2016 

BTW: When relanding, could you please include linux_site_isolation trybot in the mix?  This should catch any potential regressions in --site-per-process mode.

Comment 3 by lukasza@chromium.org, Mar 31 2016 

I also wonder if the unregistration/cleanup could be done in the layer of responsible for installing the mocks (rather than adding this responsibility to the somewhat unrelated BlinkTestRunner).  One idea would be to do unregister/cleanup on beforeunload or unload event.

OTOH I haven't been able to find where the mocks are installed;  I assume they are installed by javascript (somewhere around mock-battery-monitor.js pulled by LayoutTests/battery-status/page-visibility.html, but to my surprise I haven't been able to find mock-battery-monitor.js under third_party/WebKit/LayoutTests).

Comment 4 by sa...@chromium.org, Mar 31 2016 

Using an unload event handler sounds much nicer and it seems to run before the leak detector. Thanks for the idea.

Comment 5 by lukasza@chromium.org, Mar 31 2016

Status: Fixed (was: Untriaged)
Marking as fixed - the bot is green after relanding the latest patchset from the original CL @ https://codereview.chromium.org/1836203002/

Comment 6 by sshru...@google.com, May 18 2016

Labels: Test-Layout
►

Sign in to add a comment