CHECK triggered when attempting a restore of a currupt policy installed extension. |
|||
Issue description
This happens for me reliably every time Chrome attempts to recover a policy installed extension.
Discovered when trying to trigger the behaviour from crbug/447040.
[11816:10104:0930/141705:FATAL:registry.cc(64)] Check failed: callback_.is_null().
Backtrace:
base::debug::StackTrace::StackTrace [0x00000000007F2841+33] (c:\src\chromium\src\base\debug\stack_trace_win.cc:214)
logging::LogMessage::~LogMessage [0x000000000085C931+65] (c:\src\chromium\src\base\logging.cc:533)
base::win::RegKey::Watcher::StartWatching [0x0000000000BC6B96+406] (c:\src\chromium\src\base\win\registry.cc:66)
base::win::RegKey::StartWatching [0x0000000000BC69C3+147] (c:\src\chromium\src\base\win\registry.cc:417)
extensions::ExternalRegistryLoader::CompleteLoadAndStartWatchingRegistry [0x00000000070041D0+736] (c:\src\chromium\src\chrome\browser\extensions\external_registry_loader_win.cc:220)
base::internal::FunctorTraits<void (__cdecl extensions::ExternalRegistryLoader::*)(void) __ptr64,void>::Invoke<scoped_refptr<extensions::ExternalRegistryLoader> const & __ptr64> [0x000000000700314B+59] (c:\src\chromium\src\base\bind_internal.h:215)
base::internal::InvokeHelper<0,void>::MakeItSo<void (__cdecl extensions::ExternalRegistryLoader::*const & __ptr64)(void) __ptr64,scoped_refptr<extensions::ExternalRegistryLoader> const & __ptr64> [0x000000000700352D+77] (c:\src\chromium\src\base\bind_internal.h:287)
base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::ExternalRegistryLoader::*)(void) __ptr64,scoped_refptr<extensions::ExternalRegistryLoader> >,void __cdecl(void)>::RunImpl<void (__cdecl extensions::ExternalRegistryLoader::*const [0x000000000700371C+76] (c:\src\chromium\src\base\bind_internal.h:365)
base::internal::Invoker<base::internal::BindState<void (__cdecl extensions::ExternalRegistryLoader::*)(void) __ptr64,scoped_refptr<extensions::ExternalRegistryLoader> >,void __cdecl(void)>::Run [0x00000000070064D3+51] (c:\src\chromium\src\base\bind_internal.h:343)
base::internal::RunMixin<base::Callback<void __cdecl(void),1,1> >::Run [0x00000000007A3C74+84] (c:\src\chromium\src\base\callback.h:65)
base::debug::TaskAnnotator::RunTask [0x00000000007F93D7+503] (c:\src\chromium\src\base\debug\task_annotator.cc:56)
base::MessageLoop::RunTask [0x00000000008A11A9+953] (c:\src\chromium\src\base\message_loop\message_loop.cc:458)
base::MessageLoop::DeferOrRunPendingTask [0x000000000089E5AC+60] (c:\src\chromium\src\base\message_loop\message_loop.cc:469)
base::MessageLoop::DoWork [0x000000000089EDE2+370] (c:\src\chromium\src\base\message_loop\message_loop.cc:590)
base::MessagePumpForUI::DoRunLoop [0x00000000008AC181+97] (c:\src\chromium\src\base\message_loop\message_pump_win.cc:263)
base::MessagePumpWin::Run [0x00000000008AE8DD+157] (c:\src\chromium\src\base\message_loop\message_pump_win.cc:143)
base::MessageLoop::RunHandler [0x00000000008A0DA6+246] (c:\src\chromium\src\base\message_loop\message_loop.cc:421)
base::RunLoop::Run [0x000000000098084D+61] (c:\src\chromium\src\base\run_loop.cc:36)
ChromeBrowserMainParts::MainMessageLoopRun [0x0000000004E661EB+363] (c:\src\chromium\src\chrome\browser\chrome_browser_main.cc:2110)
content::BrowserMainLoop::RunMainMessageLoopParts [0x00000000135EBD15+325] (c:\src\chromium\src\content\browser\browser_main_loop.cc:957)
content::BrowserMainRunnerImpl::Run [0x00000000135F2DB8+424] (c:\src\chromium\src\content\browser\browser_main_runner.cc:156)
content::BrowserMain [0x00000000135DAE70+192] (c:\src\chromium\src\content\browser\browser_main.cc:46)
content::RunNamedProcessTypeMain [0x000000001614E6F4+212] (c:\src\chromium\src\content\app\content_main_runner.cc:418)
content::ContentMainRunnerImpl::Run [0x000000001614E538+632] (c:\src\chromium\src\content\app\content_main_runner.cc:786)
content::ContentMain [0x000000001614B901+129] (c:\src\chromium\src\content\app\content_main.cc:20)
ChromeMain [0x00000000031021AC+300] (c:\src\chromium\src\chrome\app\chrome_main.cc:97)
MainDllLoader::Launch [0x000000014003293B+1131] (c:\src\chromium\src\chrome\app\main_dll_loader_win.cc:170)
wWinMain [0x000000014002B774+676] (c:\src\chromium\src\chrome\app\chrome_exe_main_win.cc:245)
invoke_main [0x00000001400FEA5D+45] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:118)
__scrt_common_main_seh [0x00000001400FE907+295] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
__scrt_common_main [0x00000001400FE7CE+14] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:296)
wWinMainCRTStartup [0x00000001400FEA79+9] (f:\dd\vctools\crt\vcstartup\src\startup\exe_wwinmain.cpp:17)
BaseThreadInitThunk [0x00007FFB99188102+34]
RtlUserThreadStart [0x00007FFB9977C5B4+52]
I guess the callbacks are called on a destroyed ExternalRegistryLoader object?
This used to be a comment in the old bug but seems to have been lost in the noise. https://bugs.chromium.org/p/chromium/issues/detail?id=447040#c75
,
Oct 5 2016
The base::Unretained(this) in external_registry_loader_win.cc looks wrong :\
base::win::RegKey::ChangeCallback callback =
base::Bind(&ExternalRegistryLoader::OnRegistryKeyChanged,
base::Unretained(this), base::Unretained(&hkcu_key_));
hkcu_key_.StartWatching(callback);
I'm going to try to repro this with ExtenInstallForceList, hopefully that will work.
But basically since ExternalRegistryLoader is ref counted, we can and should use retained ref on callbacks.
,
Oct 11 2016
Getting back to this, the base::Unretained(this) ^^^ is safe/correct actually. I'll add a comment to the code as to why. The cause of this bug is actually calling ExternalLoader.StartLoading() for a loader that has already been loaded. This happens when content verification fails (https://codereview.chromium.org/2336403002/): ChromeContentVerifierDelegate::VerifyFailed() ExtensionService::CheckForExternalUpdates() ExternalProviderImpl::VisitRegisteredExtension() ExternalRegistyLoader::StartLoading() ... The failures are in DCHECKs, should just work on release builds. Either I or Antony will fix the issue...
,
Apr 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/36920277941e915314f558ada0a27236c633f751 commit 36920277941e915314f558ada0a27236c633f751 Author: lazyboy <lazyboy@chromium.org> Date: Tue Apr 04 23:51:32 2017 [Ext Registry] Do not try to re-watch registry if we are already watching it. Firstly, registry observer/watcher is already set up, then we do not need to observe that again. Because we didn't use to call ExternalLoader::StartLoading more than once before https://crrev.com/2336403002, this wan't an issue when we added registry watching code. After we started calling StartLoading > 1nce, we unintentionally started retrying to observe windows registry if previous observing failed for some reason. This CL also removes this hard-to-explain retry. The reasoning is that if we want to retry observing registry, we should be doing that explicitly (and periodically?) and not rely on whether additional StartLoading was called or not. Secondly, RegKey::Watcher fails a DCHECK if we want to re-observe using same RegKey. This is DCHECK(callback_.is_null()) in RegKey::Watcher::StartWatching. BUG= 653045 Test=On windows, make chrome have a policy forced extension installed. Shut down chrome. Manually corrupt the policy installed extension. Re-laucnch chrome in debug mode: observe no more DCHECK failures. Review-Url: https://codereview.chromium.org/2796783002 Cr-Commit-Position: refs/heads/master@{#461903} [modify] https://crrev.com/36920277941e915314f558ada0a27236c633f751/chrome/browser/extensions/external_registry_loader_win.cc [modify] https://crrev.com/36920277941e915314f558ada0a27236c633f751/chrome/browser/extensions/external_registry_loader_win.h [add] https://crrev.com/36920277941e915314f558ada0a27236c633f751/chrome/browser/extensions/external_registry_loader_win_unittest.cc [modify] https://crrev.com/36920277941e915314f558ada0a27236c633f751/chrome/test/BUILD.gn
,
Apr 5 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by asargent@chromium.org
, Oct 5 2016Owner: lazyboy@chromium.org
Status: Assigned (was: Untriaged)