Split off from bug 1538968 because there are some blockers for this.

Depends on D24909

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4566877648ff3ea523e5ed3f29fc725781e4b868

Marco, I'd like to land this but I'm a bit stuck because test_sanitizer.js creates elements (as part of parsing the HTML) that trip the DOM registervisit blah blah blah listeners. Which try to then init history.

Unfortunately, on Windows debug builds, some of the time, because the DOM code delays doing so, it can happen when we're in the middle of shutdown, and this assert https://searchfox.org/mozilla-central/rev/44a212460990ffffecf50a8e972d3cbde2e7216b/toolkit/components/places/Database.cpp#484-493 fails because we've already run the profile-change-teardown shutdown phase:

17:50:35 INFO - PID 15268 | JavaScript error: resource://gre/modules/AsyncShutdown.jsm, line 674: Error: Phase "profile-change-teardown" is finished, it is too late to register completion condition "Places Clients shutdown"

Without creating a profile in that test (which I do in the current version of the commit adding the assert), we hit the assert I'm trying to add in this test, because we attempt to create the history service when there is no profile (which will mean that places is broken, which this xpcshell test really doesn't particularly care about).

I'm not sure what best to do about this, because although xpcshell might not care, we should probably not be hitting these assertions in Database.cpp on shutdown (though admittedly they're debug only) for other places consumers in "normal" firefox runs. On the other hand, in those cases I'd expect the DB instances (and the history service) to have been instantiated sooner. I tried to make the history service's singleton getter return nullptr when we're shutting down ( https://hg.mozilla.org/try/rev/0a8232c1c8de54af23306bf98c514292d45cd30c#l2.79 ) but that doesn't seem to be sufficient, presumably I need to detect the profile-change-teardown thing separately, but I'm not sure how to do so...

Oh, meant to incliude an example of the failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03a1342a05c335402786f8421950eef1d906b61&selectedJob=237655066

Let me see if I got this correctly:
Some tests run without a profile, but expect to use profile things (like history), so you are adding a profile to those tests... But now history may be initialized in the shutdown path, after the xpcshell test harness has fired the profile shutdown notification.
My gut answer would be that the test is wrong, it should actually do everything before the test is complete. Obviously that's not a trivial answer you can implement in a minute if the number of tests is large or if there isn't a clear "complete" event you could wait for.
This is a long standing problem, services initing when shutdown is already ongoing. I wonder if we could use nsIAppStartup.shuttingDown to detect this case and bail out of the service constructor?

(In reply to Marco Bonardo [::mak] from comment #6)

Let me see if I got this correctly:
Some tests run without a profile, but expect to use profile things (like history), so you are adding a profile to those tests... But now history may be initialized in the shutdown path, after the xpcshell test harness has fired the profile shutdown notification.
My gut answer would be that the test is wrong, it should actually do everything before the test is complete. Obviously that's not a trivial answer you can implement in a minute if the number of tests is large or if there isn't a clear "complete" event you could wait for.
This is a long standing problem, services initing when shutdown is already ongoing. I wonder if we could use nsIAppStartup.shuttingDown to detect this case and bail out of the service constructor?

I tried this in the trypush in my previous comment, but it didn't seem to be enough. https://hg.mozilla.org/try/rev/0a8232c1c8de54af23306bf98c514292d45cd30c#l2.75 . I'm guessing profile-change-teardown happens before that shuttingDown flag is set...

we may need another flag then, maybe profileShuttingDown.

(In reply to :Gijs (he/him) from comment #7)

(In reply to Marco Bonardo [::mak] from comment #6)

Let me see if I got this correctly:
Some tests run without a profile, but expect to use profile things (like history), so you are adding a profile to those tests... But now history may be initialized in the shutdown path, after the xpcshell test harness has fired the profile shutdown notification.
My gut answer would be that the test is wrong, it should actually do everything before the test is complete. Obviously that's not a trivial answer you can implement in a minute if the number of tests is large or if there isn't a clear "complete" event you could wait for.
This is a long standing problem, services initing when shutdown is already ongoing. I wonder if we could use nsIAppStartup.shuttingDown to detect this case and bail out of the service constructor?

I tried this in the trypush in my previous comment, but it didn't seem to be enough. https://hg.mozilla.org/try/rev/0a8232c1c8de54af23306bf98c514292d45cd30c#l2.75 . I'm guessing profile-change-teardown happens before that shuttingDown flag is set...

OK, so xpcshell tests are the weirdest.

They manually fire all the shutdown notifications to convince things to shut down ( https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/testing/xpcshell/head.js#608-617 ) . But of course, that doesn't change nsIAppStartup.

This explains why the extra check against nsIAppStartup when creating the history service wasn't helpful.

I believe this means we can't hit this case in practice as it is, so I just hacked up the test to deal with this by forcing us to initialize the history service while the profile (and everybody's concept of xpcom shutdown state) is sane.

Backed out 2 changesets (Bug 1540170) for xpcshell failures at test_sanitizer.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/cec8e6a113fac60e16520b2bcff0b0f5d4eb27ab

Push that started the failures (this test dint ran here due to some bustage from before of this push): https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=a13bf582f195439790257dcd375838f2aced5da3

Failure log:https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238301517&repo=autoland&lineNumber=1831

Ugh.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea75baa6e51a94790170b17845a021b12ef0d298

https://hg.mozilla.org/mozilla-central/rev/c2298e57e803
https://hg.mozilla.org/mozilla-central/rev/c36ad62407f3