409624 - FastFind not cleared when doing Clear private data

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[G]

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

Hello,
when on a site, after doing a fastfind (ctrl + f) and entering a word if I do Clear private data with "Saved form and search history" enabled this wont clean the fastfinder. Then if someone use the computer he can see which word was lastly searched with the fastfinder.

Reproducible: Always

Steps to Reproduce:
1. Do a fastfind search (ctrl +f) then enter a word and found it into any web page
2. Tools -> Clear Private data -> Saved form and search history enabled -> clear private data now
3. Do ctrl + f and you will see the last searched word in the fastfinder
Actual Results:  
We can see the last searched word in the fastfinder.

Expected Results:  
Fastfinder should be cleared.

Comment 1

[u88484]

It is cleared after a restart but not during the same session.  This is inconsistent with the location bar history being cleared right after clearing private data.

Comment 2

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1 - Clear find bar when clearing form data

Patch for review v1

Comment 3

[u88484]

Just click the Accept bug (change status to ASSIGNED) radio button and then hit commit to change this bugs status to Assigned and will set you as the person it is assigned to.

Comment 4

[Graeme McCutcheon [:graememcc]]

>Just click the Accept bug (change status to ASSIGNED) radio button and then hit
>commit to change this bugs status to Assigned and will set you as the person it
>is assigned to.

I was hoping noone noticed that I'd fouled that up! (Getting used to my new priveleges)

Apologies for bugspam all 

Comment 6

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

Comment on attachment 298331 [details] [diff] [review]
Patch v1 - Clear find bar when clearing form data

It seems a bit weird to tie find bar history to "saved form and search history", but I guess it is the closest fit (being somewhat like a "form").

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

>     formdata: {
>       clear: function ()

>         while (windows.hasMoreElements()) {
>-          var searchBar = windows.getNext().document.getElementById("searchbar");
>+          var currentWindow = windows.getNext();

nit: name it currentDoc and keep a reference to windows.getNext().document, since we never care about only the window.

>+          var searchBar = currentWindow.document.getElementById("searchbar");
>           if (searchBar) {
>             searchBar.value = "";
>             searchBar.textbox.editor.transactionManager.clear();

Not your code, but while you're here I think we should change this to:
searchBar.textbox.reset(); // clear value and clear transaction manager
searchBar.value = ""; // restore grey text (might not be necessary once bug 406095 lands)

>+          var findBar = currentWindow.document.getElementById("FindToolbar");
>+          if (findBar) {
>+            findBar._findField.value = "";
>+            findBar._findField.editor.transactionManager.clear();

Same here, though findBar._findField.reset(); should be sufficient since there's no "grey text".

>+            // As text box has been cleared, remove any lingering highlighting
>+            findBar._updateStatusUI(findBar.nsITypeAheadFind.FIND_FOUND);

Using FIND_FOUND here is a bit confusing, perhaps just omit the parameter and let _updateStatusUI's switch take the default branch (which is equivalent)? I'm a bit worried that there's more state in the findbar binding that we need to clear. Perhaps we should also call findBar.close() after setting the value?

r- for now, but will r+ if these concerns are addressed (either through code changes or justification of the current code).

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

This does not block the final release of Firefox 3.

Comment 9

[Graeme McCutcheon [:graememcc]]

Attachment: Checkpoint: Patch 2: WIP

> It seems a bit weird to tie find bar history to "saved form and search
> history", but I guess it is the closest fit (being somewhat like a "form").

And also somewhat like a search. Nothing else really ties-in, and I'm not sure it warrants its own option in the UI.

> searchBar.value = ""; // restore grey text (might not be necessary once 
> bug 406095 lands)
Assuming it now isn't necessary. WFM.

> Using FIND_FOUND here is a bit confusing, perhaps just omit the parameter and
> let _updateStatusUI's switch take the default branch (which is equivalent)?
OK.

> I'm a bit worried that there's more state in the findbar binding that we need  
> to clear.
Have removed some more obvious bits of state.
 
> Perhaps we should also call findBar.close() after setting the value?
Why? We wouldn't close the download manager if open when clearing download history.

A couple of points I could use your input on:

1) toggleHighlight() is called to remove any highlighting in the current tab. However, there could be residual highlighting lurking on other tabs from earlier usages of the find toolbar. Should I iterate over all the tabs in all the browsers to get rid of any highlighting that may have been left behind?

2) When the findbar finds a text match, it will select the text in the document.  If the user has the prefill from selection pref enabled, the next time they open find bar on that tab, assuming they don't select anything else in the meantime) the text will be prefilled again, giving the perception that the clearing of the find bar has failed.

I'm not really sure about selection code, but iterating over the tabs, calling window.getSelection() and then selection.removeRange() seems unworkable - we don't want to remove all selections, just the residuals from fastFind results.

I have discovered that gBrowser.fastFind.find("",false) will work, as calling it with the empty string resets the selection controller.

Should I go with that?

Comment 10

[Graeme McCutcheon [:graememcc]]

Attachment: Patch for review

This patch does the following:

- Moves logic to the respective widgets, given the code was using some "private" functions.

- Incorporates feedback from comment 6

- No longer worries about residual highlighting. This is an open bug: bug 279022

- Collapses the fastFind selection. (If this is not done, and the user has the prefill with selection pref set, it will appear as if the clear has not worked if they immediately reopen the find bar)

- Corrects logic for canClear. Currently on trunk, there is no way to clear search history if your form history is clear.

> It seems a bit weird to tie find bar history to "saved form and search
> history", but I guess it is the closest fit (being somewhat like a "form").

>And also somewhat like a search. Nothing else really ties-in, and I'm not sure
>it warrants its own option in the UI.

I think once we can get strings in the codebase again, a sensible split would be seperate options for Form History and Find/Search History.

Comment 11

[Graeme McCutcheon [:graememcc]]

Attachment: Patch for review + unit test

Above patch, now with unit test goodness.

Comment 13

[(mostly gone) XtC4UaLL [:xtc4uall]]

is there any special reason the patch not being reviewed?

Comment 14

[u88484]

Probably because there are still hundreds of bugs with blocking1.9.1? and wanted1.9.1 and same for 3.1 that haven't been sorted through yet so it is not know if this is wanted for 1.9.1/3.1 yet.

Comment 15

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

Comment on attachment 319773 [details] [diff] [review]
Patch for review + unit test

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

>     formdata: {
>       clear: function ()

>+          var currentDocument = windows.getNext().document;
>+          var searchBar = currentDocument.getElementById("searchbar");
>           if (searchBar) {
>+            searchBar.clear();
>+          }
>+          var findBar = currentDocument.getElementById("FindToolbar");
>+          if (findBar) {
>+            findBar.clear();
>           }

nit: no brackets around single-line then clauses.

>       get canClear()
>       {
>         var formHistory = Components.classes["@mozilla.org/satchel/form-history;1"]
>                                     .getService(Components.interfaces.nsIFormHistory2);
>-        return formHistory.hasEntries;
>+        var canClearForms = formHistory.hasEntries;

You can return true here and skip the rest of this work if hasEntries is true, right? Can also return early in the window loop if you find a clearable window.

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

>+      <property name="canClear" readonly="true">

>+        return  this.textbox.value != "" || tmn.numberOfUndoItems > 0 
>+                                         || tmn.numberOfRedoItems > 0;

nit: use this.value

>Index: browser/modules/Sanitizer.jsm

This file has since been removed on the trunk, no need to update it anymore.

Would also be nice to avoid adding trailing whitespace (including indentation whitespace on blank lines).

Looks good otherwise, sorry for the delay here. I'll r+ an updated patch addressing those comments.

Comment 16

[Graeme McCutcheon [:graememcc]]

Attachment: Patch

When clearing your inbox, one can occasionally discover long-forgotten bugs...this must have been one of my earliest bits of Moz involvement.

Updated patch. Gavin took care of search in bug 446274, so that part's dropped. Moved the findbar-changing parts of the test closer to the findbar code, and added various additional bits of testing.

I believe the Fx3 string freeze has now ended, so per first bit of comment 6 and last part of comment 10, we could now split form/search/find in the sanitizer and associated UI if you wish. (Personally, I still think they are semantically close enough that we don't need the distinction)

Comment 17

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

Comment on attachment 671525 [details] [diff] [review]
Patch

Re-directing to Drew, I won't be able to look at this for a little while.

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 671525 [details] [diff] [review]
Patch

Review of attachment 671525 [details] [diff] [review]:
-----------------------------------------------------------------

This is a nice patch.  I think including the find bar in "form and search history" is fine.

The reason I'm clearing the review request is that the patch (and bug) doesn't address the "Remember search and form history" option in Firefox's Privacy pref pane.  If clearing form and search history clears the find bar, then unchecking the similar option in the Privacy pref pane ought to cause the find bar to forget its state when it's closed.

I can r+ this patch (with the comments below addressed), but in that case I don't think it should land without the aforementioned companion behavior.  Whether that's done in a separate bug or here isn't important.

::: browser/base/content/test/browser_bug409624.js
@@ +29,5 @@
> +  prefBranch.setBoolPref("sessions", false);
> +  prefBranch.setBoolPref("siteSettings", false);
> +
> +  gBrowser.addEventListener("pageshow", doPageShow, false);
> +  gBrowser.loadURI("data:text/html,<h2>Text mozilla</h2>");

Is there a reason for setting all these prefs and loading a page?  Isn't the part inside doPageShow sufficient?

::: toolkit/content/tests/chrome/bug409624_window.xul
@@ +51,5 @@
> +      ok(!gFindBar.canClear, "canClear property false after clear() call");
> +
> +      gFindBar.open();
> +      let matchCaseCheckbox = gFindBar.getElement("find-case-sensitive");
> +      if (!matchCaseCheckbox.hidden & matchCaseCheckbox.checked)

& -> &&

@@ +52,5 @@
> +
> +      gFindBar.open();
> +      let matchCaseCheckbox = gFindBar.getElement("find-case-sensitive");
> +      if (!matchCaseCheckbox.hidden & matchCaseCheckbox.checked)
> +        matchCaseCheckbox.click();

You might want to assert matchCaseCheckbox.checked at this point.

::: toolkit/content/tests/chrome/test_bug409624.xul
@@ +32,5 @@
> +
> +      /** Test for Bug 409624 **/
> +      SimpleTest.waitForExplicitFinish();
> +      window.open("bug409624_window.xul", "409624test",
> +                  "chrome,width=600,height=600");

Is there a reason for opening a new window rather than doing the test here in this file?  It seems unnecessary.

::: toolkit/content/widgets/findbar.xml
@@ +286,5 @@
> +
> +          // Watch out for lazy editor init
> +          if (this._findField.editor) {
> +            let tm = this._findField.editor.transactionManager;
> +            return tm.numberOfUndoItems || tm.numberOfRedoItems;

This returns a number, while the other two returns return a bool.  So return !!(tm.numberOfUndoItems || tm.numberOfRedoItems) or the equivalent.

Comment 19

[Graeme McCutcheon [:graememcc]]

Attachment: Patch

Commments addressed. I had to modify the browser-chrome test for bug 567306. When bug 567306 landed, it had to modify test_typeaheadfind, which made an unsafe assumption that it was the first findbar-using test to run. Sadly the test for 567306 then went on to make the exact same assumption, which this patch breaks.

I also modified the browser-chrome test to simulate input rather than manually setting the value, so wrapped the relevant code in waitForFocus to avoid the potential for random orange.

> Is there a reason for setting all these prefs and loading a page?
I set the prefs only so the test proves that the findbar is indeed cleared as a result of the form history sanitize option, and not as the side-effect of another sanitize option. I can remove these if you want, although the form history pref certainly needs to stay to ensure we're testing what we think we're testing.

> Is there a reason for opening a new window rather than doing the test here in this file?
The way the test harness loads the test files confuses nsITypeaheadFind. The selections for the found text get set in the selection controller for the <iframe> in the test harness rather than the test's window object. This breaks any tests which need to check the find selection, such as this one.

Comment 20

[Graeme McCutcheon [:graememcc]]

> If clearing form and search history clears the find bar, then unchecking the similar option in the Privacy pref pane ought to cause the find bar to forget its state when it's closed.
I'm a bit cautious about this: although the wording is unfortunately similar, I think there is a slight semantic difference here: the pref in the privacy pane is controlling autocomplete suggestions. The privacy pane pref just prevents new entries being added, or historic entries being suggested - setting the pref doesn't clear any data. Likewise, setting the pref doesn't affect the contents of the search bar UI at any time - for example it doesn't clear the search bar after a search, it just ensures that search isn't suggested again. I suspect having a form input suggestion pref affecting the find UI/highlighting/selection when the findbar closes might not be intuitive.

That said, I accept I may be off here - I'm not sure what user expectations are given the similar wording of the options.

Also, when clearing state, we need to collapse the selection if the findbar found a match, as the textbox gets filled with the selection when the findbar opens, so if the selection isn't cleared and the findbar reopened it will look as if the findbar wasn't cleared. With the pref set, I'm worried we'd break find for the user who searches for a word, but wants to immediately close the findbar to reclaim some screen real-estate - they might not be able to locate the word again because we just cleared the selection. Likewise for highlighted matches. (Of course the landscape here may change depending on what happens with bugs like bug 352354, bug 340814 and bug 342101.)

Comment 21

[Graeme McCutcheon [:graememcc]]

>bugs like bug 352354...
er 325234

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 673282 [details] [diff] [review]
Patch

Review of attachment 673282 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Graeme McCutcheon [:graememcc] from comment #19)
> I set the prefs only so the test proves that the findbar is indeed cleared
> as a result of the form history sanitize option, and not as the side-effect
> of another sanitize option. I can remove these if you want, although the
> form history pref certainly needs to stay to ensure we're testing what we
> think we're testing.

OK, I see now.  The false prefs prevent sanitize from clearing their corresponding items, and the true pref does the opposite.  The page load gives the find some text to match, which creates a selection in the page, but the test doesn't check the selection, so is the load necessary?

(In reply to Graeme McCutcheon [:graememcc] from comment #20)

I'm only suggesting that the find bar be cleared after it's closed.  I don't think the page selection should be collapsed, even if that selection was made as a result of using find, and I think the bar should still be populated with the page selection when it's reopened.

But I didn't realize that highlights stick around after the bar is closed.  If the user chooses to highlight matches and then closes the bar, I can't say whether or not he's done with the find and wants the highlights to disappear.

So for now let's forget what I said about the pref in the Privacy pane.

Please run this by tryserver before landing.

::: browser/base/content/test/browser_bug567306.js
@@ +41,3 @@
>    selectText();
> +  findBar.onFindCommand();
> +  ok(findBar._findField.value == "Select Me", "Findbar is initialized with selection");

Use is() so that if this fails the actual value is logged.

Comment 23

[Graeme McCutcheon [:graememcc]]

Ah! Good point about the unnecessary load in the browser-chrome test. Testing the selection is cleared probably isn't needed there, given the toolkit test checks all the required bits of state are removed. If it's OK I'll switch back to just setting textbox.value manually, but set it immediately after setting the prefs and remove the page loading goop?

Thanks for the review :)

Comment 24

[Drew Willcoxon :adw]

(In reply to Graeme McCutcheon [:graememcc] from comment #23)
> If it's OK I'll switch back to just setting textbox.value
> manually, but set it immediately after setting the prefs and
> remove the page loading goop?

Sounds fine.  It sounds like you're saying you're removing the first !s.canClearItem() check, but at some point you should do that check after calling sanitize.

Comment 25

[Graeme McCutcheon [:graememcc]]

Try run at https://tbpl.mozilla.org/?tree=Try&rev=66ad180a4580 and one with an earlier version of the patch at https://tbpl.mozilla.org/?tree=Try&rev=cc1c3aed87af seem to point to this turning random orange browser_webconsole_bug_595934_message_categories.js into perma-orange, though I can't see how this patch could possibly affect it.

Curiously though, the webconsole test was green when I pushed this patch when testing another bug: https://tbpl.mozilla.org/?tree=Try&rev=bc9dfb6a482a

I'm claiming code problems: bug 803882, however will hold off on landing for now.

Comment 26

[Graeme McCutcheon [:graememcc]]

> I'm claiming code problems: bug 803882, however will hold off on landing for now.
A fix for that landed. Another try run was green modulo randomorange after a WinXP retrigger (https://tbpl.mozilla.org/?tree=Try&rev=a98954bf11c8), so let's go for it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3a1bcdc429

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f3a1bcdc429