Closed Bug 1434414 Opened 3 years ago Closed 3 years ago

Locking the "sanitize on shutdown" pref causes sanitization to happen at every startup, breaking pages opened through the command line

Categories

(Firefox :: General, defect, P1)

58 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: garycheng12, Assigned: mak)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20100101

Steps to reproduce:

The setting `lockPref("privacy.sanitize.sanitizeOnShutdown", true);` in my mozilla.cfg is used (this is the only setting). Launch Firefox in the command line, opening the Twitter homepage: `firefox twitter.com`.


Actual results:

Firefox is launched, but the Twitter homepage is not opened.


Expected results:

The Twitter homepage should have opened.

The culprit is confirmed by me and several people on Reddit to be the setting `privacy.sanitize.sanitizeOnShutdown`--when it's set to true, many (but not all, apparently) URLs cannot be opened if they are specified as arguments to Firefox when launching it from the command line. Simply setting it to false or commenting it out will restore the expected behavior.
Component: Untriaged → Private Browsing
I suspect the startup code in https://searchfox.org/mozilla-central/source/browser/base/content/sanitize.js#854-899 is the cause. In particular, setting the preference before starting the browser will cause the browser to think that it was interrupted while sanitizing during shutdown, so it will execute a sanitization immediately. As part of this we sanitize open windows, which closes all browser windows.
As a workaround, you could set privacy.sanitize.didShutdownSanitize to true and see if comment 1 is correct.
Component: Private Browsing → General
Marco, I think you were the last person to touch that code. Could you chime in?
Flags: needinfo?(mak77)
Josh is right, if you set privacy.sanitize.sanitizeOnShutdown but don't set privacy.sanitize.didShutdownSanitize, we end up thinking your last shutdown sanitization was interrupted, and try it again on startup.
On the other side, setting privacy.sanitize.didShutdownSanitize also means that if the previous sanitization was really interrupted, we won't recover it and risk a privacy leak.

Off-hand, looks like we could just use PREF_SANITIZE_IN_PROGRESS for any case in which the sanitization was interrupted, and just issue a new one with its value on startup if it's set. It should allow to completely remove PREF_SANITIZE_DID_SHUTDOWN and avoid checking PREF_SANITIZE_ON_SHUTDOWN on startup.

The only problem is that I'd not want to bitrot Johann's work in bug 1167238, since it's a pretty large patch. that also means uplifting to ESR52 could require a separate effort. The good thing is that 60 will be the next ESR, so we should try fixing this for 60.

(we REALLY NEED a component for sanitization and the clear history dialog)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Flags: needinfo?(mak77)
Summary: Certain URLs as arguments do not work → Locking the "sanitize on shutdown" pref causes sanitization to happen at every startup, breaking pages opened through the command line
adding [fxsearch] to track this "somewhere", I'm clearly happy for anyone to look into it in case we didn't yet.
Whiteboard: [fxsearch]
I'll look if I can simplify this code path.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #8951276 - Flags: review?(jhofmann)
Comment on attachment 8951276 [details]
Bug 1434414 - Locking the 'sanitize on shutdown' pref causes sanitization to happen at every startup.

https://reviewboard.mozilla.org/r/220530/#review227384

Sorry for the delay, this looks good in general, just a couple of remarks/questions.

