Is DeviceLocalAccountExternalPolicyLoader being an ExternalLoader correct? |
||
Issue descriptionI stumbled upon this code while making some ExternalLoader changes. ExternalLoaders are supposed to be used with ExternalProviderImpls, which will set the provider as the owner_ of the loader through ExternalProviderImpl::Init(), however DeviceLocalAccountExternalPolicyLoader seem to use it without an ExternalProviderImpl: So then the question is why does it need to be an ExternalLoader? Two virtual methods from ExternalLoaders are interesting to look at in DeviceLocalAccountExternalPolicyLoader: 1. ExternalLoader::StartLoading This is called when an ExternalProvider is visited though ExternalProviderImpl::VisitRegisteredExtension. With the lack of a provider, this will never get called. 2. ExternalLoader::LoadFinished This is called by the loader when it has discovered new extensions and has set them in |ExtensionLoader::prefs_|. DeviceLocalAccountExternalPolicyLoader::OnExtensionListsUpdated() calls LoadFinished, but again without owner_, it won't do anything: https://cs.chromium.org/chromium/src/chrome/browser/extensions/external_loader.cc?rcl=fbf276f018ea6ad8efbade99908eefd34aa1f73d&l=42 When I run DeviceLocalAccountTest.* browser tests, I always see ExternalLoader::owner_ == nullptr. Highly likely I'm missing something though, Toni, can you answer?
,
Aug 31 2017
Thanks, seems we are good then. I was only looking for LoadFinished with owner_ = nullptr cases, so I got tricked. e.g. when I run DeviceLocalAccountTest.FullscreenAllowed, I see four calls to LoadFinished with owner_ = nullptr, #1-#3 during start, then the owner_ is set (so we are good), and again #4 during teardown. So this is WontFix. For future reference, I'm leaving the backtraces for the cases where owner_ == nullptr happens. 1) #1 0x000002dd76ac extensions::ExternalLoader::LoadFinished() #2 0x000002dd7a63 extensions::ExternalPolicyLoader::StartLoading() #3 0x000002dbc249 ExtensionService::CheckForExternalUpdates() 2) #1 0x000002dd76ac extensions::ExternalLoader::LoadFinished() #2 0x000001cc1a25 chromeos::DeviceLocalAccountExternalPolicyLoader::OnExtensionListsUpdated() #3 0x000001cc3eb7 chromeos::ExternalCache::UpdateExtensionsList() #4 0x000001cc16d8 chromeos::DeviceLocalAccountExternalPolicyLoader::UpdateExtensionListFromStore() #5 0x000001cc188d chromeos::DeviceLocalAccountExternalPolicyLoader::OnStoreLoaded() #6 0x7f1139c70fd4 policy::CloudPolicyStore::NotifyStoreLoaded() 3) #0 0x7f113f4fd68c base::debug::StackTrace::StackTrace() #1 0x000002dd76ac extensions::ExternalLoader::LoadFinished() #2 0x000001cc1a25 chromeos::DeviceLocalAccountExternalPolicyLoader::OnExtensionListsUpdated() #3 0x000001cc3eb7 chromeos::ExternalCache::UpdateExtensionsList() #4 0x000001cc16d8 chromeos::DeviceLocalAccountExternalPolicyLoader::UpdateExtensionListFromStore() #5 0x000001cc188d chromeos::DeviceLocalAccountExternalPolicyLoader::OnStoreLoaded() 4) #0 0x7f113f4fd68c base::debug::StackTrace::StackTrace() #1 0x000002dd76ac extensions::ExternalLoader::LoadFinished() #2 0x000001cc1a25 chromeos::DeviceLocalAccountExternalPolicyLoader::OnExtensionListsUpdated() #3 0x000001cc1778 chromeos::DeviceLocalAccountExternalPolicyLoader::StopCache() #4 0x000001e2f5ba policy::DeviceLocalAccountPolicyService::DeleteBrokers() #5 0x000001e1b417 policy::BrowserPolicyConnectorChromeOS::Shutdown() #6 0x0000024e059b BrowserProcessImpl::StartTearDown() |
||
►
Sign in to add a comment |
||
Comment 1 by xiy...@chromium.org
, Aug 31 2017