Once we complete a sanitize on shutdown we set privacy.sanitize.didShutdownSanitize to true We never unset it, and on startup we do: + if (Preferences.has(Sanitizer.PREF_SANITIZE_DID_SHUTDOWN)) { + // Firefox crashed before having a chance to sanitize during shutdown. + // (note that if Firefox crashed during shutdown sanitization, we + // will hit both `if` so we will run a second double-sanitization). + yield Sanitizer.onShutdown(); + } this is quite wrong, since we will always have that pref set! didShutdownSanitize indicates that a shutdown sanitize succeeded sanitizeInProgress indicates that a sanitize failed so if we don't have didShutdownSanitize, but we have sanitize on shutdown, means we were interrupted and also sanitizeInProgress will be set. if sanitizeInProgress is set, we were interrupted, but it may have not been a shutdown sanitize. In practice when a sanitization fails sanitizeInProgress is always set, didShutdownSanitize along with sanitizeOnShutdown tells us it was a shutdown sanitization. in both cases we should sanitize on startup. Ideally didShutdownSanitize is no more needed with sanitizeInProgress, but we will keep it for add-ons compat.

[Tracking Requested - why for this release]: We always sanitize on startup if a user has sanitize on shutdown set.

Review commit: https://reviewboard.mozilla.org/r/35691/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35691/

Comment on attachment 8721511 [details] MozReview Request: Bug 1249786 - Sanitize on startup prefs are broken. r=yoric https://reviewboard.mozilla.org/r/35691/#review32339 Patch looks good, thanks. Do you think that it will also have any effect on shutdown, or "just" on the million startup issues we encounter? ::: browser/base/content/sanitize.js:768 (Diff revision 1) > + let failedShutownSanitization = Preferences.get(Sanitizer.PREF_SANITIZE_ON_SHUTDOWN, false) && Since you're fixing the protocol around `PREF_SANITIZE_ON_SHUTDOWN`/`PREF_SANITIZE_DID_SHUTDOWN`, could you take the opportunity to document it around the definition of these constants? Also, what does _fail_ mean? Crash? Exception? Both? This deserves a clarification. ::: browser/base/content/sanitize.js:772 (Diff revision 1) > + Services.prefs.savePrefFile(null); Is that really necessary? ::: browser/base/content/sanitize.js:809 (Diff revision 1) > - if (Preferences.has(Sanitizer.PREF_SANITIZE_IN_PROGRESS)) { > + let inProgressSanitization = Preferences.get(Sanitizer.PREF_SANITIZE_IN_PROGRESS, ""); `inProgressSanitization` is a weird name for this. Maybe `sanitizationInterrupted`, or something such? ::: browser/base/content/sanitize.js:813 (Diff revision 1) > + if (failedShutownSanitization && !inProgressSanitization) { The flow is not entirely clear here. Do I understand correctly that this means the following? ``` if (...) { // 1. We crashed during sanitization, but are not sure why. } else if (...) { // 2. We crashed during sanitization, and we know exactly what's left to do. } /* else { // 3. Sanitization succeeded. } */ ``` If so, `2.` is essentially an optimization on `1.`, or is there another reason, e.g. avoiding startup race conditions? Clarifications (as comments) would be useful. ::: browser/base/content/sanitize.js:817 (Diff revision 1) > - let json = Preferences.get(Sanitizer.PREF_SANITIZE_IN_PROGRESS); > + let itemsToClear = JSON.parse(inProgressSanitization); Perhaps we should defend a little against ill-formed JSON? ::: browser/base/content/sanitize.js:833 (Diff revision 1) > + Services.prefs.savePrefFile(null); Ok, this one is definitely useful.

https://reviewboard.mozilla.org/r/35691/#review32339 It should reduce the number of pointless clear history on startup we are currently doing, that should also reduce the number of startup hangs. It won't have an effect on shutdown aborts. > Is that really necessary? if we crash before prefs are stored, we will sanitize again. If it's the sanitization causing the crash, the browser may become unusable. I added a comment. > The flow is not entirely clear here. > > Do I understand correctly that this means the following? > > ``` > if (...) { > // 1. We crashed during sanitization, but are not sure why. > } else if (...) { > // 2. We crashed during sanitization, and we know exactly what's left to do. > } /* else { > // 3. Sanitization succeeded. > } */ > ``` > > If so, `2.` is essentially an optimization on `1.`, or is there another reason, e.g. avoiding startup race conditions? > > Clarifications (as comments) would be useful. The if/else if is only handling backwards compatibility. In some cases, like when the prefs are set by add-ons, or for previous versions, we may not have PREF_SANITIZE_IN_PROGRESS, but we know shutdown sanitization didn't go well cause didShutdownSanitize is not set. In such a case even we don't have the list of sanitized items, we can still do a shutdown sanitize. I modified the if/else to make that clearer. > Perhaps we should defend a little against ill-formed JSON? well, JSON.parse will just throw and the task will reject, we won't try to sanitize. Any other kind of handling could easily fail. I just added a brief comment about the fact json.parse could throw.

Comment on attachment 8721511 [details] MozReview Request: Bug 1249786 - Sanitize on startup prefs are broken. r=yoric Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35691/diff/1-2/

Comment on attachment 8721511 [details] MozReview Request: Bug 1249786 - Sanitize on startup prefs are broken. r=yoric Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35691/diff/2-3/

https://hg.mozilla.org/mozilla-central/rev/194f52b4d779 https://hg.mozilla.org/mozilla-central/rev/135340a254f4

Unfortunately, this is too late for 45. If you think it should be in 45.1.0, please let me know.

we will be fine with uplifting in 46, I backed out all the other stuff in 45 so no reason to uplift this. I'll post a patch including the follow-up fix for bug 1250362, for the uplift.

Approval Request Comment [Feature/regressing bug #]: bug 1089695 [User impact if declined]: We sanitize on startup when we should not, or don't when instead we should. Also some add-ons setting these prefs are not behaving properly. [Describe test coverage new/current, TreeHerder]: nightly, local [Risks and why]: low risk, this is restoring the behavior in a similar way as it was before. Also, more comments than code. [String/UUID change made/needed]: none

Regression from 44, tracking.

Comment on attachment 8723844 [details] [diff] [review] Aurora patch Fix for regression that causes some startup hangs. Please uplift to aurora.

[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected