New issue
Advanced search Search tips

Issue 891476 link

Starred by 1 user

Issue metadata

Status: Available
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Potential jank caused by BrowserPolicyConnector

Project Member Reported by ssid@chromium.org, Oct 2

Issue description

We collected slow reports from users facing janks and found that the function mentioned above is taking too much time on the main thread, potentially causing janks at startup.
The issue can be found in reports:
84d831b7f1f69cfa
7557f2da18e748b6
4235fd6bf348196a
6e608ee54638435b
d928a79ace8f7a2f
3c31f13114f9f68b
9c11a38ac9cfc67c
59a0097389e81c81
0b0320f2bd8ab4df

Go to crash/ReportID to view the traces.

The stack trace that was hit the most:
Unknown
icu_62::UnicodeSet::add(int const*, int, signed char)
icu_62::UnicodeSet::add(int, int)
icu_62::UnicodeSet::applyFilter(signed char (*)(int, void*), void*, int, UErrorCode&)
icu_62::UnicodeSet::applyIntPropertyValue(UProperty, int, UErrorCode&)
icu_62::UnicodeSet::applyPropertyAlias(icu_62::UnicodeString const&, icu_62::UnicodeString const&, UErrorCode&)
icu_62::UnicodeSet::applyPropertyPattern(icu_62::UnicodeString const&, icu_62::ParsePosition&, UErrorCode&)
icu_62::UnicodeSet::applyPropertyPattern(icu_62::RuleCharacterIterator&, icu_62::UnicodeString&, UErrorCode&)
icu_62::UnicodeSet::applyPattern(icu_62::RuleCharacterIterator&, icu_62::SymbolTable const*, icu_62::UnicodeString&, unsigned int, icu_62::UnicodeSet& (icu_62::UnicodeSet::*)(int), int, UErrorCode&)
icu_62::UnicodeSet::applyPatternIgnoreSpace(icu_62::UnicodeString const&, icu_62::ParsePosition&, icu_62::SymbolTable const*, UErrorCode&)
icu_62::UnicodeSet::applyPattern(icu_62::UnicodeString const&, UErrorCode&)
icu_62::UnicodeSet::UnicodeSet(icu_62::UnicodeString const&, UErrorCode&)
icu_62::RegexStaticSets::RegexStaticSets(UErrorCode*)
icu_62::initStaticSets(UErrorCode&)
icu_62::umtx_initOnce(icu_62::UInitOnce&, void (*)(UErrorCode&), UErrorCode&)
icu_62::RegexCompile::RegexCompile(icu_62::RegexPattern*, UErrorCode&)
icu_62::RegexPattern::compile(icu_62::UnicodeString const&, unsigned int, UParseError&, UErrorCode&)
icu_62::RegexMatcher::RegexMatcher(icu_62::UnicodeString const&, unsigned int, UErrorCode&)
policy::BrowserPolicyConnector::IsNonEnterpriseUser(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&)
policy::UserPolicySigninServiceBase::ShouldLoadPolicyForUser(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&)
policy::UserPolicySigninServiceBase::InitializeForSignedInUser(AccountId const&, scoped_refptr<network::SharedURLLoaderFactory>)
policy::UserPolicySigninServiceBase::InitializeOnProfileReady(Profile*)
content::NotificationServiceImpl::Notify(int, content::NotificationSource const&, content::NotificationDetails const&)
ProfileManager::DoFinalInit(Profile*, bool)
ProfileManager::AddProfile(Profile*)
ProfileManager::CreateAndInitializeProfile(base::FilePath const&)
ProfileManager::GetProfile(base::FilePath const&)
ProfileManager::CreateInitialProfile()
ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
ChromeBrowserMainParts::PreMainMessageLoopRun()
content::BrowserMainLoop::PreMainMessageLoopRun()
int base::internal::Invoker<base::internal::BindState<int (content::BrowserMainLoop::*)(), base::internal::UnretainedWrapper<content::BrowserMainLoop> >, int ()>::RunImpl<int (content::BrowserMainLoop::* const&)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<content::BrowserMainLoop> > const&, 0u>(int (content::BrowserMainLoop::* const&&&)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<content::BrowserMainLoop> > const&&&, std::__ndk1::integer_sequence<unsigned int, 0u>)
content::StartupTaskRunner::WrappedTask()
void base::internal::Invoker<base::internal::BindState<void (net::DnsConfigService::*)(), base::internal::UnretainedWrapper<net::DnsConfigService> >, void ()>::RunImpl<void (net::DnsConfigService::*)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<net::DnsConfigService> >, 0u>(void (net::DnsConfigService::*&&)(), std::__ndk1::tuple<base::internal::UnretainedWrapper<net::DnsConfigService> >&&, std::__ndk1::integer_sequence<unsigned int, 0u>)
base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
base::MessageLoop::RunTask(base::PendingTask*)
base::MessageLoop::DoWork()
base::MessagePumpForUI::OnNonDelayedLooperCallback()
base::(anonymous namespace)::NonDelayedLooperCallback(int, int, void*)
 
Components: Enterprise
Cc: atwilson@chromium.org emaxx@chromium.org
Labels: Enterprise-Triaged
Unless something really bad happens in this pattern matching code, I would presume the problem here is in the slowness of the regexps?

For the very least, we may try merging our ~20 regexps into a single one, so that we avoid multiple regexp compilations and matchings. This approach isn't compatible with the "DomainWhitelistRegexFailure" UMA that we report, but maybe we may sacrifice this UMA, since it's reporting about internal issues in the code?

I'm also wondering whether we may throw away regexps at all here and do some simple ad-hoc string comparison...
It looks like BrowserPolicyConnector::IsNonEnterpriseUser() is doing regex matches for about 15 patterns in kNonManagedDomainPatterns with a given username. Can you help me understand why this might consume time? Is there some disk access involved while calling icu match?
Cc: ultrotter@chromium.org
Labels: Hotlist-GoodFirstBug
Owner: ultrotter@chromium.org
So, do we know how much time is involved in these janks? I don't know why they would particularly take much time, but in general the ICU code is a black box to me.

Also, the vast majority of those regexes are just straight string matches, so we could probably optimize them all away into string compares. Guido, maybe a reasonable noogler task?
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 15

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

commit a549c5a8c0b75af885de174fe7147b25108cf2a9
Author: Siddhartha S <ssid@chromium.org>
Date: Mon Oct 15 20:01:01 2018

Add trace event in BrowserPolicyConnector

Helps to measure why BrowserPolicyConnector::IsNonEnterpriseUser()
sometimes takes too long to run on main thread.

BUG=891476

Change-Id: I5dd76dfb22a3334dc1015cf6d6c1f23c7e6f2da0
Reviewed-on: https://chromium-review.googlesource.com/c/1280550
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599726}
[modify] https://crrev.com/a549c5a8c0b75af885de174fe7147b25108cf2a9/components/policy/core/browser/browser_policy_connector.cc

Cc: etienneb@chromium.org
Status: Available (was: Untriaged)

Sign in to add a comment