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+

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

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+

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

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

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

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

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

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.

Archive content: /browser/components/nsIBrowserGlue.idl /browser/components/nsBrowserGlue.js

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

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.

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

Revised indentation

Single patch, as suggested by gavin. #firefox - [16:07] Tonglebeak: mconnor's gonna come after you with this pump-action 12gauge brrrrr

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

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! ;)

Accepted changes suggested by mconnor & gavin

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

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 on attachment 193571 [details] [diff] [review] Hopefully ultimate nit-fixer :) following with a diff from MOZILLA_1_8_BRANCH for approval

Land me on MOZILLA_1_8_BRANCH

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.

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;

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

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 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.

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.

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)

(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.

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

(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.

(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 ?

(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.

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

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

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

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

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.

(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 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;

Thanks for the response, guys! :)

Note that this patch caused bug 321833

(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;

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