New issue
Advanced search Search tips

Issue 798797 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

mash_browser_tests failure SystemWebDialogTest.ModalTest

Project Member Reported by jamescook@chromium.org, Jan 3 2018

Issue description

First failure: https://ci.chromium.org/buildbot/chromium.fyi/Mojo%20ChromiumOS/25451

BrowserTestBase received signal: Segmentation fault. Backtrace:
#0 0x000003e17abc base::debug::StackTrace::StackTrace()
#1 0x000004453292 content::(anonymous namespace)::DumpStackTraceSignalHandler()
#2 0x7f77fd94fcb0 <unknown>
#3 0x0000069e54d0 ash::ShellPort::ShellPort()
#4 0x0000069e5639 ash::ShellPort::IsSystemModalWindowOpen()
#5 0x0000015d6069 SystemWebDialogTest_ModalTest_Test::RunTestOnMainThread()
#6 0x000004452fe5 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop()
#7 0x000003f49db6 ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
#8 0x000003f48b4d ChromeBrowserMainParts::PreMainMessageLoopRun()
#9 0x0000018d3818 chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun()
#10 0x000002666141 content::BrowserMainLoop::PreMainMessageLoopRun()
#11 0x000002a9fb15 content::StartupTaskRunner::RunAllTasksNow()
#12 0x00000266463f content::BrowserMainLoop::CreateStartupTasks()
#13 0x0000026691d8 content::BrowserMainRunnerImpl::Initialize()
#14 0x000002662302 content::BrowserMain()
#15 0x000003de7b01 content::ContentMainRunnerImpl::Run()
#16 0x000005eba62b service_manager::Main()
#17 0x000003de64e4 content::ContentMain()
#18 0x000004452c4c content::BrowserTestBase::SetUp()
#19 0x000003f03289 InProcessBrowserTest::SetUp()
#20 0x00000140c28a chromeos::LoginManagerTest::SetUp()
#21 0x000001c431e1 testing::Test::Run()
#22 0x000001c43d10 testing::TestInfo::Run()
#23 0x000001c441f7 testing::TestCase::Run()
#24 0x000001c4a817 testing::internal::UnitTestImpl::RunAllTests()
#25 0x000001c4a467 testing::UnitTest::Run()
#26 0x000003f18e82 base::TestSuite::Run()
#27 0x000003e0ba99 ChromeTestSuiteRunner::RunTestSuite()
#28 0x000003e0ba0d (anonymous namespace)::MusTestLauncherDelegate::RunTestSuite()
#29 0x00000448aa44 content::LaunchTests()
#30 0x000003e0bec5 LaunchChromeTests()
#31 0x000003e0b7b6 RunMashBrowserTests()
#32 0x000003e0b667 main
#33 0x7f77fd93af45 __libc_start_main
#34 0x0000005bf44a _start
[ RUN      ] SystemWebDialogTest.ModalTest

It's due to ash::ShellPort access in the browser test added in https://chromium-review.googlesource.com/c/chromium/src/+/833528

IN_PROC_BROWSER_TEST_F(SystemWebDialogTest, ModalTest) {
  chromeos::SystemWebDialogDelegate* dialog = new MockSystemWebDialog();
  dialog->ShowSystemDialog();
  EXPECT_TRUE(ash::ShellPort::Get()->IsSystemModalWindowOpen());
}

Root cause is that we don't have a DEPS-ban on //ash access in //chrome/browser/ui/webui/chromeos.

I think I should add a test-only mojo api to check if a system modal window is open. That seems like a useful thing.

 
Could we add a test helper that uses the WindowTree.GetWindowTree interface instead? It looks like ShellPort::GetOpenSystemModalWindowContainerId() could possibly be re-implemented using that interface instead?

I agree that this seems like the kind of thing that Chrome tests are going to need, one way or another.


Status: Started (was: Assigned)
I think it's a little premature to add a general purpose give-me-the-whole-window-tree test interface. WindowTree is owned by the UI service. The window IDs returned by WindowTree.GetWindowTree are per-client (that is, different in ash vs. browser processes). They don't contain information about ash window container ids, which is what you'd need for this issue. The test helper would have to reconstruct some sort of tree from the array of IDs (not hard, but annoying). Likewise, where a modal dialog shows up in the ash window container hierarchy is kind of an implementation detail of ash, not necessarily something the browser should care about.

I'm happy to implement something more general if we end up with multiple cases that need the whole tree, but for now I'm going to start with a one-off is-modal-open interface.

Makes sense. Thanks for the explainiation!
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 5 2018

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

commit 7b7c6a5a6f685373454f15048c7da5f0d8e6df00
Author: James Cook <jamescook@chromium.org>
Date: Fri Jan 05 16:26:37 2018

cros: Move IsSystemModalDialogOpen from ShellPort to Shell

The implementation is the same in classic ash, mus and mash. There are
no "port" differences.

This is a precursor CL to fixing SystemWebDialogTest under mash.

Bug:  798797 
Test: ash_unittests
Change-Id: I53d65e793f45ebba768e503b46dcf10ddb5ac41d
Reviewed-on: https://chromium-review.googlesource.com/851071
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527293}
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/shell.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/shell.h
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/shell_port.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/shell_port.h
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/shell_test_api.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/shell_test_api.h
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/system/tray/system_tray_view.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/system/user/user_view.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/wm/ash_focus_rules.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/wm/overview/window_selector_controller.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/wm/system_modal_container_event_filter.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/wm/system_modal_container_layout_manager_unittest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/wm/tablet_mode/tablet_mode_window_manager_unittest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/ash/wm/window_cycle_controller.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/chrome/browser/ui/webui/chromeos/system_web_dialog_browsertest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/components/exo/client_controlled_shell_surface_unittest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/components/exo/pointer_unittest.cc
[modify] https://crrev.com/7b7c6a5a6f685373454f15048c7da5f0d8e6df00/components/exo/touch_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 5 2018

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

commit 00848f0aae21c5b8d2c7e4d907befd2f494a4768
Author: James Cook <jamescook@chromium.org>
Date: Fri Jan 05 20:04:11 2018

cros: Fix SystemWebDialogTest under --mash

This browser test was directly accessing ash::Shell, which is in the
ash process under mash.

Introduce ash::mojom::ShellTestApi and use mojo to query for whether
or not a modal window is open.

Bug:  798797 
Test: browser_tests --mash
Change-Id: I2bec2d7c6611999c9c28b5fb7ef1c361efece497
Reviewed-on: https://chromium-review.googlesource.com/852480
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527363}
[modify] https://crrev.com/00848f0aae21c5b8d2c7e4d907befd2f494a4768/ash/manifest.json
[modify] https://crrev.com/00848f0aae21c5b8d2c7e4d907befd2f494a4768/ash/mojo_test_interface_factory.cc
[modify] https://crrev.com/00848f0aae21c5b8d2c7e4d907befd2f494a4768/ash/public/interfaces/BUILD.gn
[add] https://crrev.com/00848f0aae21c5b8d2c7e4d907befd2f494a4768/ash/public/interfaces/shell_test_api.mojom
[modify] https://crrev.com/00848f0aae21c5b8d2c7e4d907befd2f494a4768/ash/shell_test_api.cc
[modify] https://crrev.com/00848f0aae21c5b8d2c7e4d907befd2f494a4768/ash/shell_test_api.h
[modify] https://crrev.com/00848f0aae21c5b8d2c7e4d907befd2f494a4768/chrome/browser/ui/webui/chromeos/system_web_dialog_browsertest.cc

Status: Fixed (was: Started)
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment