Skip to content

Commit

Permalink
Revert "Make Lacros lifetime closer to real use."
Browse files Browse the repository at this point in the history
This reverts commit 65b9357.

Reason for revert: Made linux-chromeos-rel very unhappy

First failure:
https://ci.chromium.org/ui/p/chromium/builders/ci/linux-chromeos-rel/54339/overview

Example stack trace:
BrowserTestBase received signal: Segmentation fault. Backtrace:
#0 0x5602878b1df9 base::debug::CollectStackTrace()
#1 0x560287814a63 base::debug::StackTrace::StackTrace()
#2 0x560287f236f9 content::(anonymous namespace)::DumpStackTraceSignalHandler()
#3 0x7f5dca479040 (/lib/x86_64-linux-gnu/libc-2.27.so+0x3f03f)
#4 0x56028382eb06 std::__1::__tree<>::__erase_unique<>()
#5 0x5602849cda7e crosapi::BrowserManager::StopKeepAlive()
#6 0x5602877e163f std::__1::unique_ptr<>::reset()
#7 0x5602877e0fa2 crosapi::test::AshBrowserTestStarter::~AshBrowserTestStarter()
#8 0x5602877ff1b3 std::__1::unique_ptr<>::reset()
#9 0x560287f22469 content::BrowserTestBase::SetUp()
#10 0x5602877fd219 InProcessBrowserTest::SetUp()
#11 0x560284bb448d testing::Test::Run()
#12 0x560284bb4e28 testing::TestInfo::Run()
#13 0x560284bb55e3 testing::TestSuite::Run()
#14 0x560284bbecd5 testing::internal::UnitTestImpl::RunAllTests()
#15 0x560284bbe8e7 testing::UnitTest::Run()
#16 0x560287919f5a base::TestSuite::Run()
#17 0x5602877c996f BrowserTestSuiteRunnerChromeOS::RunTestSuite()
#18 0x560287f6ba53 content::LaunchTests()
#19 0x5602877cc08e LaunchChromeTests()
#20 0x5602877c98f1 main
#21 0x7f5dca45bbf7 __libc_start_main
#22 0x56028236e7ea _start


Original change's description:
> Make Lacros lifetime closer to real use.
>
> We started to enable Lacros's KeepAlive for AppService.
> Following that, now AshBrowserTestStarter uses it to make it
> closer to the real use cases.
>
> To avoid name conflict, put AshBrowserTestStarter to crosapi::test
> namespace.
>
> BUG=1277898
> TEST=Ran browser_tests.
>
> Change-Id: I007e2cf5dc75cf581b3fdca595c18b2a9fbc8c5c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3386527
> Reviewed-by: Sven Zheng <svenzheng@chromium.org>
> Reviewed-by: David Tseng <dtseng@chromium.org>
> Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#960880}

