416233 - Add sanitize (clear private data) option for SeaMonkey

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Robert Kaiser]

SeaMonkey should have a sanitize feature, i.e. "clear private data", just like Firefox has. In fact, we can copy their implementation to a very big part, idieally also using the approach from bug 397719. This will also fix bug 405709, and a sidenote from bug 390158.

I'm planning to use the main privacy & security pane for that, which is empty so far and makes up bad UI because of that.

Comment 1

[Robert Kaiser]

Attachment: patch v1: add sanitize

Here is a first patch. Most of this is copied from Firefox and just made to fit into SeaMonkey - we're also using the module approach that has not landed for Firefox yet, as I stumbled over the already cvs copied file for that (and it looks cleaner than the previous solution).

Comment 2

[[e-mail]]

link -NOLOGO -DLL -OUT:suitecommon_s.dll -PDB:suitecommon_s.pdb -SUBSYSTEM:WINDOWS      ./module.res   -DEBUG -DEBUGTYPE:CV         kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib
LINK : warning LNK4068: /MACHINE not specified; defaulting to X86
LINK : error LNK2001: unresolved external symbol __DllMainCRTStartup@12
suitecommon_s.dll : fatal error LNK1120: 1 unresolved externals

Comment 3

[Robert Kaiser]

Hmm, I didn't see that error on Linux - should we kill the LIBRARY_NAME as we are not creating any real library there right now?

Comment 4

[[e-mail]]

Comment on attachment 302019 [details] [diff] [review]
patch v1: add sanitize

>+pref("privacy.item.history",     true);
>+pref("privacy.item.formdata",    true);
>+pref("privacy.item.passwords",   false);
>+pref("privacy.item.downloads",   true);
>+pref("privacy.item.cookies",     false);
>+pref("privacy.item.cache",       true);
>+pref("privacy.item.siteprefs",   false);
>+pref("privacy.item.sessions",    true);
>+pref("privacy.item.offlineApps", false);
I think the unimplemented items should be false for now.

>+    <key id="key_sanitize" command="Tools:Sanitize" keycode="VK_DELETE" modifiers="accel,shift"/>
Some Macs don't have delete keys...

>+    <command id="Tools:Sanitize"
>+     oncommand="Components.classes['@mozilla.org/suite/suiteglue;1'].getService(Components.interfaces.nsISuiteGlue).sanitize(window || null);"/>
Nit: incorrect indentation (although for that long a line, a function might be worth while)

>+<!DOCTYPE prefwindow [
>+  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
>+  <!ENTITY % sanitizeDTD SYSTEM "chrome://communicator/locale/sanitize.dtd">
>+  %brandDTD;
>+  %sanitizeDTD;
>+]>
Nit: we normally alternate these lines rather than having the %; lines together.

>+            style="width: &window.width;;"
Why do we need a width? If it needs one, I'd prefer the entity to be the whole style.

>+      var gSanitizePromptDialog = {
Since this is the only pane, why not just use globals?

>+          var bundleUtil = document.getElementById("bundleUtil");
>+          document.documentElement.getButton("accept").label = bundleUtil.getString("sanitizeButton");
Can't you define this in the .DTD? (buttonlabelaccept="&sanitizeButton.label;")

>+          var preference, name;
Nit: you can declare these on first use

>+          try {
>+            document.documentElement.getButton("accept").disabled = !found;
>+          }
>+          catch (e) { }
Why the try/catch?

>+          return undefined;
Nit: don't need to explicitly return

>+    <checkbox label="&itemHistory.label;"
>+              accesskey="&itemHistory.accesskey;"
>+              preference="privacy.item.history"
>+              onsyncfrompreference="return gSanitizePromptDialog.onReadGeneric();"/>
IMHO onchange on the <preferences> element would be better.

>+  const Cc = Components.classes, Ci = Components.interfaces;
>+  var glue = Cc["@mozilla.org/suite/suiteglue;1"]
>+               .getService(Ci.nsISuiteGlue);
Not worth Cc/Ci here.

>+  glue.sanitize(window || null);
Don't need || null here. Could also inline "glue" variable.

>+sanitizeWithPromptLabel=Clear Private Data…
... (pun intended)

>-<!ENTITY pref.security.title              "Privacy &amp; Security">
>+<!ENTITY pref.security.title             "Privacy &amp; Security">
Why the change in spacing?

>+<!ENTITY clearDataNow.label              "Clear Now…">
>+<!ENTITY clearDataNow.label2             "Clear Now">
Maybe we could make them more meaningful, e.g. clearDataDialog.label and clearDataNow.label

>+LIBRARY_NAME	= suitecommon_s
Yeah, that looks wrong :-)

>+META_COMPONENT  = suite
???

