New issue
Advanced search Search tips

Issue 751242 link

Starred by 1 user
Status: Untriaged
Owner: ----
Cc:
rdevlin....@chromium.org
r...@chromium.org
thestig@chromium.org
sky@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

BrowserProcessImplTest creates a second BrowserProcessImpl in-process

Project Member Reported by michae...@chromium.org, Aug 1 2017 
BrowserProcessImplTest[1] creates a second BrowserProcessImpl, in addition to the one provided by the unit_tests framework(?).

This is problematic because it leads to global state being set twice. The test juggles |g_browser_process| to "restore" the original BrowserProcessImpl after the test runs, but there's more to the global state than just that variable.

BrowserProcessImpl's constructor calls static methods like:

extensions::ExtensionsClient::Set(ExtensionsClient* client)
extensions::AppWindowClient::Set(AppWindowClient* client)

which expect to be called once per process, so they have special-case logic for unit tests, e.g. [2].

Not all unit tests have a BrowserProcess, so sometimes we want to set this global state on a test-by-test basis, e.g. to test AppWindowClient behavior. That complicates the special-case logic: we'd need to allow tests to Set(nullptr) so that future tests can Set() again to a new pointer.

Can we test BrowserProcessImpl a different way? Creating a second BrowserProcessImpl in-process doesn't seem like a realistic test case.

[1] https://cs.chromium.org/chromium/src/chrome/browser/browser_process_impl_unittest.cc?sq=package:chromium&dr&l=27&q=BrowserProcessImplTest.LifeCycle
[2] https://cs.chromium.org/chromium/src/extensions/common/extensions_client.cc?q=extensions_client%5C.cc&sq=package:chromium&dr&l=36
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 3 2017 

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

commit 43228dc6cc078928643ee04f28d81d81609a93be
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Thu Aug 03 04:29:13 2017

Allow changing AppWindowClient for tests

AppWindowClient::Set sets the global AppWindowClient. Unit tests
sometimes call it more than once, e.g. when multiple BrowserProcesses
are created. Currently, each additional call silently returns instead of
changing g_client. Even Set(nullptr) does nothing.

Normally, this is fine: only tests should call it more than once, and
usually it's called with the same value anyway (because BrowserProcess
and TestingBrowserProcess both use the global ChromeAppWindowClient
singleton).

Some tests do create and destroy unique AppWindowClients, like ones in
app_shell_unittest. This CL allows g_client to be changed to nullptr and
then to a new value. Otherwise, since these tests run sequentially in the
same process, g_client may point to freed memory. This will become an
issue as we add more tests here.

Bug: 751242
Change-Id: I1bfa8f12f1fbbd5e086ae3c033e038b93c5a18ce
Reviewed-on: https://chromium-review.googlesource.com/593052
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491645}
[modify] https://crrev.com/43228dc6cc078928643ee04f28d81d81609a93be/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/43228dc6cc078928643ee04f28d81d81609a93be/chrome/test/base/testing_browser_process.cc
[modify] https://crrev.com/43228dc6cc078928643ee04f28d81d81609a93be/extensions/browser/app_window/app_window_client.cc
[modify] https://crrev.com/43228dc6cc078928643ee04f28d81d81609a93be/extensions/shell/browser/shell_native_app_window_aura_unittest.cc
►

Sign in to add a comment