Sanitize on startup prefs are broken

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 unaffected, firefox45- wontfix, firefox46+ fixed, firefox47+ fixed)

Details

Attachments

(2 attachments)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
[Tracking Requested - why for this release]: We always sanitize on startup if a user has sanitize on shutdown set.
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.
Attachment #8721511 - Flags: review?(dteller) → review+
Assignee

Comment 4

3 years ago
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.
Assignee

Comment 5

3 years ago
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/
Assignee

Comment 6

3 years ago
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/
Assignee

Updated

3 years ago
Blocks: 1250424

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/194f52b4d779
https://hg.mozilla.org/mozilla-central/rev/135340a254f4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Unfortunately, this is too late for 45.
If you think it should be in 45.1.0, please let me know.
Assignee

Comment 11

3 years ago
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.
Assignee

Comment 12

3 years ago
Posted patch Aurora patchSplinter Review
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
Attachment #8723844 - Flags: approval-mozilla-aurora?
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.
Attachment #8723844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

3 years ago
[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
You need to log in before you can comment on or make changes to this bug.