gtestify toolkit/components/places/tests/cpp/test_IHistory.cpp

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
We are converting GeckoCppUnitTests to gtests for bug 1313141. This bug is about toolkit/components/places/tests/cpp/test_IHistory.cpp.
Assignee

Comment 1

3 years ago
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...
Attachment #8809719 - Flags: feedback?(mak77)
Assignee

Updated

3 years ago
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.
Assignee

Comment 4

3 years ago
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.
Assignee

Comment 5

3 years ago
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 :(
Assignee

Comment 6

3 years ago
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)
Assignee

Updated

3 years ago
Attachment #8809719 - Attachment is obsolete: true
Attachment #8809719 - Flags: feedback?(mak77)
Assignee

Updated

3 years ago
Blocks: 1313141
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+
Assignee

Comment 8

3 years ago
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.
Assignee

Comment 9

3 years ago
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?
Assignee

Comment 11

3 years ago
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.
Assignee

Comment 12

3 years ago
The problem is with the MediaFormatReader.RequestAudioRawData. I've filed bug 1318202.
Assignee

Updated

3 years ago
Depends on: 1318225
Assignee

Comment 13

3 years ago
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.

Comment 14

3 years ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1db5c6d73718
gtestify toolkit/components/places/tests/cpp/test_IHistory.cpp. r=mak.
Assignee

Comment 15

3 years ago
> 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.

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1db5c6d73718
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.