861691 - Search bar should not save search history in private windows

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Open a private window
2. Customise the toolbar to show the search bar
3. Perform a search
4. Press down arrow

Actual results: Search just performed appears in search history

Expected results: Search history unaffected by searches in private windows

Comment 1

[[e-mail]]

Attachment: Possible patch

While I was looking into this I noticed that the search backend (nsSearchSuggestions.js) expects the autocomplete search param to have a |private suffix when the window is private, so that it can use private channels. However the problem then appears to arise that the code to clear search suggestions (which I presume should continue to function in private windows) does not want the |private suffix. (Firefox doesn't do this, so I don't know whether it gets this wrong here, or whether I have misread the code.) To work around this I have simply hardcoded the search param when dealing with the search history.

Comment 2

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

(In reply to [e-mail] from comment #1)
> (Firefox doesn't do this, so I don't know whether it gets this wrong here, or
> whether I have misread the code.)

In the Firefox version of search.xml, the "private" suffix is only added by the JS getter (.searchParam, read by the autocomplete controller), the DOM attribute itself is left as-is.

Comment 3

[[e-mail]]

(In reply to Gavin Sharp from comment #2)
> (In reply to comment #1)
> > (Firefox doesn't do this, so I don't know whether it gets this wrong here, or
> > whether I have misread the code.)
> 
> In the Firefox version of search.xml, the "private" suffix is only added by
> the JS getter (.searchParam, read by the autocomplete controller), the DOM
> attribute itself is left as-is.

Subtle! Turns out it was my idea (bug 640427 comment 2, but only one version got changed) to switch SeaMonkey's "clear search autocomplete" to use the getter instead of the attribute...

Comment 4

[Philip Chee]

Comment on attachment 737311 [details] [diff] [review]
Possible patch

> +        if (this.usePrivateBrowsing)
> +          this._textbox.searchParam += "|private";

> -              textBox._formHistSvc.addEntry(textBox.getAttribute("autocompletesearchparam"),
> -                                            textValue);
> +              textBox._formHistSvc.addEntry("searchbar-history", textValue);

> -              return this._formHistSvc.nameExists(this.searchParam);
> +              return this._formHistSvc.nameExists("searchbar-history");

> -              var param = this.getAttribute("autocompletesearchparam");
> -              this._formHistSvc.removeEntriesForName(param);
> +              this._formHistSvc.removeEntriesForName("searchbar-history");

Instead of hardcoding for "searchbar-history" can we (in the constructor) cache the original autocompletesearchparam and reuse this?

Comment 5

[Philip Chee]

Comment on attachment 737311 [details] [diff] [review]
Possible patch

f=me

Comment 6

[Ian Neal]

Comment on attachment 737311 [details] [diff] [review]
Possible patch

Should switching to a different search engine within the private window be remembered?

Comment 7

[Philip Chee]

> Should switching to a different search engine within the private window be remembered?
What does Firefox do in this case?

Comment 8

[Philip Chee]

>> Should switching to a different search engine within the private window be remembered?
> What does Firefox do in this case?
As far as I can tell, Firefox doesn't check the private browsing status when switching to a different search engine.

Comment 9

[Philip Chee]

Comment on attachment 737311 [details] [diff] [review]
Possible patch

Discussed this over IRC.
r=me

Comment 10

[[e-mail]]

Pushed comm-central changeset 3658f802883b.

Comment 11

[[e-mail]]

Comment on attachment 737311 [details] [diff] [review]
Possible patch

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Private search information leak
Testing completed (on m-c, etc.): Landed on c-c a few days ago
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None