324354 - Ctrl-Z (undo) reveals visited URLs AFTER clearing history

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[bwj321]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

After clearing all private data (Ctrl-Shift-Del, checking all boxes), you can select either the URL address bar or the built in search bar and hit Ctrl-Z.  This will cycle through all of the previously visited addresses even though I have already cleared all history.  This works in both the URL and search fields.

Reproducible: Always

Steps to Reproduce:
1. Visit a few web sites and/or make a few searches from the built in search bar.
2. Clear all private data (Ctrl-Shift-Del)
3. Select either the URL address bar or the search bar.
4. Hit Ctrl-Z to repeatedly cycle through previously viewed URLs and/or search requests.

Actual Results:  
I can see all previously viewed URLs.

Expected Results:  
I see nothing since I've already cleared all private data.

I don't know if this is a Firefox 1.5 problem or an OS problem.  I'm running WinXP SP2 with all updates.

Comment 1

[Martijn Wargers (dead)]

I guess I'm responsible for this.

Comment 2

[Daniel Veditz [:dveditz]]

There's a known problem where "clear private data" doesn't clear session history (back-forward buttons) for which the workaround is to open a new tab (or window) and close the old one. This has a similar workaround: close the current window to clear the edit history stored in the address widget.

Sanitize Browsing History should zap the location bar undo history in all open browser windows. Sanitize Form Data should do the same for the undo history of the search box.

Comment 3

[Daniel Veditz [:dveditz]]

I don't think fixing bug 158071 is responsible. We want that bug to remain fixed, we want Sanitize to take proactive steps to tell these edit boxes to forget their undo history.

Comment 4

[Martijn Wargers (dead)]

Attachment: patch

so I guess the patch has to be something like this.

Comment 5

[Jesse Ruderman]

Does the patch fix the search bar and the URL bar, or just the URL bar?

Comment 6

[bwj321]