::: browser/modules/Sanitizer.jsm:48
(Diff revision 1)
>  
>    /**
> -   * During a sanitization this is set to a json containing the array of items
> -   * being sanitized, then cleared once the sanitization is complete.
> -   * This allows to retry a sanitization on startup in case it was interrupted
> -   * by a crash.
> +   * During a sanitization this is set to a json containing an array of the
> +   * pending sanitizations. This allows to retry sanitizations on startup in case
> +   * they were interrupted by a crash.
> +   * Use addPendingSanitization and removePendingSanitization to manage it.

I'm not sure whether this needs to be documented here, given that those two functions are private to the module.

::: browser/modules/Sanitizer.jsm:122
(Diff revision 1)
>  
> -    // Check if Firefox crashed during a sanitization.
> -    let lastInterruptedSanitization = Services.prefs.getStringPref(Sanitizer.PREF_SANITIZE_IN_PROGRESS, "");
> -    if (lastInterruptedSanitization) {
> -      // If the json is invalid this will just throw and reject the Task.
> -      let {itemsToClear, options} = JSON.parse(lastInterruptedSanitization);
> +    // Check if Firefox crashed before completing a sanitization.
> +    for (let {id, itemsToClear, options} of interruptedSanitizations) {
> +      if (id == "shutdown") {
> +        // Rebuild the list of items to clear, because the stored value may be
> +        // stale if the user modified the prefs after we stored it.

I'm not sure I understand. The scenario is:

- The user selects clear on shutdown
- We set the pending sanitize task
- The user closes their browser
- Clear on shutdown fails
- The user starts up their browser
- We resume clear on shutdown

At what point does the user modify their prefs?

::: browser/modules/Sanitizer.jsm:129
(Diff revision 1)
> +        options = {};
> +      }
> +      try {
> -      await this.sanitize(itemsToClear, options);
> +        await this.sanitize(itemsToClear, options);
> -    } else if (shutdownSanitizationWasInterrupted) {
> -      // Otherwise, could be we were supposed to sanitize on shutdown but we
> +      } catch (ex) {
> +        Cu.reportError("A previously interrupted sanitization failed: " + itemsToClear);

Nit: is it worth adding the exception to this message?

::: browser/modules/Sanitizer.jsm:744
(Diff revision 1)
> -  let { prefDomain = "privacy.cpd.", ignoreTimespan = true, range } = options;
> -  let seenError = false;
> -  let itemsToClear;
> -  if (Array.isArray(aItemsToClear)) {
> -    // Shallow copy the array, as we are going to modify
> -    // it in place later.
> + * Whether we should sanitize on shutdown.
> + * When this is set, a pending sanitization is also added and removed when
> + * shutdown sanitization is complete. This allows to retry a failed sanitization
> + * on startup.
> + */
> +XPCOMUtils.defineLazyPreferenceGetter(this, "shouldSanitizeOnShutdown",

Please either put this on the Sanitizer global or prefix it with "g". It's a little confusing reading this variable name in the startup and shutdown functions.

::: browser/modules/Sanitizer.jsm:749
(Diff revision 1)
> -    // it in place later.
> -    itemsToClear = [...aItemsToClear];
> -  } else {
> -    let branch = Services.prefs.getBranch(prefDomain);
> -    itemsToClear = Object.keys(items).filter(itemName => {
> -      try {
> +XPCOMUtils.defineLazyPreferenceGetter(this, "shouldSanitizeOnShutdown",
> +  Sanitizer.PREF_SANITIZE_ON_SHUTDOWN, false,
> +  (_, oldVal, newVal) => {
> +    if (!oldVal && newVal) {
> +      let itemsToClear = getItemsToClearFromPrefBranch(Sanitizer.PREF_SHUTDOWN_BRANCH);
> +      addPendingSanitization("shutdown", itemsToClear, {});

You're relying on the lazy getter to get triggered in Sanitizer.onStartup() here, otherwise the update function will not run. Don't you think that's a little underhanded? I don't think many people consider that moving a variable around might mess with adding sanitization somewhere else.

I don't think it's a blocking issue, but maybe an explicit preference observer would be clearer here.

::: browser/modules/Sanitizer.jsm:767
(Diff revision 1)
> +  let itemsToClear = [...aItemsToClear];
>  
>    // Store the list of items to clear, in case we are killed before we
>    // get a chance to complete.
> -  Services.prefs.setStringPref(Sanitizer.PREF_SANITIZE_IN_PROGRESS,
> -                               JSON.stringify({itemsToClear, options}));
> +  let uid = gPendingSanitizationSerial++;
> +  let isShutdownSanitization = options.progress && options.progress.isShutdown;

Can't you shorten this to just progress.isShutdown given that progress is passed from sanitize and guaranteed to be at least {}?

::: browser/modules/Sanitizer.jsm:942
(Diff revision 1)
> + * @param id A unique id identifying the sanitization
> + * @param itemsToClear The items to clear
> + * @param options The Sanitize options
> + */
> +function addPendingSanitization(id, itemsToClear, options) {
> +  let pendingSanitizations = JSON.parse(

Since we're parsing JSON from a pref it might be safer to wrap this with a try..catch and restore the pref to something sane in case it fails?
Attachment #8951276 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #8)
> I'm not sure whether this needs to be documented here, given that those two
> functions are private to the module.

I want to document the interruption behavior at the top of the module, because it's a bit exotic and I also don't want people to manually mess with the pref trying to guess how it works.

> ::: browser/modules/Sanitizer.jsm:122
> > +        // Rebuild the list of items to clear, because the stored value may be
> > +        // stale if the user modified the prefs after we stored it.
> 
> I'm not sure I understand. The scenario is:

Basically, clear history on shutdown means the sanitization is always pending. We set it pending on startup. That way if we crash BEFORE shutdown, we can sanitize at the next startup.
Since we set the pending pref at startup, and we don't (currently) listen to pref changes this is what may happen:
- we start and notice CHS is set, so we set a pending sanitization
- the user opens prefs and changes the items that should be finalized on shutdown 
- the user closes the browser, at this point the pending sanitization we set is stale
We could alternatively set a pref observer on the shutdown branch, but that sounds excessive when we can just read the pref.

I can probably clarify the comment.
Btw, if I make Sanitizer a pref observer, I could also at that point observe the shutdown branch and remove the "stale" thing. let's see.
I made the prefs managemend far more explicit, it's a little bit more code but it should be clear.

I also removed a reference to sanitize.js from sanitize.xul, that was printing errors to the console when launching the CRH dialog, since sanitize.js doesn't exist anymore.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/56ebb683fa91
Locking the 'sanitize on shutdown' pref causes sanitization to happen at every startup. r=johannh
https://hg.mozilla.org/mozilla-central/rev/56ebb683fa91
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.