New issue
Advanced search Search tips

Issue 814845 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-03-12
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

PasswordManagerBrowserTestBase.InFrameNavigationDoesNotClearPopupState flaky on chromium.memory/Mac ASan 64 Tests (1)

Project Member Reported by reillyg@chromium.org, Feb 22 2018

Issue description

When running under MSan this test is failing a CHECK in a flaky manner. The actual failure appears to be in the spell-check module and is potentially not directly related to the password manager.

Example run: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/38851


Error:

[94277:7431:0222/101507.307752:FATAL:spellcheck.mojom.cc(758)] Check failed: !connected. SpellCheckHost::RequestTextCheckCallback was destroyed without first either being run or its corresponding binding being closed. It is an error to drop response callbacks which still correspond to an open interface pipe.
0   browser_tests                       0x000000010c2f497c base::debug::StackTrace::StackTrace(unsigned long) + 28
1   browser_tests                       0x000000010c351fa5 logging::LogMessage::~LogMessage() + 645
2   browser_tests                       0x00000001098e593b spellcheck::mojom::SpellCheckHost_RequestTextCheck_ProxyToResponder::OnIsConnectedComplete(bool) + 331
3   browser_tests                       0x00000001101b6248 mojo::(anonymous namespace)::ResponderThunk::IsConnectedAsync(base::OnceCallback<void (bool)>) + 1304
4   browser_tests                       0x00000001098e567c base::internal::BindState<void (spellcheck::mojom::SpellCheckHost_RequestTextCheck_ProxyToResponder::*)(std::__1::vector<SpellCheckResult, std::__1::allocator<SpellCheckResult> > const&), base::internal::PassedWrapper<std::__1::unique_ptr<spellcheck::mojom::SpellCheckHost_RequestTextCheck_ProxyToResponder, std::__1::default_delete<spellcheck::mojom::SpellCheckHost_RequestTextCheck_ProxyToResponder> > > >::Destroy(base::internal::BindStateBase const*) + 428
5   browser_tests                       0x000000010d44b8db base::internal::BindState<base::OnceCallback<void (std::__1::vector<SpellCheckResult, std::__1::allocator<SpellCheckResult> > const&)>, std::__1::vector<SpellCheckResult, std::__1::allocator<SpellCheckResult> > >::Destroy(base::internal::BindStateBase const*) + 187
6   browser_tests                       0x000000010c3879b8 base::internal::IncomingTaskQueue::TriageQueue::Clear() + 792
7   browser_tests                       0x000000010c38ec3c base::MessageLoop::DeletePendingTasks() + 396
8   browser_tests                       0x000000010c38de29 base::MessageLoop::~MessageLoop() + 793
9   browser_tests                       0x0000000104f7a8be base::MessageLoopForUI::~MessageLoopForUI() + 14
10  browser_tests                       0x00000001079bb748 content::BrowserMainLoop::~BrowserMainLoop() + 3080
11  browser_tests                       0x00000001079bba1e content::BrowserMainLoop::~BrowserMainLoop() + 14
12  browser_tests                       0x00000001079cf183 content::BrowserMainRunnerImpl::Shutdown() + 1251
13  browser_tests                       0x00000001079ba18e content::BrowserMain(content::MainFunctionParams const&) + 718
14  browser_tests                       0x000000010c21bc2e content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) + 1310
15  browser_tests                       0x000000010c21d74a content::ContentMainRunnerImpl::Run() + 1354
16  browser_tests                       0x0000000112669e08 service_manager::Main(service_manager::MainParams const&) + 5352
17  browser_tests                       0x000000010c21b630 content::ContentMain(content::ContentMainParams const&) + 368
18  browser_tests                       0x000000010d9e8e48 content::BrowserTestBase::SetUp() + 5992
19  browser_tests                       0x000000010c5ac595 InProcessBrowserTest::SetUp() + 1301
20  browser_tests                       0x00000001052363c0 testing::Test::Run() + 256
21  browser_tests                       0x0000000105238614 testing::TestInfo::Run() + 900
22  browser_tests                       0x0000000105239957 testing::TestCase::Run() + 967
23  browser_tests                       0x0000000105251147 testing::internal::UnitTestImpl::RunAllTests() + 2503
24  browser_tests                       0x00000001052506ca testing::UnitTest::Run() + 298
25  browser_tests                       0x000000010c5fb5f9 base::TestSuite::Run() + 505
26  browser_tests                       0x000000010c2c6dd6 ChromeTestSuiteRunner::RunTestSuite(int, char**) + 198
27  browser_tests                       0x000000010dacc9d9 content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) + 1081
28  browser_tests                       0x000000010c2c7a53 LaunchChromeTests(unsigned long, content::TestLauncherDelegate*, int, char**) + 723
29  browser_tests                       0x000000010c2c6c2d main + 269
30  libdyld.dylib                       0x00007fff89d755fd start + 1
31  ???                                 0x000000000000000b 0x0 + 11
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 22 2018

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

