Closed Bug 1215885 Opened 4 years ago Closed 4 years ago

Shutdown crash [@ mozilla::storage::Service::Observe] with privacy.sanitize.sanitizeOnShutdown

Categories

(Firefox :: Bookmarks & History, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: jruderman, Assigned: Yoric)

References

Details

(Keywords: regression)

Attachments

(3 files)

1. Create a directory with the following prefs.js:
  user_pref("privacy.sanitize.sanitizeOnShutdown", true);
  user_pref("browser.tabs.remote.autostart", false);
  user_pref("browser.tabs.remote.autostart.1", false);
  user_pref("browser.tabs.remote.autostart.2", false);

2. Run a Firefox with e.g. "-profile ~/pdir/"

3. Quit Firefox

Result:
  * Lots of JS errors, starting with "Error sanitizing cookies"
  * In debug builds only, a crash [@ mozilla::storage::Service::Observe]
I've found a security bug in this feature in the past (bug 851416), so it would be nice to have this fixed and be able to test the feature again.
99% is due to bug 1213124 that is a regression of bug 1089695. We are not doing shutdown in the proper order anymore.
Blocks: 1089695
Depends on: 1213124
Keywords: regression
Component: General → Bookmarks & History
Priority: -- → P1
I'll try and get another look at what went wrong with bug 1089695 tomorrow.
Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r?mak
Testing in progress.
Assignee: nobody → dteller
As expected, the shutdown of Places is complicated and we clearly haven't nailed it down yet.

I believe that the following covers the dependencies:

1/ Places shutdown should be part of profile-before-change (async blocker).
2/ Shutdown of external users of Places should be part of Places shutdown (async blocker), before either the low-level connection or the high-level wrapper is closed.
3/ Closing the high-level wrapper should be part of Places shutdown (async blocker), before the low-level connection is closed.
4/ Closing the high-level wrapper should block Sqlite shutdown, but is not part of that shutdown.

* We currently attempt to handle 1/, 2/ and 3/ with just two shutdown barriers, but these are actually three distinct stages of shutdown, all of them async, so that's actually not a good idea.
* We also get 4/ wrong.

I'm experimenting a new version that I hope will improve things.
Comment on attachment 8676680 [details]
MozReview Request: Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r=mak

Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r?mak
For reasons I do not understand, I currently cannot run tests for this repo, so I'm not sure how fast I'll be able to go with this bug.
Comment on attachment 8676680 [details]
MozReview Request: Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r=mak

Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r?mak
Attachment #8676680 - Flags: review?(mak77)
Comment on attachment 8676759 [details]
MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj

Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r?froydnj
Attachment #8676759 - Flags: review?(nfroyd)
As mentioned, I haven't been able to run tests on the above patches, but in a `./mach run` build, the SQL warnings do not appear, which I believe is a good sign.
Comment on attachment 8676680 [details]
MozReview Request: Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r=mak

Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r?mak
Comment on attachment 8676759 [details]
MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj

Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r?froydnj
I have finally been able to run tests, and it seems to work. Waiting for final result.
Comment on attachment 8676759 [details]
MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj

https://reviewboard.mozilla.org/r/22805/#review20481
Attachment #8676759 - Flags: review?(nfroyd) → review+
Attachment #8676680 - Flags: review?(mak77) → review+
Comment on attachment 8676680 [details]
MozReview Request: Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r=mak

https://reviewboard.mozilla.org/r/22787/#review20585

As discussed over IRC, it would be much nicer to factor out the shutdown code to an helper reused by both connection and wrapper.
but it looks good off-hand. Thanks
Comment on attachment 8676680 [details]
MozReview Request: Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r=mak

Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r=mak
Attachment #8676680 - Attachment description: MozReview Request: Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r?mak → MozReview Request: Bug 1215885 - The shutdown of the Places database must be triggered by the shutdown of Places, not by that of Sqlite.jsm;r=mak
Comment on attachment 8676759 [details]
MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj

Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj
Attachment #8676759 - Attachment description: MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r?froydnj → MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj
Grmpf. For some reason, I can't push to try using MozReview these days.
https://hg.mozilla.org/mozilla-central/rev/6735d6ed0ad1
https://hg.mozilla.org/mozilla-central/rev/ab1666bd0cc7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Blocks: 1213124
No longer depends on: 1213124
See Also: → 1206393
You need to log in before you can comment on or make changes to this bug.