Last Comment Bug 861691 - Search bar should not save search history in private windows
: Search bar should not save search history in private windows
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.20
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 460895 595235
  Show dependency treegraph
 
Reported: 2013-04-14 16:06 PDT by neil@parkwaycc.co.uk
Modified: 2013-07-21 20:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Possible patch (4.25 KB, patch)
2013-04-14 16:12 PDT, neil@parkwaycc.co.uk
philip.chee: review+
iann_bugzilla: feedback+
philip.chee: feedback+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2013-04-14 16:06:34 PDT
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 neil@parkwaycc.co.uk 2013-04-14 16:12:17 PDT
Created attachment 737311 [details] [diff] [review]
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: gavin@gavinsharp.com] 2013-04-14 17:48:33 PDT
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2013-04-15 00:34:36 PDT
(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 2013-04-17 08:46:29 PDT
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 2013-04-19 07:11:08 PDT
Comment on attachment 737311 [details] [diff] [review]
Possible patch

f=me
Comment 6 Ian Neal 2013-04-22 16:10:46 PDT
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 2013-04-22 20:55:15 PDT
> Should switching to a different search engine within the private window be remembered?
What does Firefox do in this case?
Comment 8 Philip Chee 2013-04-24 08:27:37 PDT
>> 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 2013-05-03 07:20:35 PDT
Comment on attachment 737311 [details] [diff] [review]
Possible patch

Discussed this over IRC.
r=me
Comment 10 neil@parkwaycc.co.uk 2013-05-05 02:12:59 PDT
Pushed comm-central changeset 3658f802883b.
Comment 11 neil@parkwaycc.co.uk 2013-05-08 02:29:13 PDT
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

Note You need to log in before you can comment on or make changes to this bug.