Bug: 1277898
Change-Id: I5d1b9053bb82d0a5593f8ffaf5a70ff66cd72618
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3401559
Auto-Submit: Leonard Grey <lgrey@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Leonard Grey <lgrey@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/main@{#960983}
  • Loading branch information
speednoisemovement authored and Chromium LUCI CQ committed Jan 19, 2022
1 parent 7b855cb commit 408a5d5
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 31 deletions.
8 changes: 0 additions & 8 deletions chrome/browser/ash/crosapi/browser_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ namespace mojom {
class Crosapi;
} // namespace mojom

namespace test {
class AshBrowserTestStarter;
} // namespace test

class BrowserLoader;
class TestMojoConnectionManager;

Expand Down Expand Up @@ -277,10 +273,6 @@ class BrowserManager : public session_manager::SessionManagerObserver,
// web apps in Lacros. Need to decouple the App Platform systems from
// needing lacros-chrome running all the time.
friend class apps::AppServiceProxyAsh;
// Currently in practice KeepAlive is enabled for AppService as described
// above. To make the testing environment closer to the case, set up
// KeepAlive for AshBrowserTestStarter, too.
friend class test::AshBrowserTestStarter;

// Returns true if the binary is ready to launch or already launched.
bool IsReady() const;
Expand Down
20 changes: 9 additions & 11 deletions chrome/test/base/chromeos/ash_browser_test_starter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "chrome/test/base/in_process_browser_test.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace crosapi {
namespace test {

AshBrowserTestStarter::AshBrowserTestStarter() = default;
Expand All @@ -46,21 +45,21 @@ bool AshBrowserTestStarter::PrepareEnvironmentForLacros() {
return true;
}

class LacrosStartedObserver : public BrowserManagerObserver {
class LacrosStartedObserver : public crosapi::BrowserManagerObserver {
public:
LacrosStartedObserver() = default;
LacrosStartedObserver(const LacrosStartedObserver&) = delete;
LacrosStartedObserver& operator=(const LacrosStartedObserver&) = delete;
~LacrosStartedObserver() override = default;

void OnStateChanged() override {
if (BrowserManager::Get()->IsRunning()) {
if (crosapi::BrowserManager::Get()->IsRunning()) {
run_loop_.Quit();
}
}

void Wait(base::TimeDelta timeout) {
if (BrowserManager::Get()->IsRunning()) {
if (crosapi::BrowserManager::Get()->IsRunning()) {
return;
}
base::ThreadPool::PostDelayedTask(FROM_HERE, run_loop_.QuitClosure(),
Expand Down Expand Up @@ -92,16 +91,16 @@ void AshBrowserTestStarter::StartLacros(InProcessBrowserTest* test_class_obj) {
DCHECK(HasLacrosArgument());

WaitForExoStarted(scoped_temp_dir_xdg_.GetPath());
auto* browser_manager = BrowserManager::Get();
lacros_keep_alive_ =
browser_manager->KeepAlive(BrowserManager::Feature::kTestOnly);

crosapi::BrowserManager::Get()->NewWindow(
/*incongnito=*/false, /*should_trigger_session_restore=*/false);

LacrosStartedObserver observer;
browser_manager->AddObserver(&observer);
crosapi::BrowserManager::Get()->AddObserver(&observer);
observer.Wait(TestTimeouts::action_max_timeout());
browser_manager->RemoveObserver(&observer);
crosapi::BrowserManager::Get()->RemoveObserver(&observer);

CHECK(browser_manager->IsRunning());
CHECK(crosapi::BrowserManager::Get()->IsRunning());

// Create a new ash browser window so browser() can work.
Profile* profile = ProfileManager::GetActiveUserProfile();
Expand All @@ -110,4 +109,3 @@ void AshBrowserTestStarter::StartLacros(InProcessBrowserTest* test_class_obj) {
}

} // namespace test
} // namespace crosapi
4 changes: 0 additions & 4 deletions chrome/test/base/chromeos/ash_browser_test_starter.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@

#include "base/files/scoped_temp_dir.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/ash/crosapi/browser_manager.h"

class InProcessBrowserTest;

namespace crosapi {
namespace test {

class AshBrowserTestStarter {
Expand Down Expand Up @@ -39,10 +37,8 @@ class AshBrowserTestStarter {
// This is XDG_RUNTIME_DIR.
base::ScopedTempDir scoped_temp_dir_xdg_;
base::test::ScopedFeatureList scoped_feature_list_;
std::unique_ptr<BrowserManager::ScopedKeepAlive> lacros_keep_alive_;
};

} // namespace test
} // namespace crosapi

#endif // CHROME_TEST_BASE_CHROMEOS_ASH_BROWSER_TEST_STARTER_H_
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class DemoAshRequiresLacrosTest : public InProcessBrowserTest {
}

protected:
crosapi::test::AshBrowserTestStarter ash_starter_;
test::AshBrowserTestStarter ash_starter_;
};

IN_PROC_BROWSER_TEST_F(DemoAshRequiresLacrosTest, NewTab) {
Expand Down
11 changes: 8 additions & 3 deletions chrome/test/base/javascript_browser_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,18 @@ void JavaScriptBrowserTest::AddLibrary(const base::FilePath& library_path) {
user_libraries_.push_back(library_path);
}

JavaScriptBrowserTest::JavaScriptBrowserTest() = default;
JavaScriptBrowserTest::JavaScriptBrowserTest() {
#if BUILDFLAG(IS_CHROMEOS_ASH)

JavaScriptBrowserTest::~JavaScriptBrowserTest() = default;
#endif
}

JavaScriptBrowserTest::~JavaScriptBrowserTest() {
}

void JavaScriptBrowserTest::SetUpInProcessBrowserTestFixture() {
#if BUILDFLAG(IS_CHROMEOS_ASH)
ash_starter_ = std::make_unique<crosapi::test::AshBrowserTestStarter>();
ash_starter_ = std::make_unique<test::AshBrowserTestStarter>();
if (ash_starter_->HasLacrosArgument())
ASSERT_TRUE(ash_starter_->PrepareEnvironmentForLacros());
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
Expand Down
6 changes: 2 additions & 4 deletions chrome/test/base/javascript_browser_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,7 @@ class JavaScriptBrowserTest : public InProcessBrowserTest {
std::vector<base::Value> args);

#if BUILDFLAG(IS_CHROMEOS_ASH)
crosapi::test::AshBrowserTestStarter* ash_starter() {
return ash_starter_.get();
}
test::AshBrowserTestStarter* ash_starter() { return ash_starter_.get(); }
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

private:
Expand All @@ -62,7 +60,7 @@ class JavaScriptBrowserTest : public InProcessBrowserTest {
std::vector<base::FilePath> library_search_paths_;

#if BUILDFLAG(IS_CHROMEOS_ASH)
std::unique_ptr<crosapi::test::AshBrowserTestStarter> ash_starter_;
std::unique_ptr<test::AshBrowserTestStarter> ash_starter_;
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
};

Expand Down

0 comments on commit 408a5d5

Please sign in to comment.