We are converting GeckoCppUnitTests to gtests for bug 1313141. This bug is about toolkit/components/places/tests/cpp/test_IHistory.cpp.
mak: I'm having a problem with leaked URLs, as reported by the DumpLeakedURLs class. I've commented out some of the tests which makes the problem go away. Do you have any idea what the problem might be? We don't get these leaked URLs in the old standalone CPP unit test. I did remove the ScopedXPCOM::GetProfileDirectory() call -- might it be related to that? I don't understand what the "Initialize a profile folder to ensure a clean shutdown" comment means...
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
without the GetProfileDirectory call we don't have a profile (but likely the test will work) but mostly we won't fire the profile-change-teardown and profile-before-change notifications, and that will likely cause leaks in most components using a profile. That's what the test calls a "clean shutdown", that is a shutdown where all the expected xpcom notifications are fired (profile-change-teardown, profile-before-change, profile-before-change2 (sigh), xpcom-shutdown, xpcom-shutdown-threads) Does not gtest do that? How does it support components using a profile folder?
here is the list of notifications fired by ~ScopedXPCOM http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/xpcom/tests/TestHarness.h#103 As you can see they are only fired if mProfD is defined.
I tried creating a cut-down version of ScopedXPCOM, called ScopedProf, much like the one in attachment 8808469 [details] [diff] [review], but it didn't seem to help. I'll try again. Thank you for the suggestion.
I changed RunGTestFunc() in testing/gtest/mozilla/GTestRunner.cpp to always call ScopedXPCOM::GetProfileDirectory(), and I confirmed that mProfD was non-null and therefore the notifications were fired at shutdown. But I'm still getting the URL leaks. I also confirmed ~ScopedXPCOM() runs before the URL leak detection. AFAICT, that invalidates the hypothesis that missing profile shutdown is the cause of the leaks. Now I'm stumped :(
mak: the profile shutdown was a red herring. Turns out the cause of the leaks was the switch from the fake Link constructor to the real Link constructor. Once I added an alternative constructor that doesn't set |mHistory| the leaks went away.
Attachment #8810289 - Flags: review?(mak77)
Comment on attachment 8810289 [details] [diff] [review] gtestify toolkit/components/places/tests/cpp/test_IHistory.cpp Review of attachment 8810289 [details] [diff] [review]: ----------------------------------------------------------------- thank you for doing this conversion! I suspect the leaked urls are somehow not invoking unregisterVisitedCallback, it would be interesting to follow where that happens, but it's not critical for the test itself.
Attachment #8810289 - Flags: review?(mak77) → review+
Hmm. When I run this new gtest by itself it works fine, or if I run it along with part of the gtest suite. But if I run the whole gtest suite I get a timeout due to a hang at shutdown. It's got something to do with WaitForTopicSpinner, which is used by WaitForConnectionClosed. ~ScopedXPCOM() runs and mProfD is set and NotifyObservers("profile-before-change") is called and we end up in WaitForTopicSpin::Spinner, looping indefinitely.
For some reason, when the "profile-before-change" notification is sent from ~ScopedXPCOM(), WaitForTopicSpinner::Observe() isn't triggered, which leads to the hang.
AFAICT profile-before-change creates a WaitForTopicSpinner for places-connection-closed notification, so what should hit WaitForTopicSpinner::Observe() is places-connection-closed. Places shutdown is explained in this comment http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/toolkit/components/places/Shutdown.h#18 We are at phase 3 here when profile-before-change is fired. I wonder if it's possible that the db closes before WaitForTopicSpinner starts listening here http://searchfox.org/mozilla-central/source/toolkit/components/places/tests/cpp/places_test_harness.h#393 Maybe you could see if changing WaitForConnectionClosed to use profile-change-teardown instead of profile-before-change helps. How long is the gtest timeout? Maybe it's too short to allow a db closing?
WaitForTopicSpinner is created by WaitForConnectionClosed's constructor. So they are both created at the start of the test. I've done some printf debugging. The problem is that, when all the tests are run, the "places-connection-closed" notification isn't sent, because ConnectionShutdownBlocker::Complete doesn't run. It's a concern that subtle shutdown problems are possible here. When the test was standalone the potential for problems was low. But now that this is just one of many gtests running within a single process, the potential for badness is obviously higher. Hmm.
The problem is with the MediaFormatReader.RequestAudioRawData. I've filed bug 1318202.
I did a try run with MediaFormatReader.RequestAudioRawData disabled and it was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43b98e038eceb24d0b0899abd5536e42afdd9a19 So I will land this once bug 1318225 lands and that test is gone.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1db5c6d73718 gtestify toolkit/components/places/tests/cpp/test_IHistory.cpp. r=mak.
> So I will land this once bug 1318225 lands and that test is gone. I ended up just commenting out the test in the moz.build file and landing ahead of bug 1318225.
thank you again for the conversion.
You need to log in before you can comment on or make changes to this bug.