529823 - ensure that xpcshell tests using do_get_profile will fire profile shutdown notifications

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Marco Bonardo [:mak]]

if a test requires profile then the harness should fire related profile notifications (like profile-before-change)

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

So, i think i'm taking the easy road, i see that we have some test that rely on firing profile-after-change later, so we can't just fire them before the tests :(

Instead, no test relies on profile shutting down notifications, thus i'm going to fire those shutdown notifications if do_get_profile has been called before the test finishes.  This should ensure no leaks for those services relying on profile shutdown.

This is actually all i need to move Places shutdown to be profile dependent, actually i could even just do this in all places heads, but looks like something good for general testing.

Ted, what do you think?

Comment 2

[Marco Bonardo [:mak]]

also, any test in need to fire profile shutdown notifications earlier can do so, without big deal.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 435716 [details] [diff] [review]
patch v1.0

>diff --git a/testing/xpcshell/head.js b/testing/xpcshell/head.js
>--- a/testing/xpcshell/head.js
>+++ b/testing/xpcshell/head.js
>+  if (_hasProfile) {
>+    // Fire profile related notifications to ensure correct cleanup on shutdown.
>+    let obsSvc = Components.classes["@mozilla.org/observer-service;1"].
>+                 getService(Components.interfaces.nsIObserverService);
>+    obsSvc.notifyObservers(null, "profile-change-net-teardown", null);
>+    obsSvc.notifyObservers(null, "profile-change-teardown", null);
>+    obsSvc.notifyObservers(null, "profile-before-change", null);
>+  }

Instead of doing this, could you just make do_get_profile call do_register_cleanup, and pass this in as a cleanup function? (It could even be an anonymous function.) Then you don't need the extra global var either.

r=me with that change.

Comment 4

[(not currently active) Ted Mielczarek]

(In reply to comment #1)
> So, i think i'm taking the easy road, i see that we have some test that rely on
> firing profile-after-change later, so we can't just fire them before the tests
> :(

As a followup, maybe we could add an extra parameter to do_get_profile that indicates whether the profile-after-change notifications should be sent? That way most tests would get them by default when using do_get_profile, but tests that don't want them can opt out. (I assume it's not a majority of the tests using do_get_profile.)

Comment 5

[Marco Bonardo [:mak]]

good suggestion for using register cleanup! will do.

Well, all Places tests call do_get_profile() in the head.
It's not a problem for us if those notifications fire or not.

There are about 4 or 5 tests globally that use profile-after-change, and i think just a couple need to fire the notification at will. Still since the notification fires on do_get_profile i'm unsure about the usefulness of it, and i'm not sure if the default should be to fire, or to not fire them. Btw, since it involves test changes i agree should be a followup.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 7

[Marco Bonardo [:mak]]

wtf, i attached the wrong patch :)

Comment 8

[Marco Bonardo [:mak]]

Attachment: real patch v1.1

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/afe194ff6c08