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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Toolkit
Places
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.3a5
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

Updated

9 years ago
Depends on: 529823
(Assignee)

Comment 1

9 years ago
bug 529823 covers the xpcshell tests part
(Assignee)

Updated

8 years ago
Summary: Places should move shutdown a bit earlier (profile-before-change) → Places should shutdown a earlier (profile-before-change)
(Assignee)

Updated

8 years ago
Summary: Places should shutdown a earlier (profile-before-change) → Places should shutdown a earlier (at profile-before-change notification)
(Assignee)

Comment 2

8 years ago
Created attachment 435906 [details] [diff] [review]
patch v1.0
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Summary: Places should shutdown a earlier (at profile-before-change notification) → Places should shutdown earlier (at profile-before-change notification)
(Assignee)

Updated

8 years ago
Depends on: 556376
(Assignee)

Comment 3

8 years ago
Created attachment 440224 [details] [diff] [review]
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.
Attachment #435906 - Attachment is obsolete: true
Attachment #440224 - Flags: review?(sdwilsh)
(Assignee)

Comment 4

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

Asking addl review to Ehsan for privatebrowsing head file changes
Attachment #440224 - Flags: review?(ehsan)

Comment 5

8 years ago
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.
Attachment #440224 - Flags: review?(ehsan) → review+
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
Attachment #440224 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Comment 8

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

Comment 9

8 years ago
(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.
(Assignee)

Comment 10

8 years ago
Created attachment 440374 [details] [diff] [review]
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.
Attachment #440224 - Attachment is obsolete: true
(Assignee)

Comment 11

8 years ago
Created attachment 440439 [details] [diff] [review]
patch v1.3

fix typos
Attachment #440374 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Created attachment 440521 [details] [diff] [review]
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.
Attachment #440439 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/80e39b33fc3a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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
(Assignee)

Comment 15

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

let os = Services.obs;
os.addObserver();
Documented places-shutdown here:
https://developer.mozilla.org/en/Observer_Notifications#section_14
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.