1249786 - Sanitize on startup prefs are broken

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

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

Comment 2

[Marco Bonardo [:mak]]

Attachment: MozReview Request: Bug 1249786 - Sanitize on startup prefs are broken. r=yoric

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

Comment 3

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

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.

Comment 4

[Marco Bonardo [:mak]]

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 5

[Marco Bonardo [:mak]]

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 6

[Marco Bonardo [:mak]]

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/

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/194f52b4d779

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/135340a254f4

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

Attachment: Aurora patch

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

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Regression from 44, tracking.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8723844 [details] [diff] [review]
Aurora patch

Fix for regression that causes some startup hangs. Please uplift to aurora.

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4bf362ade78

Comment 16

[Mayur Patil]

[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