Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
566573004:
Remove deprecated extension notification from themeservice.
|
Created:
6 years, 3 months ago by Jitu( very slow this week) Modified:
5 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis patch used EntensionRegistryObserver instead of DEPRECATED extension notification.
Now below DEPRECATED notifications removed.
-- NOTIFICATION_EXTENSION_LOADED_DEPRECATED
-- NOTIFICATION_EXTENSION_WILL_BE_INSTALLED_DEPRECATED
-- NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED
BUG=411568
Patch Set 1 #Patch Set 2 : added namespace #Patch Set 3 : fix build error #
Total comments: 4
Patch Set 4 : Fixed indentation #
Total comments: 11
Patch Set 5 : Fix the review comments #
Total comments: 1
Patch Set 6 : Added ExtensionRegistry as a dependency of ThemeService #Patch Set 7 : Fixed build errors #
Total comments: 1
Patch Set 8 : Fix unittest error and review comments #
Total comments: 7
Patch Set 9 : Fix review comments #
Total comments: 6
Patch Set 10 : fix #Patch Set 11 : #Patch Set 12 : Rebased #Patch Set 13 : Rebased #Patch Set 14 : Rebased #Patch Set 15 : Fixed unittest failed #
Messages
Total messages: 37 (10 generated)
jitendra.ks@samsung.com changed reviewers: + bauerb@chromium.org
PTAL...
As I already mentioned in https://codereview.chromium.org/566863002/, please remove [WIP] from the CL if you are actually planning to land it, and make sure you select a reviewer who is in the OWNERS file. https://codereview.chromium.org/566573004/diff/40001/chrome/browser/themes/th... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/40001/chrome/browser/themes/th... chrome/browser/themes/theme_service.cc:280: if (extension->is_theme() && installed_pending_load_id_ != kDefaultThemeID && One condition per line makes this more readable. Also below. https://codereview.chromium.org/566573004/diff/40001/chrome/browser/themes/th... chrome/browser/themes/theme_service.cc:283: } Fix indentation.
jitendra.ks@samsung.com changed reviewers: + pkotwicz@chromium.org
PTAL https://codereview.chromium.org/566573004/diff/40001/chrome/browser/themes/th... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/40001/chrome/browser/themes/th... chrome/browser/themes/theme_service.cc:280: if (extension->is_theme() && installed_pending_load_id_ != kDefaultThemeID && On 2014/09/12 16:05:46, Bernhard Bauer wrote: > One condition per line makes this more readable. Also below. Done. https://codereview.chromium.org/566573004/diff/40001/chrome/browser/themes/th... chrome/browser/themes/theme_service.cc:283: } On 2014/09/12 16:05:46, Bernhard Bauer wrote: > Fix indentation. Done.
I will take a look at this CL tomorrow
LGTM with nits https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.cc:103: extension_registry_ = extensions::ExtensionRegistry::Get(profile_); I am unsure if you gain anything from caching |extension_registry_|. https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... File chrome/browser/themes/theme_service.h (right): https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:81: // Overridden from ui::ThemeProvider: Nit: Can you change the comment to "ui::ThemeProvider:" ? https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:99: // Overridden from content::NotificationObserver: Nit: Can you change the comment to "content::NotificationObserver:" ? https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:104: // Overridden from extensions::ExtensionRegistryObserver: I think the new style for these comments is "extensions::ExtensionRegistryObserver:" Optional: Order the overrides in the order that they were declared in extensions::ExtensionRegistryObserver https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:110: const std::string& old_name) OVERRIDE; Nit: Remove the new line https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:114: const extensions::Extension* extension) OVERRIDE; Nit: Remove the new line
Fixed the comments https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... File chrome/browser/themes/theme_service.h (right): https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:81: // Overridden from ui::ThemeProvider: On 2014/09/16 15:18:25, pkotwicz wrote: > Nit: Can you change the comment to "ui::ThemeProvider:" ? Done. https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:104: // Overridden from extensions::ExtensionRegistryObserver: On 2014/09/16 15:18:25, pkotwicz wrote: > I think the new style for these comments is > "extensions::ExtensionRegistryObserver:" > > Optional: Order the overrides in the order that they were declared in > extensions::ExtensionRegistryObserver > Done. https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:110: const std::string& old_name) OVERRIDE; On 2014/09/16 15:18:25, pkotwicz wrote: > Nit: Remove the new line Done. https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.h:114: const extensions::Extension* extension) OVERRIDE; On 2014/09/16 15:18:25, pkotwicz wrote: > Nit: Remove the new line Done.
https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... chrome/browser/themes/theme_service.cc:103: extension_registry_ = extensions::ExtensionRegistry::Get(profile_); On 2014/09/16 15:18:25, pkotwicz wrote: > I am unsure if you gain anything from caching |extension_registry_|. ^^^ https://codereview.chromium.org/566573004/diff/80001/chrome/browser/themes/th... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/80001/chrome/browser/themes/th... chrome/browser/themes/theme_service.cc:93: extension_registry_->RemoveObserver(this); There is no guarantee that |extension_registry_| will still be alive at this point, unless you add ExtensionRegistry as a dependency of ThemeService. Even then, you should do this in Shutdown().
On 2014/09/24 13:06:42, Bernhard Bauer wrote: > https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... > File chrome/browser/themes/theme_service.cc (right): > > https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... > chrome/browser/themes/theme_service.cc:103: extension_registry_ = > extensions::ExtensionRegistry::Get(profile_); > On 2014/09/16 15:18:25, pkotwicz wrote: > > I am unsure if you gain anything from caching |extension_registry_|. > > ^^^ > > https://codereview.chromium.org/566573004/diff/80001/chrome/browser/themes/th... > File chrome/browser/themes/theme_service.cc (right): > > https://codereview.chromium.org/566573004/diff/80001/chrome/browser/themes/th... > chrome/browser/themes/theme_service.cc:93: > extension_registry_->RemoveObserver(this); > There is no guarantee that |extension_registry_| will still be alive at this > point, unless you add ExtensionRegistry as a dependency of ThemeService. Even > then, you should do this in Shutdown(). So you mean to say i need to add this as dependency in theme_service_factory.cc to make sure that this will be alive. I didn't found any shutdown() method for themeservice. So can you please let me understand which shutdown (). If possible if you can give some reference code ... will be helpful for me to understand about BrowserContextKeyedServicefactory and its uses. Sorry for trouble you. Thanks
On 2014/09/24 14:23:51, Jitu (slow this week) wrote: > On 2014/09/24 13:06:42, Bernhard Bauer wrote: > > > https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... > > File chrome/browser/themes/theme_service.cc (right): > > > > > https://codereview.chromium.org/566573004/diff/60001/chrome/browser/themes/th... > > chrome/browser/themes/theme_service.cc:103: extension_registry_ = > > extensions::ExtensionRegistry::Get(profile_); > > On 2014/09/16 15:18:25, pkotwicz wrote: > > > I am unsure if you gain anything from caching |extension_registry_|. > > > > ^^^ > > > > > https://codereview.chromium.org/566573004/diff/80001/chrome/browser/themes/th... > > File chrome/browser/themes/theme_service.cc (right): > > > > > https://codereview.chromium.org/566573004/diff/80001/chrome/browser/themes/th... > > chrome/browser/themes/theme_service.cc:93: > > extension_registry_->RemoveObserver(this); > > There is no guarantee that |extension_registry_| will still be alive at this > > point, unless you add ExtensionRegistry as a dependency of ThemeService. Even > > then, you should do this in Shutdown(). > > So you mean to say i need to add this as dependency in theme_service_factory.cc > to make sure that this will be alive. Correct. > I didn't found any shutdown() method for themeservice. So can you please let me > understand which shutdown (). ThemeService inherits from KeyedService, which has a virtual method Shutdown(). > If possible if you can give some reference code ... will be helpful for me to > understand about BrowserContextKeyedServicefactory and its uses. I would advise you to look at classes which inherit from KeyedService (and their associated factories, which usually inherit from BrowserContextKeyedServiceFactory); there are tons of them. You can also look at the implementation of e.g. BrowserContextKeyedServiceFactory. > Sorry for trouble you. > > Thanks
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
PTAL
https://codereview.chromium.org/566573004/diff/160001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.h (right): https://codereview.chromium.org/566573004/diff/160001/chrome/browser/themes/t... chrome/browser/themes/theme_service.h:119: virtual void Shutdown() OVERRIDE; Please add a comment that states from which class you're overriding the method (similar to the comments above).
Patchset #8 (id:180001) has been deleted
Fixed the review comments and the unit test fail errors. PTAL
@pkotwicz, PTAL
https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:269: if (extension->is_theme() && installed_pending_load_id_ != kDefaultThemeID && Please put each of these conditions on a separate line, to make it easier to read. https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:298: if (extension_registry_) { When is this NULL? https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.h (right): https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... chrome/browser/themes/theme_service.h:119: // KeyedService::Shutdown: This should be just KeyedService.
PTAL https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:269: if (extension->is_theme() && installed_pending_load_id_ != kDefaultThemeID && On 2014/10/15 12:32:45, Bernhard Bauer wrote: > Please put each of these conditions on a separate line, to make it easier to > read. Done. https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:298: if (extension_registry_) { On 2014/10/15 12:32:45, Bernhard Bauer wrote: > When is this NULL? This was added for safer side. Let say we created the object of ThemeService, without invoking init() method and destroyed the object. In that case extension_registry_->RemoveObserver(this); will crash. Please correct me if i am wrong. https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.h (right): https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... chrome/browser/themes/theme_service.h:119: // KeyedService::Shutdown: On 2014/10/15 12:32:45, Bernhard Bauer wrote: > This should be just KeyedService. Done.
https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/200001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:298: if (extension_registry_) { On 2014/10/15 13:01:12, (slow this week) wrote: > On 2014/10/15 12:32:45, Bernhard Bauer wrote: > > When is this NULL? > > This was added for safer side. > > Let say we created the object of ThemeService, without invoking init() method > and destroyed the object. In that case > > extension_registry_->RemoveObserver(this); > > will crash. > > Please correct me if i am wrong. That would be a bug. If we silently accept NULL here, we will never find out about that bug. Dereferencing a NULL pointer here OTOH will crash, but the crash will produce a crash report or a test failure, which we will learn about and fix. https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:281: if (reason != UnloadedExtensionInfo::REASON_UPDATE && extension->is_theme() && Also here please, and below. The original code in Observe() had this pretty much right; you can use that as a guide. https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.h (right): https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... chrome/browser/themes/theme_service.h:119: // KeyedService. Nit: add a colon at the end like the other comments do.
Patchset #10 (id:240001) has been deleted
PTAL https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:281: if (reason != UnloadedExtensionInfo::REASON_UPDATE && extension->is_theme() && On 2014/10/15 13:21:04, Bernhard Bauer wrote: > Also here please, and below. The original code in Observe() had this pretty much > right; you can use that as a guide. In original code in Observe(), it is casting the details. And checking like unloaded_details->reason != UnloadedExtensionInfo::REASON_UPDATE So same way we did it here, in the param of OnExtensionUnloaded() we are getting reason (extensions::UnloadedExtensionInfo::Reason) and checking like reason != UnloadedExtensionInfo::REASON_UPDATE. Please let me know weather this is your comment or i misunderstood. https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:299: if (extension_registry_) { Removed the check https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.h (right): https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... chrome/browser/themes/theme_service.h:119: // KeyedService. On 2014/10/15 13:21:04, Bernhard Bauer wrote: > Nit: add a colon at the end like the other comments do. Done.
https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/566573004/diff/220001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:281: if (reason != UnloadedExtensionInfo::REASON_UPDATE && extension->is_theme() && On 2014/10/15 14:19:06, (slow this week) wrote: > On 2014/10/15 13:21:04, Bernhard Bauer wrote: > > Also here please, and below. The original code in Observe() had this pretty > much > > right; you can use that as a guide. > > In original code in Observe(), it is casting the details. > And checking like > unloaded_details->reason != UnloadedExtensionInfo::REASON_UPDATE > > So same way we did it here, in the param of OnExtensionUnloaded() > we are getting reason (extensions::UnloadedExtensionInfo::Reason) and checking > like > > reason != UnloadedExtensionInfo::REASON_UPDATE. > > Please let me know weather this is your comment or i misunderstood. I was referring to the formatting. The original code has each condition on a separate line, like I pointed out for OnExtensionLoaded. The formatting should be kept for OnExtensionUnloaded() and OnExtensionWillBeInstalled() as well.
PTAL It was my mistake, after changes i gave git cl format which again changed the formatting.
lgtm
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/566573004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/566573004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2014/10/16 06:05:40, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_rel_swarming on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hi, This patch is failed while RemoveObserver(), But i observe different callstack for different build. #0 0x000002a33865 base::debug::(anonymous namespace)::StackDumpSignalHandler() #1 0x7f4381f98cb0 \u003Cunknown> #2 0x0000009550a7 ObserverListBase\u003C>::RemoveObserver() #3 0x000004f61a4d BrowserContextDependencyManager::DestroyBrowserContextServices() #4 0x00000269b3b7 TestingProfile::~TestingProfile() #5 0x0000009f61c9 extensions::DefaultAppsTest_Install_Test::TestBody()::DefaultTestingProfile::~DefaultTestingProfile() #6 0x00000166c2e1 ThemeSyncableServiceTest::TearDown() #7 0x00000265c70e testing::TestInfo::Run() #8 0x00000265cbd3 testing::TestCase::Run() #9 0x0000026646e9 testing::internal::UnitTestImpl::RunAllTests() #10 0x00000266438e testing::UnitTest::Run() #11 0x0000056d1adf base::TestSuite::Run() #12 0x0000056ce4fa base::(anonymous namespace)::LaunchUnitTestsInternal() #13 0x0000056ce2e3 base::LaunchUnitTests() #14 0x0000026b8862 main #15 0x7f438031b76d __libc_start_main #16 0x0000004a2f01 \u003Cunknown> ================================= #0 0xbe6e45c in end third_party/libc++/trunk/include/vector:1476:12 #1 0xbe6e45c in RemoveObserver base/observer_list.h:168 #2 0xbe6e45c in OmniboxPopupModel::RemoveObserver(OmniboxPopupModelObserver*) chrome/browser/ui/omnibox/omnibox_popup_model.cc:294 #3 0xd9908ff in BrowserContextDependencyManager::DestroyBrowserContextServices(content::BrowserContext*) components/keyed_service/content/browser_context_dependency_manager.cc:118:5 #4 0x5447493 in TestingProfile::~TestingProfile() chrome/test/base/testing_profile.cc:440:3 #5 0x110b3ed in extensions::DefaultAppsTest_Install_Test::TestBody()::DefaultTestingProfile::~DefaultTestingProfile() chrome/browser/extensions/default_apps_unittest.cc:75:9 #6 0x2bbe21c in operator() base/memory/scoped_ptr.h:137:5 #7 0x2bbe21c in reset base/memory/scoped_ptr.h:257 #8 0x2bbe21c in reset base/memory/scoped_ptr.h:386 #9 0x2bbe21c in ThemeSyncableServiceTest::TearDown() chrome/browser/themes/theme_syncable_service_unittest.cc:164 #10 0x537c7d0 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5 #11 0x537d935 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5 #12 0x53915fe in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4591:11 #13 0x5390bf9 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2418:12 #14 0x5390bf9 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209 #15 0xeea61f6 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10 #16 0xeea61f6 in base::TestSuite::Run() base/test/test_suite.cc:226 #17 0xee9f594 in Run base/callback.h:401:12 #18 0xee9f594 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:498 #19 0xee9f05b in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:553:10 #20 0x54933c1 in main chrome/test/base/run_all_unittests.cc:15:10 #21 0x7f9b4d65276c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #22 0x52da6c in _start (/tmp/run_tha_testcoGB40/out/Release/unit_tests+0x52da6c) ============================= app_list::SearchBoxModel::RemoveObserver [0x04647B56+6] ThemeService::Shutdown [0x02492BB6+22] BrowserContextDependencyManager::DestroyBrowserContextServices [0x047B2604+148] TestingProfile::~TestingProfile [0x027ACC4A+74] TestingProfile::`scalar deleting destructor' [0x01600F5B+11] ThemeSyncableServiceTest::TearDown [0x01D0290D+29] testing::TestCase::Run [0x027A1DE9+233] testing::internal::UnitTestImpl::RunAllTests [0x027A23B9+649] testing::UnitTest::Run [0x027A2106+214] base::TestSuite::Run [0x04EE9A9D+189] base::`anonymous namespace'::LaunchUnitTestsInternal [0x04EEF217+663] base::LaunchUnitTests [0x04EEEF6A+106] main [0x04F1B7F2+130] __tmainCRTStartup [0x04E04965+254] (f:\dd\vctools\crt\crtw32\startup\crt0.c:255) BaseThreadInitThunk [0x7534338A+18] RtlInitializeExceptionChain [0x774F9F72+99] RtlInitializeExceptionChain [0x774F9F45+54] Any clue for this.
On 2014/10/16 10:13:27, (slow this week) wrote: > On 2014/10/16 06:05:40, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_swarming on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > Hi, > > This patch is failed while RemoveObserver(), But i observe different callstack > for different build. > > #0 0x000002a33865 base::debug::(anonymous namespace)::StackDumpSignalHandler() > #1 0x7f4381f98cb0 \u003Cunknown> > #2 0x0000009550a7 ObserverListBase\u003C>::RemoveObserver() > #3 0x000004f61a4d > BrowserContextDependencyManager::DestroyBrowserContextServices() > #4 0x00000269b3b7 TestingProfile::~TestingProfile() > #5 0x0000009f61c9 > extensions::DefaultAppsTest_Install_Test::TestBody()::DefaultTestingProfile::~DefaultTestingProfile() > #6 0x00000166c2e1 ThemeSyncableServiceTest::TearDown() > #7 0x00000265c70e testing::TestInfo::Run() > #8 0x00000265cbd3 testing::TestCase::Run() > #9 0x0000026646e9 testing::internal::UnitTestImpl::RunAllTests() > #10 0x00000266438e testing::UnitTest::Run() > #11 0x0000056d1adf base::TestSuite::Run() > #12 0x0000056ce4fa base::(anonymous namespace)::LaunchUnitTestsInternal() > #13 0x0000056ce2e3 base::LaunchUnitTests() > #14 0x0000026b8862 main > #15 0x7f438031b76d __libc_start_main > #16 0x0000004a2f01 \u003Cunknown> > > ================================= > #0 0xbe6e45c in end third_party/libc++/trunk/include/vector:1476:12 > #1 0xbe6e45c in RemoveObserver base/observer_list.h:168 > #2 0xbe6e45c in > OmniboxPopupModel::RemoveObserver(OmniboxPopupModelObserver*) > chrome/browser/ui/omnibox/omnibox_popup_model.cc:294 > #3 0xd9908ff in > BrowserContextDependencyManager::DestroyBrowserContextServices(content::BrowserContext*) > components/keyed_service/content/browser_context_dependency_manager.cc:118:5 > #4 0x5447493 in TestingProfile::~TestingProfile() > chrome/test/base/testing_profile.cc:440:3 > #5 0x110b3ed in > extensions::DefaultAppsTest_Install_Test::TestBody()::DefaultTestingProfile::~DefaultTestingProfile() > chrome/browser/extensions/default_apps_unittest.cc:75:9 > #6 0x2bbe21c in operator() base/memory/scoped_ptr.h:137:5 > #7 0x2bbe21c in reset base/memory/scoped_ptr.h:257 > #8 0x2bbe21c in reset base/memory/scoped_ptr.h:386 > #9 0x2bbe21c in ThemeSyncableServiceTest::TearDown() > chrome/browser/themes/theme_syncable_service_unittest.cc:164 > #10 0x537c7d0 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5 > #11 0x537d935 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5 > #12 0x53915fe in testing::internal::UnitTestImpl::RunAllTests() > testing/gtest/src/gtest.cc:4591:11 > #13 0x5390bf9 in > HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> > testing/gtest/src/gtest.cc:2418:12 > #14 0x5390bf9 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209 > #15 0xeea61f6 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10 > #16 0xeea61f6 in base::TestSuite::Run() base/test/test_suite.cc:226 > #17 0xee9f594 in Run base/callback.h:401:12 > #18 0xee9f594 in base::(anonymous > namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, > bool, base::Callback\u003Cvoid ()> const&) > base/test/launcher/unit_test_launcher.cc:498 > #19 0xee9f05b in base::LaunchUnitTests(int, char**, base::Callback\u003Cint > ()> const&) base/test/launcher/unit_test_launcher.cc:553:10 > #20 0x54933c1 in main chrome/test/base/run_all_unittests.cc:15:10 > #21 0x7f9b4d65276c in __libc_start_main > /build/buildd/eglibc-2.15/csu/libc-start.c:226 > #22 0x52da6c in _start > (/tmp/run_tha_testcoGB40/out/Release/unit_tests+0x52da6c) > > ============================= > app_list::SearchBoxModel::RemoveObserver [0x04647B56+6] If you mean this, this is most likely a red herring. MSVC++ coalesces identical methods to save space, but this results in wrong debug symbols. It also fairly aggressively inlines method calls, which might explain why you don't see ThemeService::Shutdown in some of the stacks above. > ThemeService::Shutdown [0x02492BB6+22] > BrowserContextDependencyManager::DestroyBrowserContextServices [0x047B2604+148] > TestingProfile::~TestingProfile [0x027ACC4A+74] > TestingProfile::`scalar deleting destructor' [0x01600F5B+11] > ThemeSyncableServiceTest::TearDown [0x01D0290D+29] > testing::TestCase::Run [0x027A1DE9+233] > testing::internal::UnitTestImpl::RunAllTests [0x027A23B9+649] > testing::UnitTest::Run [0x027A2106+214] > base::TestSuite::Run [0x04EE9A9D+189] > base::`anonymous namespace'::LaunchUnitTestsInternal [0x04EEF217+663] > base::LaunchUnitTests [0x04EEEF6A+106] > main [0x04F1B7F2+130] > __tmainCRTStartup [0x04E04965+254] > (f:\dd\vctools\crt\crtw32\startup\crt0.c:255) > BaseThreadInitThunk [0x7534338A+18] > RtlInitializeExceptionChain [0x774F9F72+99] > RtlInitializeExceptionChain [0x774F9F45+54] > > Any clue for this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
