Open Bug 1364496 Opened 9 years ago Updated 4 years ago

Get rid of all sync pref file saves

Categories

(Core :: Preferences: Backend, enhancement)

Product:
Core
Component:
Preferences: Backend
Type:
enhancement

Tracking

()

People

(Reporter: milan, Unassigned)

References

Details

If bug 789945 solution gets us to the point where some of the pref file saves are still synchronous, we should try to get rid of those in this bug.
https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#857 in CreateContentProcessSandboxTempDir(). We count on evaluating a success of the preference file save, to decide if a temporary sandbox directory is going to be remembered. And if this succeed, we potentially create temporary directory, and pass it to the callers to use.
Database::ForceCrashAndReplaceDatabase() function forces a crash using MOZ_CRASH_UNSAFE_OOL if preference file save fails.
Bob, if we were to eliminate blocking preference file writes, what should we do in the case mentioned in comment 1? Is there a way around it? An option I've been entertaining is to be able to register a callback when requesting async pref write, and get that callback executed when the actual write happens (the callback would be back on the main thread.) Would that be enough to handle the sandbox case, or is there another approach you can envision?
Flags: needinfo?(bobowencode)
::mak, same question for you, as for Bob above, but for the Database stuff - if we were to eliminate blocking preference file writes, what should we do in the case mentioned in comment 2? Is there a way around it? An option I've been entertaining is to be able to register a callback when requesting async pref write, and get that callback executed when the actual write happens (the callback would be back on the main thread.) Would that be enough to handle the sandbox case, or is there another approach you can envision?
Flags: needinfo?(mak77)
(In reply to Milan Sreckovic [:milan] from comment #4) > if we were to eliminate blocking preference file writes, what should we do > in the case mentioned in comment 2? Actually the crash is forced only if the preferences write SUCCEEDS, not if it fails. The situation there is very particular, basically we are in a situation where the database is corrupt, we must replace it, but since the replacement code may fail the only left solution is to store somewhere that the database should be replaced at the next startup and then forcibly crash the app. Otherwise the user would be stuck with a corrupt database forever. The only way to be sure that the pref is persisted before crashing the app is to synchronously write the prefs file. > I've been entertaining is to be able to register a callback when requesting > async pref write, and get that callback executed when the actual write > happens The whole database-Is-Corrupt-And-Should-Be-Replaced thing is still synchronous, because we didn't finish converting all the APIs to be asynchronous yet, thus when one of these APIs is invoked, the connection is expected to work immediately. I was working on bug 1355561 to remove some of the crash needs here, I didn't finish it cause other priorities took my time. I suppose that fixing bug 1355561 (to reduce the likely to need to crash) and then having a callback based solution as you suggested would work, we'd just wait for the prefs file to be written before crashing, but only in rare cases where bug 1355561 doesn't solve the problem.
Flags: needinfo?(mak77)
(In reply to Milan Sreckovic [:milan] from comment #3) > Bob, if we were to eliminate blocking preference file writes, what should we > do in the case mentioned in comment 1? Is there a way around it? An option > I've been entertaining is to be able to register a callback when requesting > async pref write, and get that callback executed when the actual write > happens (the callback would be back on the main thread.) Would that be > enough to handle the sandbox case, or is there another approach you can > envision? The main concern here was leaving "temporary" directories in a non-obvious place, where the average user may not know that they might need to look when tidying up their drive. Perhaps that case is too rare to actually require us to wait for the save. Also, with the next policy I'm working on I need to give explicit permission to write to the temp directory anyway, so we could in theory think about moving it under the normal windows TEMP folder, which people are more likely to look for things to delete. In addition to this we could think about adding some clean-up code that looks through the temp dirs and deletes any that aren't in profiles (possibly even profiles that are in use). This could run at some idle point/shutdown/on install or update. Given bug 1358964 this is probably a good idea anyway. In short, I think we could live with this being async. I'm not sure what we'd do if the save failed, because it's possibly too late to do anything about it. So, I think we'd just need a fire and forget save.
Flags: needinfo?(bobowencode)
profile-before-change is a shutdown thing, right? We seem to have to do sync save at that point, it's too late to register a shutdown observer in order to do an async save?
We should use a shutdown blocker (nsIAsyncShutdownBlocker).
Right - doing that, but I wasn't "allowed" (asserts) to create a shutdown blocker during this profile-before-change observer call; it seems that's not allowed at that point in the shutdown procedure. Should be OK without it at that point though, I'll attach patches.
(In reply to Milan Sreckovic [:milan] from comment #9) > Right - doing that, but I wasn't "allowed" (asserts) to create a shutdown > blocker during this profile-before-change observer call; it seems that's not > allowed at that point in the shutdown procedure. Should be OK without it at > that point though, I'll attach patches. But doing an async save on quit-application-granted seems to work.
(In reply to Milan Sreckovic [:milan] from comment #10) > (In reply to Milan Sreckovic [:milan] from comment #9) > > Right - doing that, but I wasn't "allowed" (asserts) to create a shutdown > > blocker during this profile-before-change observer call; it seems that's not > > allowed at that point in the shutdown procedure. Should be OK without it at > > that point though, I'll attach patches. > > But doing an async save on quit-application-granted seems to work. Or async save at profile-before-change time does work after all. I did run into assertions at one point, but enough code has changed in bug 789945 patches that it seems to be OK now, with an async save (shutdown blocker approach) save at that time. Stay tuned.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.