>+EXTRA_PP_COMPONENTS = \
>+	nsSuiteGlue.js \
>+	$(NULL)
Hmm, our browser components aren't in suite/browser/src :-(

>+# ***** BEGIN LICENSE BLOCK *****
/* */ licence please (and don't preprocess)

>+// Factory object
>+const SuiteGlueServiceFactory = {
>+  _instance: null,
>+  createInstance: function (outer, iid) 
>+  {
>+    if (outer != null)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>+    return this._instance == null ?
>+      this._instance = new SuiteGlue() : this._instance;
>+  }
>+};
Does GenerateModule not give you a factory?

>+      case "browser:purge-session-history":
>+        // reset the console service's error buffer
Why?

>+#ifdef XP_MACOSX
>+      ww.openWindow(null, // make this an app-modal window on Mac
Why?

>+  canClearItem: function(aItemName) this.items[aItemName].canClear,
>+
>+  getNameFromPreference: function(aName) aName.substr(itemBranch.length),
Not keen on this shorthand either...

>+      get canClear() {
>+        return true;
>+      }
canClear: true

>+        // Clear last URL of the Open Web Location dialog
We also have to clear the URLbar history manually


>+        //Clear undo history of all searchBars
We don't have search bars

>+        return false; // XXX: as long as we aren't using toolkit's download manager
Don't do that, comment the code out instead

>+      // Some of these clear() may raise exceptions (see bug #265028)
I notice some of the items have try/catch... remove those and let this handle it

>+  return errors;
The function is declared as returning void...

Comment 5

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

(In reply to comment #4)
> >+      case "browser:purge-session-history":
> >+        // reset the console service's error buffer
> Why?

Because exceptions/warnings potentially reveal the sites you've been visiting.

Comment 6

[Robert Kaiser]

(In reply to comment #4)
> >+          var preference, name;
> Nit: you can declare these on first use

Their first use is inside a for loop, that's why they're declared just before that loop.

> >+sanitizeWithPromptLabel=Clear Private Data…
> ... (pun intended)

See bug 417807 ;-)

> >+EXTRA_PP_COMPONENTS = \
> >+	nsSuiteGlue.js \
> >+	$(NULL)
> Hmm, our browser components aren't in suite/browser/src :-(

I'm not sure what's better - they don't implement interfaces from the browser/public dir, do they?

> >+// Factory object
> >+const SuiteGlueServiceFactory = {
> [...]
> >+};
> Does GenerateModule not give you a factory?

Umm... I don't know? I just copied FF code...

> >+      case "browser:purge-session-history":
> >+        // reset the console service's error buffer
> Why?

See Gavin's comment.

> >+#ifdef XP_MACOSX
> >+      ww.openWindow(null, // make this an app-modal window on Mac
> Why?

Apparently because we see odd positioning otherwise, see bug 309406

> >+      // Some of these clear() may raise exceptions (see bug #265028)
> I notice some of the items have try/catch... remove those and let this handle
> it

Good idea.

> >+  return errors;
> The function is declared as returning void...

Not the Sanitizer one, just the nsSuiteGlue one. Yes, many sanitize() functions around here - unfortunately, extensions wanting to hook into this mechanism are probably ported from FF and expect that...


I addressed all other comments in local work, new patch coming soon...

Comment 7

[Robert Kaiser]

Attachment: patch v2: address review comments

Here's the updated patch, addressing all review comments that I didn't explicitely reply to in the last comment.

Comment 8

[[e-mail]]

(In reply to comment #6)
>(In reply to comment #4)
>>>+          var preference, name;
>>Nit: you can declare these on first use
>Their first use is inside a for loop
That doesn't matter, because you don't reuse them between or after the loop.

Comment 9

[[e-mail]]

(In reply to comment #4)
>>+    <key id="key_sanitize" command="Tools:Sanitize" keycode="VK_DELETE" modifiers="accel,shift"/>
>Some Macs don't have delete keys...
This doesn't seem fixed?

>>+    <command id="Tools:Sanitize"
>>+     oncommand="Components.classes['@mozilla.org/suite/suiteglue;1'].getService(Components.interfaces.nsISuiteGlue).sanitize(window || null);"/>
>Nit: incorrect indentation (although for that long a line, a function might be
>worth while)
I forgot to mention that the || null removal applies here too.

>>+          var bundleUtil = document.getElementById("bundleUtil");
>>+          document.documentElement.getButton("accept").label = bundleUtil.getString("sanitizeButton");
>Can't you define this in the .DTD? (buttonlabelaccept="&sanitizeButton.label;")
Heh, turns out that we have a whold .DTD just for this string ;-)

>>-<!ENTITY pref.security.title              "Privacy &amp; Security">
>>+<!ENTITY pref.security.title             "Privacy &amp; Security">
>Why the change in spacing?
I think the latest patch sneaked in even more spacing changes...

>>+// Factory object
>>+const SuiteGlueServiceFactory = {
>>+  _instance: null,
>>+  createInstance: function (outer, iid) 
>>+  {
>>+    if (outer != null)
>>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>>+    return this._instance == null ?
>>+      this._instance = new SuiteGlue() : this._instance;
>>+  }
>>+};
>Does GenerateModule not give you a factory?
I'll look into whether this is really necessary or not.

>>+        return false; // XXX: as long as we aren't using toolkit's download manager
>Don't do that, comment the code out instead
Actually /* ... */ style comments would be better.

Comment 10

[[e-mail]]

Comment on attachment 303655 [details] [diff] [review]
patch v2: address review comments

>+    <!-- Add additional sanitize keybinding for Macs without a delete key -->
>+    <key id="key_sanitize_mac" command="Tools:Sanitize" keycode="VK_BACK" modifiers="accel,shift"/>
Ah, there it is :-)

>+  Components.classes["@mozilla.org/suite/suiteglue;1"]
>+  .getService(Components.interfaces.nsISuiteGlue)
>+  .sanitize(window);
Nit: in places like this, the .s should line up i.e.
Components.classes["@mozilla.org/suite/suiteglue;1"]
          .getService(Components.interfaces.nsISuiteGlue)
          .sanitize(window);

>+// Factory object
>+const SuiteGlueServiceFactory = {
>+  _instance: null,
>+  createInstance: function (outer, iid)
>+  {
>+    if (outer != null)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>+    return this._instance == null ?
>+      this._instance = new SuiteGlue() : this._instance;
>+  }
>+};
So, it turns out, you don't need this...

>+    // check if we're in safe mode
>+    var app = Components.classes["@mozilla.org/xre/app-info;1"]
>+              .getService(Components.interfaces.nsIXULAppInfo)
>+              .QueryInterface(Components.interfaces.nsIXULRuntime);
>+    if (app.inSafeMode) {
>+      var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>+               .getService(Components.interfaces.nsIWindowWatcher);
>+      ww.openWindow(null, "chrome://browser/content/safeMode.xul",
>+                    "_blank", "chrome,centerscreen,modal,resizable=no", null);
>+    }
Whoops, stray browser code?

>+  // redefine the default factory for XPCOMUtils
>+  _xpcom_factory: SuiteGlueServiceFactory,
... or this.

>+var Sanitizer = {
>+  get _prefs() {
>+    delete this._prefs;
>+    return this._prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                         .getService(Components.interfaces.nsIPrefService).getBranch("privacy.sanitize.");
>+  },
Yummy...

Comment 11

[[e-mail]]

Comment on attachment 303655 [details] [diff] [review]
patch v2: address review comments

>+libs:: $(_SUITE_FILES)
>+ $(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/suite/$(relsrcdir)
Needs to be a tab, not a space!

+  sanitize: function(aParentWindow) {
+    if (this._prefs.getBoolPref("promptOnSanitize")) {
+      var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
+               .getService(Components.interfaces.nsIWindowWatcher);
+#ifdef XP_MACOSX
+      ww.openWindow(null, // make this an app-modal window on Mac
+#else
+      ww.openWindow(aParentWindow,
+#endif
+                    "chrome://communicator/content/sanitize.xul", "Sanitize",
+                    "chrome,titlebar,centerscreen,modal", null);
+      return null;
+    }
+    return sanitize();
+  },
This needs to be commented - sanitize() is not the same as this.sanitize()!
Also, this is the sanitize that's documented as having a void result, right?

Comment 12

[Robert Kaiser]

(In reply to comment #9)
> I think the latest patch sneaked in even more spacing changes...

I don't see those, I removed the spacing change at this place and aligned all new strings with the old spacing.

(In reply to comment #11)
> >+libs:: $(_SUITE_FILES)
> >+ $(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/suite/$(relsrcdir)
> Needs to be a tab, not a space!

Hmm, it actually is a tab here...

> This needs to be commented - sanitize() is not the same as this.sanitize()!
> Also, this is the sanitize that's documented as having a void result, right?

I added comments for that. Actually, both functions in Sanitizer.jsm are free to return anything they like, the function in nsSuiteGlue.js is the void one.

Comment 13

[Robert Kaiser]

Attachment: patch v3: more review comments, working tests

Here's the updated patch for the rest of the review comments. Additionally, I got the browser chrome test to run now, with some mochitest tweaks (will probably be filed by ajschult) and some fixes to the test. Running browser chrome tests (which look for browser_*.js so we need to stick with that) now results in 31 passing tests for this patch :)

Comment 14

[Robert Kaiser]

Oh, I forgot to mention I also integrated the patch for bug 404531 in nsSuiteGlue.js

Comment 15

[[e-mail]]

Comment on attachment 305568 [details] [diff] [review]
patch v3: more review comments, working tests

>+        var preference, name;
>+        for (var i = 0; i < sanitizePreferences.childNodes.length; ++i) {
>+          preference = sanitizePreferences.childNodes[i];
>+          if (preference.value) {
>+            name = Sanitizer.getNameFromPreference(preference.name);
Nit: still want these as separate declarations.

>+      this.sanitize(null) || // sanitize() returns null on full success
>+      this._prefs.setBoolPref("didShutdownSanitize", true);
Nit: Use if (!this.sanitize(null)) instead, avoids the comment
Problem: Nobody clears didShutdownSanitize!

>+function sanitize() {
Does this need to be separate for some reason? We already have to other sanitize methods, and this is getting confusing...

Comment 16

[Robert Kaiser]

Attachment: patch v3.1: address comments from v3

This new patch addresses the comments for v3, carrying forward r+, requesting sr? one more time.

Comment 17

[Robert Kaiser]

Comment on attachment 305769 [details] [diff] [review]
patch v3.1: address comments from v3

>Index: mozilla/suite/common/Makefile.in
>===================================================================
>+# DEFINES for preprocessing
>+# once we support the shell service, define this for supported platforms

Sorry, this part belongs to the app pane patch, will take care to not include that either on checkin or next patch.

Comment 18

[[e-mail]]

Comment on attachment 305769 [details] [diff] [review]
patch v3.1: address comments from v3

Almost there, but I noticed some new nits... sr=me with them fixed:

>+# DEFINES for preprocessing
>+# once we support the shell service, define this for supported platforms
>+#ifneq (,$(filter windows gtk2 mac cocoa, $(MOZ_WIDGET_TOOLKIT)))
>+#DEFINES += -DHAVE_SHELL_SERVICE=1
>+#endif
Not part of this patch ;-)

>+        <checkbox label="&itemSessions.label;"
>+                  accesskey="&itemSessions.accesskey;"
>+                  preference="privacy.item.sessions"/>
Is it worth having this? We already have Tools - Password Manager - Log Out,
and sessions aren't persisted after shutdown anyway.

>+  // profile shutdown handler (contains profile cleanup routines)
>+  _onProfileShutdown: function()
>+  {
>+    this.Sanitizer.onShutdown();
>+  },
Wrong function, did you mean Sanitizer.checkAndSanitize() (no this) perhaps?

>+<!ENTITY clearPrivateDataCmd.label "Clear Private Data…">
>+<!ENTITY clearPrivateDataCmd.accesskey "P">
Sorry, but Password Manager already uses P.

>+                    "chrome://communicator/content/sanitize.xul", "Sanitize",
>+                    "chrome,titlebar,centerscreen,modal", null);
modal really needs dialog too (by comparison openDialog defaults to dialog).

Comment 19

[Robert Kaiser]

Checked in with the shellservice stuff removed, and _onProfileShutdown, accesskey and openWindow() nits addressed.

This should be fixed, please file followups for any improvements or further changes of this.

Comment 20

[Robert Kaiser]

Attachment: add new entries to makefiles.sh

Shortly after checkin, I realized I forgot to add the new Makefiles to makefiles.sh, which causes no actual problem, but should be fixed. Here's the patch for that.

Comment 21

[Robert Kaiser]

Thanks, checked in the makefiles.sh patch as well.

Comment 22

[Worcester12345]

This will avoid the need for the Clear Private Data extension (http://webdesigns.ms11.net/seamonkey.html), thanks. But, I am wondering if there will be added the functionality to only clear certain items (cache, history, etc.) like in either the extension or Firefox. Thanks again.

Comment 23

[Robert Kaiser]

That functionality, matching what Firefox has, is already in the SeaMonkey nightlies. Passwords and download entries are currently disabled there though, until we support the new backends for those, which is planned for SeaMonkey 2 Alpha.

Comment 25

[Worcester12345]

(In reply to comment #23)
> That functionality, matching what Firefox has, is already in the SeaMonkey
> nightlies. Passwords and download entries are currently disabled there though,
> until we support the new backends for those, which is planned for SeaMonkey 2
> Alpha.
> 

I'm seeing the "clear now" button in preferences, but not in the "Clear Private Data" menu item. I am using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030302 SeaMonkey/2.0a1pre ID:2008030302

Comment 26

[Robert Kaiser]

The menu item should just do the clearing, try this again with a nightly from March 4 or later, where bug 420274 is fixed and all the files are actually included in the nightly. Thanks.

Comment 27

[Tobias Fischer]

Testing with Win32 2008030401-Trunk-Nightly with Patch from Bug 420274 included, I can confirm, that "Sanitize" works fine now. 

Thanks for adding this Feature, think it was really useful.

Comment 28

[Justin Wood (:Callek)]

Verified per c#27