529821 - Places should shutdown earlier (at profile-before-change notification)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We shutdown at xpcom-shutdown atm, Timeless made me notice Places is effectively a component depending on a profile, so we should probably shutdown a bit earlier when the profile disappear.
Though, this requires xpcshell-tests depending on a profile to do_get_profile() and then fire this notification before xpcom-shutdown.
Our part should not be too hard, changing the notification should be enough, Ted says xpcshell head.js can probably be fixed to take this in count for all tests requiring a profile (that would still have to do_get_profile)

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

bug 529823 covers the xpcshell tests part

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

moves shutdown from xpcom-shutdown to profile-before-change, adding a specialized places-shutdown notification that listeners can use.
This should solve some really ugly network leaks we had and have due to initing network connection after xpcom-shutdown (with favicons for example) and is more correct since Places depends on the profile.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 440224 [details] [diff] [review]
patch v1.1

Asking addl review to Ehsan for privatebrowsing head file changes

Comment 5

[(no longer active)]

Comment on attachment 440224 [details] [diff] [review]
patch v1.1

r=me on the privatebrowsing parts, assuming that you've tested this to make sure that it doesn't break PB xpcshell tests.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 440224 [details] [diff] [review]
patch v1.1

For review comments with more context, see http://reviews.visophyte.org/r/440224/

on file: browser/components/nsBrowserGlue.js line 226
>     Services.obs.addObserver(this, "xpcom-shutdown", false);
>     Services.obs.addObserver(this, "prefservice:after-app-defaults", false);
>     Services.obs.addObserver(this, "final-ui-startup", false);
>     Services.obs.addObserver(this, "sessionstore-windows-restored", false);
>     Services.obs.addObserver(this, "browser:purge-session-history", false);
>     Services.obs.addObserver(this, "quit-application-requested", false);
>     Services.obs.addObserver(this, "quit-application-granted", false);
> #ifdef OBSERVE_LASTWINDOW_CLOSE_TOPICS
>     Services.obs.addObserver(this, "browser-lastwindow-close-requested", false);
>     Services.obs.addObserver(this, "browser-lastwindow-close-granted", false);
> #endif
>     Services.obs.addObserver(this, "session-save", false);
>     Services.obs.addObserver(this, "places-init-complete", false);
>     this._isPlacesInitObserver = true;
>     Services.obs.addObserver(this, "places-database-locked", false);
>     this._isPlacesLockedObserver = true;
>     Services.obs.addObserver(this, "distribution-customization-complete", false);
>     Services.obs.addObserver(this, "places-shutdown", false);

really should have a local variable for obs at least here.  Could even do
addObserver:
let addObserver = Services.obs.addObserver


on file: browser/components/nsBrowserGlue.js line 250
>     Services.obs.removeObserver(this, "xpcom-shutdown");
>     Services.obs.removeObserver(this, "prefservice:after-app-defaults");
>     Services.obs.removeObserver(this, "final-ui-startup");
>     Services.obs.removeObserver(this, "sessionstore-windows-restored");
>     Services.obs.removeObserver(this, "browser:purge-session-history");
>     Services.obs.removeObserver(this, "quit-application-requested");
>     Services.obs.removeObserver(this, "quit-application-granted");
> #ifdef OBSERVE_LASTWINDOW_CLOSE_TOPICS
>     Services.obs.removeObserver(this, "browser-lastwindow-close-requested");
>     Services.obs.removeObserver(this, "browser-lastwindow-close-granted");
> #endif
>     Services.obs.removeObserver(this, "session-save");
>     if (this._isIdleObserver)
>       this._idleService.removeIdleObserver(this, BOOKMARKS_BACKUP_IDLE_TIME);
>     if (this._isPlacesInitObserver)
>       Services.obs.removeObserver(this, "places-init-complete");
>     if (this._isPlacesLockedObserver)
>       Services.obs.removeObserver(this, "places-database-locked");
>     if (this._isPlacesShutdownObserver)
>       Services.obs.removeObserver(this, "places-shutdown");

same here re: local variable.


on file: browser/components/places/src/nsPlacesTransactionsService.js line 52
> const TOPIC_SHUTDOWN = "places-shutdown";

I'm starting to think we need a JSM that has a list of constants for observer
topics for us...


on file: browser/components/places/src/nsPlacesTransactionsService.js line 71
>   Services.obs.addObserver(this, TOPIC_SHUTDOWN, false);

...which we'd then use at all our call sites.  Your call, but I think it's
probably worthwhile to do.


