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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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]

Comment 1

[Jesse Ruderman]

Attachment: JS errors and C++ stack trace

Comment 2

[Jesse Ruderman]

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.

Comment 3

[Marco Bonardo [:mak]]

99% is due to bug 1213124 that is a regression of bug 1089695. We are not doing shutdown in the proper order anymore.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I'll try and get another look at what went wrong with bug 1089695 tomorrow.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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 6

[David Teller [:Yoric] - still alive but not very active]

Testing in progress.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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 8

[David Teller [:Yoric] - still alive but not very active]

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 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj

Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r?froydnj

Comment 10

[David Teller [:Yoric] - still alive but not very active]

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 11

[David Teller [:Yoric] - still alive but not very active]

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 12

[David Teller [:Yoric] - still alive but not very active]

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

Comment 13

[David Teller [:Yoric] - still alive but not very active]

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 14

[David Teller [:Yoric] - still alive but not very active]

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 15

[David Teller [:Yoric] - still alive but not very active]

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

Comment 16

[David Teller [:Yoric] - still alive but not very active]

I have finally been able to run tests, and it seems to work. Waiting for final result.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8676759 [details]
MozReview Request: Bug 1215885 - Adding a shutdown phase placesClosingInternalConnection;r=froydnj

https://reviewboard.mozilla.org/r/22805/#review20481

Comment 18

[Marco Bonardo [:mak]]

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 19

[David Teller [:Yoric] - still alive but not very active]

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 20

[David Teller [:Yoric] - still alive but not very active]

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

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Grmpf. For some reason, I can't push to try using MozReview these days.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b42038f3ee8

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6735d6ed0ad1
https://hg.mozilla.org/integration/fx-team/rev/ab1666bd0cc7

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6735d6ed0ad1
https://hg.mozilla.org/mozilla-central/rev/ab1666bd0cc7