284086 - "Sanitize on shutdown" fails if the last closed window is not a browser window

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[PikeUK]

Sanitize on shutdown doesn't work unless the last window to be closed is a
browser window.

Steps to reproduce:
1. Goto Tools->Options->Privacy->Sanitize Settings.
2. Check "Sanitize Firefox on shutdown" and one of the choices, e.g. history.
3. Open the JavaScript Console.
4. Close all other Firefox windows.
5. Close the JS console.
6. Start Firefox.

Actual Results
The chosen sanitize choices have not been cleared.

Expected Results
The chosen sanitize choices should be empty.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050228 Firefox/1.0+

Comment 1

[u88484]

Confirming 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050228
Firefox/1.0+

OS=All please

Comment 2

[Ian]

Had download panel open - did not sanitize when closed main window and still did
not when download panel was closed.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050305 Firefox/1.0+

Comment 3

[RNicoletto]

Confirmed

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050527
Firefox/1.0+ 

Comment 4

[Andrew Line]

Confirming

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050610
Firefox/1.0+

Comment 5

[Asa Dotzler [:asa]]

Ben, I think we need something here. Extensions adding windows will probably
make it worse.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

*** Bug 299080 has been marked as a duplicate of this bug. ***

Comment 7

[Jo Hermans]

*** Bug 301196 has been marked as a duplicate of this bug. ***

Comment 8

[Giorgio Maone [:ma1]]

As discussed with mconnor in IRC, I'm developing a quick and dirty nsBrowserGlue
service to perform window-independent tasks (such as observing shutdown
notifications) which need to be fulfilled by an object surviving browser windows.
This artifact will initially contain only generic (almost empty) profile
startup/profile shutdown hooks to perform sanitization and fix this bug, but
could be used to refactor other toolkit code which is not window-dependant, in
order to reduce the overhead which happens whenever a new browser window is open
because of the many scripts which are included and initialized each time.

Comment 9

[Giorgio Maone [:ma1]]

Attachment: Archivecontaining the new files

Archive content:

/browser/components/nsIBrowserGlue.idl
/browser/components/nsBrowserGlue.js

Comment 10

[Giorgio Maone [:ma1]]

Attachment: Patch to various bits touched by sanitization

Comment 11

[Giorgio Maone [:ma1]]

Attachment: /dev/null diff for new files

Per WeirdAl IRC suggestion, since I've no CVS write access, the same as
attachment #192558 [details] content, as a diff :)

Comment 12

[Giorgio Maone [:ma1]]

CCing bs per comment https://bugzilla.mozilla.org/show_bug.cgi?id=285064#c42,
since attachment #192559 [details] [diff] [review] (which incidentally fixes a couple of regressions
related to bug #285064) may be l10n sensitive.

Comment 13

[Giorgio Maone [:ma1]]

Attachment: V2 Patch to various bits touched by sanitization

Same as attachment 192559 [details] [diff] [review] + code style + safer behaviour fixes

Comment 14

[Giorgio Maone [:ma1]]

Attachment: V2 /dev/null diff for new files

Comment 15

[Giorgio Maone [:ma1]]

Attachment: V2 new files

Revised indentation

Comment 16

[Giorgio Maone [:ma1]]

Attachment: V3 all in one patch

Single patch, as suggested by gavin.

#firefox - [16:07] Tonglebeak: mconnor's gonna come after you with this
pump-action 12gauge

brrrrr

Comment 17

[Giorgio Maone [:ma1]]

Attachment: V4 all in one patch

Same as 192643, but without an out-of-context content.xul patch which had been
accidentally included...

Comment 18

[Giorgio Maone [:ma1]]

Attachment: Same as V4 but better (?) style

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 192747 [details] [diff] [review]
Same as V4 but better (?) style

+    document.getElementById("sanitizeItem").setAttribute("label",
+      gPrefService.getBoolPref(this.promptDomain)
+	 ? gNavigatorBundle.getString("sanitizeWithPromptLabel")
+	 : this._defaultLabel);

convention/readability makes me prefer the following:

var label = gPrefService.getBoolPref(this.promptDomain) ? 
	    gNavigatorBundle.getString("sanitizeWithPromptLabel") :
	    this._defaultLabel);
document.getElementById("sanitizeItem").setAttribute("label", label);