on file: browser/components/places/tests/unit/test_browserGlue_shutdown.js line 49
> let bs = PlacesUtils.bookmarks;
> 
> // Get other services.
> let ps = Services.prefs;
> let os = Services.obs;

WIN


on file: toolkit/components/places/src/nsNavHistory.cpp line 204
> #define TOPIC_SHUTDOWN "profile-before-change"
> #define TOPIC_PLACES_SHUTDOWN "places-shutdown"

and a .h file that holds them too :/


r=sdwilsh

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #6)
> (From update of attachment 440224 [details] [diff] [review])
> >     Services.obs.addObserver(this, "session-save", false);
> >     Services.obs.addObserver(this, "places-init-complete", false);
> >     this._isPlacesInitObserver = true;
> >     Services.obs.addObserver(this, "places-database-locked", false);
> >     this._isPlacesLockedObserver = true;
> >     Services.obs.addObserver(this, "distribution-customization-complete", false);
> >     Services.obs.addObserver(this, "places-shutdown", false);
> 
> really should have a local variable for obs at least here.  Could even do
> addObserver:
> let addObserver = Services.obs.addObserver

There was one, called osvr, but Gavin removed it recently when moving to Services... So i'll ask him if he's fine with the change. i like the idea of function alias.

> on file: browser/components/places/src/nsPlacesTransactionsService.js line 52
> > const TOPIC_SHUTDOWN = "places-shutdown";
> 
> I'm starting to think we need a JSM that has a list of constants for observer
> topics for us...

PlacesUtils could do that, it is already partially, but we don't need it everywhere. i know modules are pretty fast, but adding a dedicated module for some constants looks a bit awkward

> on file: toolkit/components/places/src/nsNavHistory.cpp line 204
> > #define TOPIC_SHUTDOWN "profile-before-change"
> > #define TOPIC_PLACES_SHUTDOWN "places-shutdown"
> 
> and a .h file that holds them too :/

But they are needed only in this file so far... i think we should add new headers when we need them.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #7)
> > and a .h file that holds them too :/
> 
> But they are needed only in this file so far... i think we should add new
> headers when we need them.

ando also, practically any file includes nsNavHistory.h, thus i'll move them there

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #5)
> (From update of attachment 440224 [details] [diff] [review])
> r=me on the privatebrowsing parts, assuming that you've tested this to make
> sure that it doesn't break PB xpcshell tests.

yep, i ran all tests and they were fine. btw as usual i'll tryserver the full thing before going to central.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

So, i'm now sharing places topics through PlacesUtils and nsNavHistory.h everywhere possible.
atm nor browserGlue nor other internal Places services are using PlacesUtils, that should be done elsewhere though. browserGlue Places part should probably be consolidated into a PlacesGlue helper, sharing some code in PlacesUtils that should be a lazy cu.import getter in it, and this thing should be done elsewhere too.
addressed any other comment.
i'll proceed on tryserver.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.3

fix typos

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.4

last missing change in head_expiration.js
tryserver gave random mochitest-plain failures, i did not touch them, and a couple are known oranges, thus i think this is good enough.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/80e39b33fc3a

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 440521 [details] [diff] [review]
patch v1.4

>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js

>-    // observer registration
>-    Services.obs.addObserver(this, "xpcom-shutdown", false);
>-    Services.obs.addObserver(this, "prefservice:after-app-defaults", false);
>-    Services.obs.addObserver(this, "final-ui-startup", false);
>-    Services.obs.addObserver(this, "sessionstore-windows-restored", false);
>-    Services.obs.addObserver(this, "browser:purge-session-history", false);
>-    Services.obs.addObserver(this, "quit-application-requested", false);
>-    Services.obs.addObserver(this, "quit-application-granted", false);
>+    let addObserver = Services.obs.addObserver;
>+    addObserver(this, "xpcom-shutdown", false);

This is a harmful pattern and should be reverted, imho.

var b = 2; var a = {b: 1, c: function () this.b }; a.c()
>> 1

var b = 2; var a = {b: 1, c: function () this.b }; var x = a.c; x()
>> 2

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yeah, dao is right, we are changing the scope of the method, going to change to

let os = Services.obs;
os.addObserver();

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

fixed scope change
http://hg.mozilla.org/mozilla-central/rev/1522c1d83f41

Comment 17

[Shawn Wilsher :sdwilsh]

Documented places-shutdown here:
https://developer.mozilla.org/en/Observer_Notifications#section_14