New issue
Advanced search Search tips

Issue 850541 link

Starred by 2 users
Status: Fixed
Owner:
lukasza@chromium.org
Closed: Aug 31
Cc:
thestig@chromium.org
alex...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Tests calling SetMaxRendererProcessCount should reset to the default value

Project Member Reported by lukasza@chromium.org, Jun 7 2018 
Tests calling SetMaxRendererProcessCount should reset the process limit to the default value after the test finishes running.  This will ensure that unrelated tests run with the default value (rather than a value depending on which tests have run earlier).

Tests that call SetMaxRendererProcessCount(...):
    $ git grep -l SetMaxRendererProcessCount | grep -i test
    chrome/browser/chrome_navigation_browsertest.cc
    chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
    chrome/browser/extensions/process_management_browsertest.cc
    chrome/browser/extensions/process_manager_browsertest.cc
    chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
    chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
    chrome/browser/resource_coordinator/tab_manager_browsertest.cc
    chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc
    content/browser/frame_host/render_frame_host_manager_browsertest.cc
    content/browser/isolated_origin_browsertest.cc
    content/browser/renderer_host/render_process_host_browsertest.cc
    content/browser/renderer_host/render_process_host_unittest.cc
    content/browser/site_instance_impl_unittest.cc
    content/browser/site_per_process_browsertest.cc

Tests that call SetMaxRendererProcessCount(0) to reset to the default value:
    $ git grep -l 'SetMaxRendererProcessCount(0)' | grep -i test
    content/browser/renderer_host/render_process_host_browsertest.cc
    content/browser/renderer_host/render_process_host_unittest.cc

Many thanks to thestig@ for spotting this and starting to fix it in https://crrev.com/c/1088316

Comment 1 by thestig@chromium.org, Jun 7 2018 

The browser tests may be ok if individual test cases don't share the environment. I would double check though.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 8 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2fec94222d7543b8653d59542c26486f569dea93

commit 2fec94222d7543b8653d59542c26486f569dea93
Author: Lei Zhang <thestig@chromium.org>
Date: Fri Jun 08 05:19:49 2018

Make SpareRenderProcessHostUnitTest cleanup after itself.

As is, it calls RenderProcessHost::SetMaxRendererProcessCount(), which
overrides a global variable. Since it does not cleanup after itself,
any other subsequent test cases are stuck with its override.
RenderProcessHostUnitTest, which is sensitive to the override, has to
call SetMaxRendererProcessCount(0) to compensate.

BUG= 850541 

Change-Id: I0351d2bd709e2ba850ee47b5ed9a503903b3f5f8
Reviewed-on: https://chromium-review.googlesource.com/1088316
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565554}
[modify] https://crrev.com/2fec94222d7543b8653d59542c26486f569dea93/content/browser/renderer_host/render_process_host_unittest.cc

Comment 3 by lukasza@chromium.org, Aug 31

Status: Fixed (was: Assigned)
AFAIR there is nothing more to be done here.
►

Sign in to add a comment