Because gfxPrefs calls mozilla::Preferences::Add[Type]VarCache with lots of its members, those members should basically stay alive for the duration of the process. This is the case on 'real' Firefox because gfxPrefs is only destroyed from gfxPlatform::Shutdown. However: http://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp and http://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestGfxPrefs.cpp have a bad habit of creating and destroying it several times in a row. This is not nice because the pref var caches are still hanging around with pointers to the deleted singleton's members. I talked with Benoit about this, and the intention is to: - add a static bool to gfxPrefs to ensure that we never recreate the singleton once it's been deleted - make the tests above only create/get the singleton, never destroy it - make gfxPrefs::DestroySingleton assert the return values of gfxPrefs::SingletonExists() to make up for the lost test coverage of TestGfxPrefs.

I hope I remembered everything from our discussion correctly. This seems to work fine locally. Try: https://tbpl.mozilla.org/?tree=Try&rev=8b0a938b89da

Sigh. bzexport should really warn me about stuff like this.

Comment on attachment 8445842 [details] [diff] [review] tests shouldn't destroy gfxPrefs, Review of attachment 8445842 [details] [diff] [review]: ----------------------------------------------------------------- R=me with the three ASSERT_TRUE(gfxPrefs::SingletonExists()); kept. ::: gfx/tests/gtest/TestGfxPrefs.cpp @@ -20,2 @@ > gfxPrefs::GetSingleton(); > - ASSERT_TRUE(gfxPrefs::SingletonExists()); This ASSERT_TRUE(gfxPrefs::SingletonExists()); is useful, please leave it. It means we don't need to trust the gfxPrefs implementation to have the right assert for that, at least. @@ -25,5 @@ > - > -TEST(GfxPrefs, LiveValues) { > - ASSERT_FALSE(gfxPrefs::SingletonExists()); > - gfxPrefs::GetSingleton(); > - ASSERT_TRUE(gfxPrefs::SingletonExists()); Likewise here, and below, the ASSERT_TRUE(gfxPrefs::SingletonExists()); are correct and should be kept. @@ -46,2 @@ > gfxPrefs::GetSingleton(); > - ASSERT_TRUE(gfxPrefs::SingletonExists()); This occurrence too. ::: gfx/thebes/gfxPrefs.cpp @@ +23,2 @@ > } > + MOZ_ASSERT(!SingletonExists(), "Should expose singleton being dead."); The 2nd parameter to moz_assert is optional, and in a majority of cases doesn't convey much more information than just the assert condition. Up to you ;-)

Great finding, by the way. This means that (depending on implemetation-defined allocator behavior) these GTests could easily have been intermittently crashing.

Comment on attachment 8445842 [details] [diff] [review] tests shouldn't destroy gfxPrefs, remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed26f559862

(In reply to :Gijs Kruitbosch from comment #0) > Because gfxPrefs calls mozilla::Preferences::Add[Type]VarCache with lots of > its members, those members should basically stay alive for the duration of > the process. This is the case on 'real' Firefox because gfxPrefs is only > destroyed from gfxPlatform::Shutdown. > Is it possible to remove the var caches cleanly? If so an alternate approach to fixing the root problem here would be to clear those caches on destroying gfxPrefs, so that it cleans up after itself, and there's no problem with destroying/re-creating it. The reason I ask is because now there's no way for different gtests to set different values for different "Once" prefs in gfxPrefs (for example, I would like to write a test where the touch-action behaviour is enabled, but I can't have that co-exist with tests where the touch-action behaviour is disabled.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > (In reply to :Gijs Kruitbosch from comment #0) > > Because gfxPrefs calls mozilla::Preferences::Add[Type]VarCache with lots of > > its members, those members should basically stay alive for the duration of > > the process. This is the case on 'real' Firefox because gfxPrefs is only > > destroyed from gfxPlatform::Shutdown. > > > > Is it possible to remove the var caches cleanly? If so an alternate approach > to fixing the root problem here would be to clear those caches on destroying > gfxPrefs, so that it cleans up after itself, and there's no problem with > destroying/re-creating it. There is currently no API to do that. I believe it could be implemented, but bsmedberg would know for sure. > The reason I ask is because now there's no way for different gtests to set > different values for different "Once" prefs in gfxPrefs (for example, I > would like to write a test where the touch-action behaviour is enabled, but > I can't have that co-exist with tests where the touch-action behaviour is > disabled. It's unfortunate that gfxPrefs uses both static-y-but-really-singleton-instance-y members for pref caches, and the same for 'once' read prefs. Of course, the more trivial way around this would be to switch the relevant "Once" pref to use a var cache as well, and just read and set the pref dynamically. I'm not familiar with the touch action code and if that (ie making it obey dynamic pref changes) is a realistic possibility at this point.

(In reply to :Gijs Kruitbosch from comment #8) > It's unfortunate that gfxPrefs uses both > static-y-but-really-singleton-instance-y members for pref caches, and the > same for 'once' read prefs. Hm, so another option might be to make the members truly static, and not singleton-instance-y. > Of course, the more trivial way around this would be to switch the relevant > "Once" pref to use a var cache as well, and just read and set the pref > dynamically. I'm not familiar with the touch action code and if that (ie > making it obey dynamic pref changes) is a realistic possibility at this > point. I don't think that's possible, given that the touch action code includes CSS parsing stuff that we don't want to change at runtime.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9) > (In reply to :Gijs Kruitbosch from comment #8) > > It's unfortunate that gfxPrefs uses both > > static-y-but-really-singleton-instance-y members for pref caches, and the > > same for 'once' read prefs. > > Hm, so another option might be to make the members truly static, and not > singleton-instance-y. Yes. The typical way to do this is to a have a static guard variable to check whether the cache was instantiated already (otherwise you'll fail the same test I added, because you'll add multiple caches against the same address, which bloats the pref observer list that the pref cache system keeps). I'm not sure how easy it is to do this considering the amount of macro/template magic in the current gfxPrefs implementation...