STR: 1. enter PB mode 2. clear cache 3. search "open office" from search bar 4. check about:cache

Oh boy, nsNSSCallbacks creates a channel and loadgroup out of thin air in nsHTTPDownloadEvent::Run.

Seems like this depends on bug 722979 so that PSM knows to set the private browsing flag on the channel. Also note that AuthCertificate() writes data to disk permanently and unconditionally.

I was wrong, this isn't the PSM code. There's an XMLHttpRequest that's created in search.xml.

The test for this patch depends on the patch in bug 723001, so I'll wait until that gets landed.

Comment on attachment 670955 [details] [diff] [review] Patch (v1) Review of attachment 670955 [details] [diff] [review]: ----------------------------------------------------------------- I don't think I should review this.

Comment on attachment 670955 [details] [diff] [review] Patch (v1) You never use split("|")[1] in the front-end code, so the change to autocompletesearchparam in search.xml seems unnecessary.

>+ this.setAttribute("autocompletesearchparam", >+ this.getAttribute("autocompletesearchparam").split("|")[0] + >+ (PrivateBrowsingUtils.isWindowPrivate(window) ? "private" : "non-private")); Won't this also give searchbar-historyprivate? I think you want a | in between.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6) > Comment on attachment 670955 [details] [diff] [review] > Patch (v1) > > You never use split("|")[1] in the front-end code, so the change to > autocompletesearchparam in search.xml seems unnecessary. I did that to make it easier to know that the autocompletesearchparam is carrying two pieces of information by just looking at the code, but I'll take it out. (In reply to Josh Matthews [:jdm] from comment #7) > >+ this.setAttribute("autocompletesearchparam", > >+ this.getAttribute("autocompletesearchparam").split("|")[0] + > >+ (PrivateBrowsingUtils.isWindowPrivate(window) ? "private" : "non-private")); > > Won't this also give searchbar-historyprivate? I think you want a | in > between. Good catch. I did that locally in a different commit and then forgot to squash it. :/

Comment on attachment 670992 [details] [diff] [review] Patch (v2) Rather than actually setting the autocompletesearchparam attribute in openPopup, I think you should override the searchparam getter (http://hg.mozilla.org/mozilla-central/annotate/90857937b601/toolkit/content/widgets/autocomplete.xml#l159), and have it return the hacked value. Then change all the other users of the attribute in search.xml to just use this.searchParam rather than the attribute. Then add a bunch of comments explaining what the deal is with this magic string argument munging, because this is incredibly gross :(

(In reply to comment #10) > Comment on attachment 670992 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=670992 > Patch (v2) > > Rather than actually setting the autocompletesearchparam attribute in > openPopup, I think you should override the searchparam getter > (http://hg.mozilla.org/mozilla-central/annotate/90857937b601/toolkit/content/widgets/autocomplete.xml#l159), > and have it return the hacked value. Then change all the other users of the > attribute in search.xml to just use this.searchParam rather than the attribute. > > Then add a bunch of comments explaining what the deal is with this magic string > argument munging, because this is incredibly gross :( Hmm, do you really think that is the best solution? I thought about that, but this is something that only the search box autocomplete code will ever need as far as I can tell, and baking this logic into autocomplete.xml looks like a mistake to me. We can keep the magic string local to the search box autocomplete, so why spread this plague across all autocomplete fields? (I totally agree that this is gross, but the alternative would be to modify the autocomplete interfaces, which would be even more gross, as it spreads the sickness across the code base. I _really_ would like to keep this horribleness local to the search box implementation. :/)

(In reply to Ehsan Akhgari [:ehsan] from comment #11) > Hmm, do you really think that is the best solution? I thought about that, > but this is something that only the search box autocomplete code will ever > need as far as I can tell, and baking this logic into autocomplete.xml looks > like a mistake to me. We can keep the magic string local to the search box > autocomplete, so why spread this plague across all autocomplete fields? You don't need to touch anything in autocomplete.xml to implement my suggestion. I'm suggesting that you override the searchParam getter in search.xml.

Comment on attachment 671540 [details] [diff] [review] Patch (v3) >diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml >+ textBox._formHistSvc.addEntry(this.searchParams, textValue); typo: searchParam (no "s") I think this looks OK, but it needs comments (both in search.xml and nsSearchSuggestions) about the hacks (i.e. how you're abusing searchParam), and ideally a test (do we really not have one that would have caught this typo? :/).

I'll write a test for it once bug 723001 relands.

So I'm pretty sure that we don't want to change the getAttribute calls in search.xml to use the property, since those should all return "searchbar-history". I guess we could use .searchParams.split("|")[0] there but that wouldn't make a lot of sense to me. Please let me know if you disagree.

Comment on attachment 671617 [details] [diff] [review] Patch (v4) You're right about the getAttribute calls in search.xml - those don't want the modified value, only the GetSearchParam caller in nsAutocompleteController does. I don't think you need to modify set the searchParam setter - in practice no one really calls that on the search bar, and if they somehow did I don't think they'd want this weird behavior. You could only pass "|private" in private browsing windows and leave searchParam unmodified otherwise (and change the check in nsSearchSuggestions accordingly). That might reduce the likelihood of this hack causing trouble outside of PB windows. Still need tests!

Comment on attachment 671617 [details] [diff] [review] Patch (v4) This is not the most beautiful patch ever, but it gets the job done. I think it has a medium risk level, but we do need it for Firefox 18, and it's best to land it earlier in the cycle than later so that we can get testing and deal with any possible regressions.

(In reply to Josh Matthews [:jdm] from comment #0) > STR: > 1. enter PB mode > 2. clear cache > 3. search "open office" from search bar > 4. check about:cache In PB mode, leave google.com tab opened, and in another tab open about:cache. After couples of minutes, there will be a disk cache entry: https://sb-ssl.google.com/safebrowsing/newkey?client=navclient-auto-ffox&appver=19.0a1&pver=2.2 and that will not disappear after exiting PB. Same behavior on yahoo.com, amazon.com. Tested on Nightly 19.0a1 (2012-10-16)

That's not an entry we need to care about. It doesn't reveal anything about where the user has visited, so it's fine.

(In reply to comment #22) > That's not an entry we need to care about. It doesn't reveal anything about > where the user has visited, so it's fine. Precisely.

No search suggest results to the disk cache. Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b3