commit 127c4ef0ce7f312279dd12cd0e469448a54f5011
Author: Reilly Grant <reillyg@chromium.org>
Date: Thu Feb 22 19:27:41 2018

Disable flaky password manager test on macOS ASan

PasswordManagerBrowserTestBase.InFrameNavigationDoesNotClearPopupState
is failing flakily on the macOS ASan bots. Disabling the test under this
configuration.

TBR=vasilii@chromium.org

Bug:  814845 
Change-Id: I07311475cff8afb6fb5d34023132b96224c6a5c3
Reviewed-on: https://chromium-review.googlesource.com/931934
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538521}
[modify] https://crrev.com/127c4ef0ce7f312279dd12cd0e469448a54f5011/chrome/browser/password_manager/password_manager_browsertest.cc

Comment 2 by yosin@chromium.org, Feb 23 2018

Components: -Blink>Editing>Spellcheck UI>Browser>Language>Spellcheck
Change Blink>Editing>Spellcheck to UI>Browser>Language>Spellcheck,
since this is connection management on browser side.
Labels: -Pri-3 Pri-1
Owner: xiaoche...@chromium.org
Status: Assigned (was: Untriaged)
Assigning to xiaochengh@ who made mojofication changes recently. Please prioritize high, our browser tests are important for us. 
Seems to be a ptr ownership issue.

There are two relevant ownership chains (a --> b means a owns b):

1. Message pipe for SpellCheckHost --> SpellCheckHost
2. SpellingRequest::completion_barrier_ --> SpellingRequest --> SpellCheckHost::RequestTextCheckCallback

The two chains are independent to each other, which means SpellCheckHost and RequestTextCheckCallback can be destroyed in arbitrary order.

According to the error message, we can destroy an unrun RequestTextCheckCallback before destroying the message pipe. So the fix should be moving SpellingRequest to be owned by SpellCheckHost.
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 7 2018

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

commit 62233ab43ac37f12520f37b2fa09365b0c503d61
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Mar 07 23:01:56 2018

Fix memory management of spellcheck callbacks on Mac

Mojo doesn't allow an unrun callback to be removed before the
message pipe is closed. However, Mac spellchecker manages
RequestTextCheckCallbacks as (indirectly) owned by a completion
barrier in SpellingRequest, which lives independently with the
message pipe. Hence, in some shutdown orders, the callback can
be cleared while the message pipe is still open.

This patch fixes the issue by making SpellCheckHost own
all pending SpellingRequests, and SpellingRequest own
RequestTextCheckCallback.

Note: this patch serves as a quick fix to the regressed test
cases. A follow-up patch will clean up the code.

Bug:  814845 
Change-Id: I22a08cdce2744b50fe23c0ff48c10d5207fa5f8b
Reviewed-on: https://chromium-review.googlesource.com/946848
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541624}
[modify] https://crrev.com/62233ab43ac37f12520f37b2fa09365b0c503d61/chrome/browser/spellchecker/spell_check_host_chrome_impl.cc
[modify] https://crrev.com/62233ab43ac37f12520f37b2fa09365b0c503d61/chrome/browser/spellchecker/spell_check_host_chrome_impl.h
[modify] https://crrev.com/62233ab43ac37f12520f37b2fa09365b0c503d61/chrome/browser/spellchecker/spell_check_host_chrome_impl_mac.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 8 2018

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

commit 2a5d0220ef913d6cc9a6fb3bc654d64160623064
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Thu Mar 08 22:02:55 2018

Reenable flaky password manager test

As fix attempt to the test flakes has been landed in crrev.com/c/946848,
this patch re-enables the test to see if the fix works.

Bug:  814845 
Change-Id: I2a9e0d98a172fc0cac7c0e87cc302acab04dc325
Reviewed-on: https://chromium-review.googlesource.com/942470
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541890}
[modify] https://crrev.com/2a5d0220ef913d6cc9a6fb3bc654d64160623064/chrome/browser/password_manager/password_manager_browsertest.cc

Status: Fixed (was: Assigned)
The NextAction date has arrived: 2018-03-12

Sign in to add a comment