+	 } catch(er) {
+	   (errors ? errors : errors = {})[itemName] = er;

again, convention type stuff:

if (!errors)
  errors = {};

errors[itemName] = er;


+	   (cacheDir = cacheDir.parent).append("Cache");
+	   try {
+	    cacheDir.remove(true);
+	   } catch(er) {}

this is valid syntax, but not so obvious and also gives a js warning (just like
flashgot...). 
Please break it onto two lines for readability/warning fixes

+	 }
+	 
+	 // The "nice" way
+	 cacheService.evictEntries(ci.nsICache.STORE_ON_DISK);
+	 cacheService.evictEntries(ci.nsICache.STORE_IN_MEMORY);
+	 
       },


+// "Static" members
+Sanitizer.PREF_DOMAIN		= "privacy.sanitize.";
+Sanitizer.PREF_PROMPT		= "promptOnSanitize";
+Sanitizer.PREF_SHUTDOWN	= "sanitizeOnShutdown";
+Sanitizer.PREF_DID_SHUTDOWN	= "didShutdownSanitize";

convention is interCaps, so prefDomain etc.

+// Shows sanitization UI
+Sanitizer.showUI = function(aParentWindow) 
+{
+  var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
+	    .getService(Components.interfaces.nsIWindowWatcher);

nit: alignment


+Sanitizer.sanitize = function(aParentWindow) 
+{
+  if (Sanitizer.prefs.getBoolPref(Sanitizer.PREF_PROMPT)) {
+    Sanitizer.showUI(aParentWindow);
+    return null;
+  } else {
+    return new Sanitizer().sanitize();
+  }
+};

shaver would beat me soundly if I let you do else after return.  

+Sanitizer.onStartup = function() 
+{
+  // we check for unclean exit with pending sanitization
+  Sanitizer._checkAndSanitize();
+};
+
+Sanitizer.onShutdown = function() 
+{
+  // we check if sanitization is needed and perform it
+  Sanitizer._checkAndSanitize();
+};
+
+// this is called on startup and shutdown, to perform pending sanitizations
+Sanitizer._checkAndSanitize = function() 
+{
+  const prefs = Sanitizer.prefs;
+  if (prefs.getBoolPref(Sanitizer.PREF_SHUTDOWN) && 
+  !prefs.prefHasUserValue(Sanitizer.PREF_DID_SHUTDOWN)) {

nit: alignment

+    // this is a shutdown or a startup after an unclean exit
+    Sanitizer.sanitize(null) || // sanitize() returns null on full success
+      prefs.setBoolPref(Sanitizer.PREF_DID_SHUTDOWN, true);

we don't use this style much, but I'll let it pass

+// Constructor
+
+function BrowserGlue() {
+  this._init();
+}
+
+BrowserGlue.prototype = {
+  QueryInterface: function(iid) 
+  {
+     xpcom_checkInterfaces(iid, SERVICE_IIDS,
Components.results.NS_ERROR_NO_INTERFACE);
+     return this;
+  }
+,
+  // nsIObserver implementation 
+  observe: function(subject, topic, data) 
+  {
+    switch(topic) {
+      case "xpcom-shutdown":
+	 this._dispose();
+	 break;
+      case "profile-before-change": 
+	 this._onProfileShutdown();
+	 break;
+      case "profile-after-change":
+	 this._onProfileStartup();
+	 break;
+    }
+  }
+, 
+  // initialization (called on application startup) 
+  _init: function() 
+  {
+    // observer registration
+    const osvr = Components.classes['@mozilla.org/observer-service;1']
+			   
.getService(Components.interfaces.nsIObserverService);
+    osvr.addObserver(this, "profile-before-change", false);
+    osvr.addObserver(this, "xpcom-shutdown", false);
+    osvr.addObserver(this, "profile-after-change", false);
+  },
+
+  // cleanup (called on application shutdown)
+  _dispose: function() 
+  {
+    // observer removal 
+    const osvr=Components.classes['@mozilla.org/observer-service;1']
+			 
.getService(Components.interfaces.nsIObserverService);

spacing around the operator here, like in init()

+    osvr.removeObserver(this, "profile-before-change");
+    osvr.removeObserver(this, "xpcom-shutdown");
+    osvr.removeObserver(this, "profile-after-change");
+  },
+
+  // profile startup handler (contains profile initialization routines)
+  _onProfileStartup: function() {
+   this.Sanitizer.onStartup();
+  },
+
+  // profile shutdown handler (contains profile cleanup routines)
+  _onProfileShutdown: function() {
+   this.Sanitizer.onShutdown();
+  },

two-space indentation, please, and brackets on the new line

+  // returns the (cached) Sanitizer constructor
+  get Sanitizer() {

nit: bracket on a new line here

+    if(typeof(Sanitizer)!="function") { // we should dinamically load the
script

if (typeof(Sanitizer) != "function")
spelling: dynamically


+// XPCOM Scaffolding code

all of these vars need to be interCaps, we only use this style in IDL
constants.
technically, you should use interCaps prefixed with k, but that's rare outside
C++

+function xpcom_checkInterfaces(iid, iids, ex) {
+  for (var j = iids.length; j-- >0;) {
+    if (iid.equals(iids[j])) return true;
+  }
+  throw ex;
+}

interCaps for the function name too.

+// Module
+
+var Module = {
+  firstTime: true,

I'd prefer "registered = false" and set to true.

+  registerSelf: function(compMgr, fileSpec, location, type) 
+  {
+    if (this.firstTime) {
+      compMgr.QueryInterface(Components.interfaces.nsIComponentRegistrar)
+	      .registerFactoryLocation(
+	       SERVICE_CID,
+	       SERVICE_NAME,
+	       SERVICE_CTRID, 
+	       fileSpec,
+	       location, 
+	       type);

this is really confusing alignment, the args should align to the ( for
registerFactoryLocation

+      const catman = Components.classes['@mozilla.org/categorymanager;1']
+			       
.getService(Components.interfaces.nsICategoryManager);
+      for (var j=0, len=SERVICE_CATS.length; j<len; j++) {

spaces around operators (j - 0, j < len)

nit: alignment

overall, I like the approach, once I can read the code! ;)

Comment 20

[Giorgio Maone [:ma1]]

Attachment: Same as V5, edited per review

Accepted changes suggested by mconnor & gavin

Comment 21

[Giorgio Maone [:ma1]]

Attachment: Hopefully ultimate nit-fixer :)

To be landed as discussed in IRC (mconnor, gavin)

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Trunk:

base/content/browser-sets.inc;                        1.52;
base/content/browser.js;                              1.489;
base/content/global-scripts.inc;                      1.3;
base/content/sanitize.js;                             1.6;
base/content/sanitize.xul;                            1.11;
components/Makefile.in;                               1.42;
components/nsBrowserGlue.js;                          1.1;
components/nsIBrowserGlue.idl;                        1.1;
locales/en-US/chrome/browser/browser.dtd;             1.26;
locales/en-US/chrome/browser/preferences/privacy.dtd; 1.8;

Comment 23

[Giorgio Maone [:ma1]]

Comment on attachment 193571 [details] [diff] [review]
Hopefully ultimate nit-fixer :)

following with a diff from MOZILLA_1_8_BRANCH for approval

Comment 24

[Giorgio Maone [:ma1]]

Attachment: Fix for MOZILLA_1_8_BRANCH

Land me on MOZILLA_1_8_BRANCH

Comment 25

[Benjamin Smedberg]

This introduces a late-l10n change and should not have been landed this way on
the trunk. When we change a l10n string (in this case sanitizeCmd.label) to
change the meaning or formatting, we must rename the key so that the l10n teams
and the tinderboxes are aware of the change. If this lands on the branch we must
rename the key.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: change entity name

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Attachment 193595 [details] [diff] (change entity name):

browser/base/content/browser-menubar.inc; new revision: 1.59;
browser/locales/en-US/chrome/browser/browser.dtd; new revision: 1.27;

Comment 28

[Giorgio Maone [:ma1]]

Attachment: All-in-one patch for Moz1.8 branch (bug fix + dtd change + installer additions)

Backport for branch, including all the gavin trunk tweakages: browser.dtd,
installer/(windows|linux)/packages-static.

Comment 29

[Asaf Romano (gone)]

1.8 Branch:

Checking in base/content/browser-sets.inc;
/cvsroot/mozilla/browser/base/content/browser-sets.inc,v  <--  browser-sets.inc
new revision: 1.51.2.1; previous revision: 1.51
done
Checking in base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.16; previous revision: 1.479.2.15
done
Checking in base/content/global-scripts.inc;
/cvsroot/mozilla/browser/base/content/global-scripts.inc,v  <--  global-scripts.inc
new revision: 1.2.4.1; previous revision: 1.2
done
Checking in base/content/sanitize.js;
/cvsroot/mozilla/browser/base/content/sanitize.js,v  <--  sanitize.js
new revision: 1.5.2.1; previous revision: 1.5
done
Checking in base/content/sanitize.xul;
/cvsroot/mozilla/browser/base/content/sanitize.xul,v  <--  sanitize.xul
new revision: 1.10.2.1; previous revision: 1.10
done
Checking in components/Makefile.in;
/cvsroot/mozilla/browser/components/Makefile.in,v  <--  Makefile.in
new revision: 1.41.2.1; previous revision: 1.41
done
Checking in components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.4.2.2; previous revision: 1.4.2.1
done
Checking in components/nsIBrowserGlue.idl;
/cvsroot/mozilla/browser/components/nsIBrowserGlue.idl,v  <--  nsIBrowserGlue.idl
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in installer/unix/packages-static;
/cvsroot/mozilla/browser/installer/unix/packages-static,v  <--  packages-static
new revision: 1.50.2.5; previous revision: 1.50.2.4
done
Checking in installer/windows/packages-static;
/cvsroot/mozilla/browser/installer/windows/packages-static,v  <--  packages-static
new revision: 1.53.2.4; previous revision: 1.53.2.3
done
Checking in locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <-- 
browser.dtd
new revision: 1.25.2.1; previous revision: 1.25
done
Checking in locales/en-US/chrome/browser/preferences/privacy.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/privacy.dtd,v
 <--  privacy.dtd
new revision: 1.7.2.1; previous revision: 1.7
done

Comment 30

[Asaf Romano (gone)]

Comment on attachment 193717 [details] [diff] [review]
All-in-one patch for Moz1.8 branch (bug fix + dtd change + installer additions)

er, you haven't fixed browser-menubar.inc in this patch.

Comment 31

[Peter van der Woude [:Peter6]]

Hmm, this is not fixed at all
Sanitize on shutdown has turned into Sanitize on Start !
It clears the data when you restart your browser not when you close it.

Comment 32

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050826
Firefox/1.0+ ID:2005082612

Clear Privacy Data settings in Options

[V] Browsing History
[V] Saved Form Information
[ ] Saved Passwords
[V] Download History
[ ] Cookies
[V] Cache
[V] Authenticated Sessions

[V] Clear Privacy data when closing Deer Park
[V] Ask me before clearing private data

repro:
1. Make Clear Privacy Data settings as above
2. Open a bunch of tabs with pages (fill the cache)
3. Open a tab and look at disk cache size with about:cache
4. Close FF
5. Clear privacy data dialog does NOT pop up

6. Open FF
7. Clear privacy data dialog does pop up
8. Press Cancel (important)
9. open about:cache in a tab and notice nothing was cleared on shutdown (same
cache size)

Comment 33

[Mike]

(In reply to comment #32)

Confirming Peter's report using
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050826
Firefox/1.0+ ID:2005082608

In addition if 

> [V] Ask me before clearing private data

is NOT marked then NO sanitize operations occur, either on shutdown or restart.

Comment 34

[Peter van der Woude [:Peter6]]

The problem is caused by Adblock Plus 0.5.9.1
Giorgio Maone will have a look at it tomorrow
closing Resolved/Fixed - fixed1.8

Comment 35

[Mike]

(In reply to comment #34)
> The problem is caused by Adblock Plus 0.5.9.1
> Giorgio Maone will have a look at it tomorrow
> closing Resolved/Fixed - fixed1.8
> 

Uninstalled Adblock. Now the sanitize process "runs" at shutdown but NO cache
files actually are removed. IOW, still broken here.

Comment 36

[Peter van der Woude [:Peter6]]

(In reply to comment #35)
 Now the sanitize process "runs" at shutdown but NO cache files actually are
removed. IOW, still broken here.
Bug 265028 ?

Comment 37

[Mike]

(In reply to comment #36)
> (In reply to comment #35)
>  Now the sanitize process "runs" at shutdown but NO cache files actually are
> removed. IOW, still broken here.
> Bug 265028 ?
> 
> 

I don't think so. A manual "Clear Cache Now" always works here.

Comment 38

[Peter van der Woude [:Peter6]]

Adblock Plus was fixed (new version 0.9.5.2) and all is fine now

Comment 39

[Marian POPESCU]

Attachment: Sanitize window has lost 3D aspect when shutting down browser

Probably linked with this patch is the following regression: when shutting down
Firefox, the sanitize window shows up but it has lost the 3D aspect for its
controls (it's "flat" and controls are border-less).
Starting up Firefox brings again the sanitize window, this time with normal 3D
aspect.
In console I get a lot of output like:
(Gecko:21043): Gdk-CRITICAL **: gdk_draw_rectangle: assertion 'GDK_IS_GC (gc)'
failed
(Gecko:21043): Gdk-CRITICAL **: gdk_gc_set_fill: assertion 'GDK_IS_GC (gc)'
failed
(Gecko:21043): Gdk-CRITICAL **: gdk_gc_set_clip_rectangle: assertion 'GDK_IS_GC
(gc)' failed
(Gecko:21043): Gdk-CRITICAL **: gdk_gc_set_ts_origin: assertion 'GDK_IS_GC
(gc)' failed
(Gecko:21043): Gdk-CRITICAL **: gdk_gc_set_stipple: assertion 'GDK_IS_GC (gc)'
failed
(Gecko:21043): Gdk-CRITICAL **: gdk_draw_line: assertion 'gc != NULL' failed

When closing sanitize dialog, the last two lines are:

(Gecko:21043): Gdk-CRITICAL **: gtk_widget_destroy: assertion 'GTK_IS_WIDGET
(widget)' failed
(Gecko:21043): Gdk-CRITICAL **: gtk_main_quit: assertion 'main_loops != NULL'
failed

Comment 40

[Marian POPESCU]

I forgot to mention that I use
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 -
Build ID: 2005090802

Comment 41

[Marian POPESCU]

This is the output just before the assertions :

*** e = [Exception... "Component returned failure code: 0x80570016
(NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016
(NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame ::
chrome://browser/content/utilityOverlay.js :: getShellService :: line 329" 
data: no]

(Gecko:13314): GLib-GObject-WARNING **: invalid unclassed pointer in cast to
`GtkContainer'

(Gecko:13314): Gtk-CRITICAL **: gtk_container_add: assertion `GTK_IS_CONTAINER
(container)' failed

(Gecko:13314): Gtk-CRITICAL **: gtk_widget_realize: assertion
`GTK_WIDGET_ANCHORED (widget) || GTK_IS_INVISIBLE (widget)' failed

(Gecko:13314): GLib-GObject-WARNING **: invalid unclassed pointer in cast to
`GtkContainer'

(Gecko:13314): Gtk-CRITICAL **: gtk_container_add: assertion `GTK_IS_CONTAINER
(container)' failed

(Gecko:13314): Gtk-CRITICAL **: gtk_widget_realize: assertion
`GTK_WIDGET_ANCHORED (widget) || GTK_IS_INVISIBLE (widget)' failed

Comment 42

[Ben Goodger (use ben at mozilla dot org for email)]

Why did this patch introduce a setTimeout in the SanitizeListener constructor? The SanitizeListener is already constructed in delayedStartup, so it's not blocking the browser window load. How long does it really take to flush preferences that it needs to be delayed by a second? If it takes a long time (and I'm not convinced that it does), won't it be more frustrating to the user that the browser window appears and becomes usable, then mysteriously locks up as they begin typing? 

setTimeout is _evil_. Randomly executing code later because you _think_ it might be slow is not optimization, and what it does its it causes a lot of sequencing bugs people like bryner end up having to spend hours and days figuring out. 

Comment 43

[Giorgio Maone [:ma1]]

Attachment: No setTimeout() to flush preferences [checked in]

(In reply to comment #42)

> Why did this patch introduce a setTimeout in the SanitizeListener constructor?
> The SanitizeListener is already constructed in delayedStartup
> [...] setTimeout is _evil_

Good point, Mr. Goodger.
I just didn't notice that SanitizerListener is already constructed inside its own (good? ;-) ) setTimeout().
This patch fixes my evil code.
Peace.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 204249 [details] [diff] [review]
No setTimeout() to flush preferences [checked in]

Checked in at Giorgio's request.
mozilla/browser/base/content/browser.js; new revision: 1.533;

Comment 45

[Ben Goodger (use ben at mozilla dot org for email)]

Thanks for the response, guys! :)

Comment 46

[Boris Zbarsky [:bzbarsky]]

Note that this patch caused bug 321833

Comment 47

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #43)
> Created an attachment (id=204249) [edit]
> No setTimeout() to flush preferences (MOZILLA_1_8_BRANCH)

Checked this in on the 1.8 branch.
mozilla/browser/base/content/browser.js; new revision: 1.479.2.63;

Comment 48

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change