Closed
Bug 727996
Opened 12 years ago
Closed 2 years ago
nsContentPrefService.js should observe profile-before-change instead of xpcom-shutdown to close its database connection
Categories
(Toolkit :: Preferences, defect)
Toolkit
Preferences
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: adw, Unassigned)
Details
Attachments
(1 file)
From bug 699859 comment 25: > ::: toolkit/components/contentprefs/nsContentPrefService.js > @@ +143,5 @@ > > } catch (e) {} > > this._observerSvc.addObserver(this, "private-browsing", false); > > > > // Observe shutdown so we can shut down the database connection. > > this._observerSvc.addObserver(this, "xpcom-shutdown", false); > > sigh, this is plain wrong... could you please file a bug to move this to a > proper profile-before-change?
Comment 1•11 years ago
|
||
Attachment #722836 -
Flags: review?(mak77)
Updated•11 years ago
|
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Comment 2•11 years ago
|
||
Comment on attachment 722836 [details] [diff] [review] make nsContentPrefService observe profile-before-change instead of xpcom-shutdown Review of attachment 722836 [details] [diff] [review]: ----------------------------------------------------------------- The tests in toolkit/components/contentprefs/tests/unit/ don't seem to call do_get_profile, instead they implement some fancy thing here http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js#86 Unfortunately this means they won't properly invoke profile shutdown notifications and may leak. That should probably be fixed or this change may cause failures. I'd probably suggest to immediately call do_get_profile() in the head file, cache it and make getProfileDir return the cached value. A try run would be good to confirm it's fine.
Attachment #722836 -
Flags: review?(mak77) → review+
Comment 3•11 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #2) > Unfortunately this means they won't properly invoke profile shutdown > notifications and may leak. That should probably be fixed or this change may > cause failures. > I'd probably suggest to immediately call do_get_profile() in the head file, > cache it and make getProfileDir return the cached value. A try run would be > good to confirm it's fine. Are you asking me to do that here? The head_contentPrefs.js code is just bizarre. And the tests pass fine with the r+'d patch. I don't think the normal xpcom shutdown notifications are sent during xpcshell.
Flags: needinfo?(mak77)
Comment 4•11 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #3) > Are you asking me to do that here? The head_contentPrefs.js code is just > bizarre. And the tests pass fine with the r+'d patch. I don't see how the tests can pass in a debug build without assertions (did you check for assertions in the console?). We open a database connection and don't close it, it should definitely assert. > I don't think the normal xpcom shutdown notifications are sent during > xpcshell. xpcshell sends xpcom-shutdown, that is what was working until this patch, profile notifications are not invoked unless the test invokes do_get_profile() at least once.
Flags: needinfo?(mak77)
Comment 5•5 years ago
|
||
Looks like ContentPrefServiceChild.jsm still uses xpcom-shutdown, not sure if that's right or not... over to preferences.
Component: General → Preferences
Comment 6•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months.
:jaws, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: froydnj+bz → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jaws)
Comment 7•2 years ago
|
||
bug 886907 removed the synchronous content pref service, long live the synchronous content pref service.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jaws)
Resolution: --- → WORKSFORME
Comment 8•2 years ago
|
||
(In reply to Mark Banner (:standard8) (afk until 18 Apr) from comment #5)
Looks like ContentPrefServiceChild.jsm still uses xpcom-shutdown, not sure if that's right or not... over to preferences.
... and bug 1645538 removed it there.
You need to log in
before you can comment on or make changes to this bug.
Description
•