(In reply to comment #5)
> Does the patch fix the search bar and the URL bar, or just the URL bar?

It depends on which boxes are checked when clearing data.  If "Browsing History" is checked, then the URL bar history should be cleared.  If "Saved Form Information" is checked, then the search bar history should be cleared.

Comment 7

[Martijn Wargers (dead)]

No, it just fixes the URL Bar, I concentrated on what I regressed.
I'll see what I can do for the search bar.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch

Ok, this patch is actually good. It also takes care of clearing the undo history of the search bar (which was actually something I regressed after all).
If bug 312867 would be fixed in Firefox, then I would not need to QueryInterface for the editor with the searchbar (for the urlbar, it's somehow working already).

Comment 9

[[e-mail]]

Comment on attachment 209443 [details] [diff] [review]
patch

Apostrophitis: its undo history.

Comment 10

[[e-mail]]

(In reply to comment #8)
>(for the urlbar, it's somehow working already).
I guess as a result of the newline stripping patch.

Comment 11

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

(In reply to comment #8)
> If bug 312867 would be fixed in Firefox

I just landed the patch for that bug (exposing "editor").

Comment 12

[Martijn Wargers (dead)]

Attachment: patch2

Ok, removed the QI and fixed my apostrophitis.

Comment 13

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

Comment on attachment 209579 [details] [diff] [review]
patch2

>Index: browser/base/content/search.xml

>         doCommand: function (aCommand)
>         {
>+          // Clear the searchbar and its undo history
>+          this.mOuter.value = "";
>+          this.mOuter.mInputField.editor.enableUndo(false);
>+          this.mOuter.mInputField.editor.enableUndo(true);

You can just use "this.mOuter.editor", given the patch in the other bug, no?
Same comment for the other ones (gURLBar.editor and searchBar.mTextbox.editor).

>Index: browser/base/content/browser.js

>+    //Clear undo history of all searchBars

URL Bars?

Comment 14

[Martijn Wargers (dead)]

Attachment: patch3

Oops, you're right. This patch fixes that.

Comment 15

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

Comment on attachment 209704 [details] [diff] [review]
patch3

>Index: browser/base/content/browser.js

>+    //Clear undo history of all searchBars

URL bars? :)

>Index: browser/base/content/sanitize.js

>+          if (searchBar) {
>+            searchBar.mTextbox.mInputElt.value = "";

Just mTextbox.value?

>+            const nsIDOMNSEditableElement = Components.interfaces.nsIDOMNSEditableElement;

No need for this, right?

Comment 16

[Martijn Wargers (dead)]

Attachment: patch

Thanks, I fixed those issues.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 209705 [details] [diff] [review]
patch


>+    //Clear undo history of all URL Bars
>+    var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>+    var windowManagerInterface = windowManager.QueryInterface(Components.interfaces.nsIWindowMediator);
>+    var windows = windowManagerInterface.getEnumerator("navigator:browser");
>+    while (windows.hasMoreElements()) {
>+      var urlBar = windows.getNext().gURLBar;
>+      if (urlBar) {
>+        gURLBar.editor.enableUndo(false);
>+        gURLBar.editor.enableUndo(true);
>+      }
>+    }

I'm sure you meant urlbar here! r=me with that fixed

Comment 18

[Martijn Wargers (dead)]

Attachment: patch5

Oops, yes, that was an obvious mistake.

Comment 19

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

mozilla/browser/base/content/browser.js; new revision: 1.559;
mozilla/browser/base/content/sanitize.js; new revision: 1.14;
mozilla/browser/base/content/search.xml; new revision: 1.36;

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 210265 [details] [diff] [review]
patch5

>Index: browser/base/content/search.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/search.xml,v
>retrieving revision 1.35
>diff -u -8 -p -r1.35 search.xml
>--- browser/base/content/search.xml	11 Jan 2006 20:24:20 -0000	1.35
>+++ browser/base/content/search.xml	26 Jan 2006 15:52:02 -0000
>@@ -384,19 +384,23 @@
>         isCommandEnabled: function (aCommand)
>         {
>           return this.mOuter.formHistSvc.nameExists(
>                  this.mOuter.getAttribute("autocompletesearchparam"));
>         },
> 
>         doCommand: function (aCommand)
>         {
>+          // Clear the searchbar and its undo history
>+          this.mOuter.value = "";
>+          this.mOuter.editor.enableUndo(false);
>+          this.mOuter.editor.enableUndo(true);
>+
>           this.mOuter.formHistSvc.removeEntriesForName(
>               this.mOuter.getAttribute("autocompletesearchparam"));
>-          this.mOuter.value = "";
>         }
>       })
>       ]]></field>
> 
>       <!-- DND Observer -->
>       <field name="searchbarDNDObserver" readonly="true"><![CDATA[
>       ({
>         mOuter: this,
>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.555
>diff -u -8 -p -r1.555 browser.js
>--- browser/base/content/browser.js	20 Jan 2006 23:04:16 -0000	1.555
>+++ browser/base/content/browser.js	26 Jan 2006 15:52:11 -0000
>@@ -237,16 +237,28 @@ const gSessionHistoryObserver = {
>   {
>     if (topic != "browser:purge-session-history")
>       return;
> 
>     var backCommand = document.getElementById("Browser:Back");
>     backCommand.setAttribute("disabled", "true");
>     var fwdCommand = document.getElementById("Browser:Forward");
>     fwdCommand.setAttribute("disabled", "true");
>+
>+    //Clear undo history of all URL Bars
>+    var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>+    var windowManagerInterface = windowManager.QueryInterface(Components.interfaces.nsIWindowMediator);
>+    var windows = windowManagerInterface.getEnumerator("navigator:browser");
>+    while (windows.hasMoreElements()) {
>+      var urlBar = windows.getNext().gURLBar;
>+      if (urlBar) {
>+        urlBar.editor.enableUndo(false);
>+        urlBar.editor.enableUndo(true);
>+      }
>+    }
>   }
> };
> 
> /**
>  * Given a starting docshell and a URI to look up, find the docshell the URI
>  * is loaded in. 
>  * @param   aDocument
>  *          A document to find instead of using just a URI - this is more specific. 
>Index: browser/base/content/sanitize.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/sanitize.js,v
>retrieving revision 1.11
>diff -u -8 -p -r1.11 sanitize.js
>--- browser/base/content/sanitize.js	8 Oct 2005 15:44:47 -0000	1.11
>+++ browser/base/content/sanitize.js	26 Jan 2006 15:52:12 -0000
>@@ -187,16 +187,29 @@ Sanitizer.prototype = {
>                                       .getService(Components.interfaces.nsIBrowserHistory);
>         return globalHistory.count != 0;
>       }
>     },
>     
>     formdata: {
>       clear: function ()
>       {
>+        //Clear undo history of all searchBars
>+        var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>+        var windowManagerInterface = windowManager.QueryInterface(Components.interfaces.nsIWindowMediator);
>+        var windows = windowManagerInterface.getEnumerator("navigator:browser");
>+        while (windows.hasMoreElements()) {
>+          var searchBar = windows.getNext().document.getElementById("searchbar");
>+          if (searchBar) {
>+            searchBar.mTextbox.value = "";
>+            searchBar.mTextbox.editor.enableUndo(false);
>+            searchBar.mTextbox.editor.enableUndo(true);
>+          }
>+        }
>+
>         var formHistory = Components.classes["@mozilla.org/satchel/form-history;1"]
>                                     .getService(Components.interfaces.nsIFormHistory);
>         formHistory.removeAllEntries();
>       },
>       
>       get canClear()
>       {
>         var formHistory = Components.classes["@mozilla.org/satchel/form-history;1"]

Comment 21

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

Checked in on the 1.8 branch.
mozilla/browser/base/content/browser.js; new revision: 1.479.2.54;
mozilla/browser/base/content/sanitize.js; new revision: 1.5.2.9;
mozilla/browser/base/content/search.xml; new revision: 1.25.2.8;

Comment 22

[Reed Loden [:reed]]

Comment on attachment 210265 [details] [diff] [review]
patch5

>+          this.mOuter.editor.enableUndo(false);
>+          this.mOuter.editor.enableUndo(true);

>+        urlBar.editor.enableUndo(false);
>+        urlBar.editor.enableUndo(true);

>+            searchBar.mTextbox.editor.enableUndo(false);
>+            searchBar.mTextbox.editor.enableUndo(true);

^^^ What happens when enableUndo() has already been set to false? Wouldn't this set it to true (maybe causing something else to mess up)? I'm not sure if this would ever happen or cause a real problem, but I thought I should bring it up.

Comment 23

[Mike Connor [:mconnor]]

I am unaware of any situation where we support, or care about, a case where someone has disabled undo in the URL bar or search textboxes.

Comment 24

[Martijn Wargers (dead)]

Well, maybe a special xul attribute (disableundo="true" or something) could be created, so it would be easy to keep track of this.
But I doubt if it would used much.

Comment 25

[Daniel Veditz [:dveditz]]

Doesn't look like this was really wanted in 1.8.0, and it's too late now for .0.2 for a bug that isn't fixing a critical security hole. If you still want this for the 1.8.0 branch please nominate for 1.8.0.3 and request approval on the patch.