628082 - Upon leaving PB mode, selected search engine is retained, whilst the search query was restored from pre-PB

Status: RESOLVED INVALID

(Firefox :: Private Browsing, defect)

Description

[Hmmwhatsthisdo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 ( .NET CLR 3.5.30729; .NET4.0C)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 ( .NET CLR 3.5.30729; .NET4.0C)

When leaving Private Browsing mode, the search engine last used is retained as the currently selected search engine. The search query is restored from pre-PB Mode (as per bug 463202). 

Reproducible: Always

Steps to Reproduce:
<example, "foo", "bar", Google, and Wikipedia can be replaced>
1. Search "foo" using Google.
2. Enter Private Browsing mode.
3. Search "bar" using Wikipedia.
4. Leave PB mode.
Actual Results:  
The data in the search box was "foo" using Wikipedia.

Expected Results:  
The data should be "foo" using Google.

The bug can be replicated using any search terms or providers, not just "foo" on Google and "bar" on Wikipedia.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

Confirmed with Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre ID:20110122154308.

Comment 2

[Hmmwhatsthisdo]

Confirmed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10 ID:20110121161358

Comment 4

[Hmmwhatsthisdo]

Still confirmed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0 ID:20110318052756.

Comment 5

[Tanner Filip [:tanner]]

Can confirm that this happens on Linux, not only Windows, on latest nightly.

Comment 6

[bryant rojas]

hi, me and a buddy are going to work on this if there's no one else on it :)

Comment 7

[Josh Matthews [:jdm]]

Great! Your first step should be to ensure that the bug is still reproducible by following the steps in comment 0.

Comment 8

[bryant rojas]

Great thanks! Yes the bug is still reproducible. Could you help us get started on this?

Comment 9

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#8483 is the code that is notified in the browser UI when a private browsing transition occurs. The onExitPrivateBrowsing function it calls in the same file contains the code that does the resetting here.

Comment 10

[(no longer active)]

Bryant, great to hear that you are interested to work on this!

Please let me know if the instructions in comment 9 are not sufficient, or if you need help with something else (building Firefox for example).  :-)

Comment 11

[bryant rojas]

Ah thank you very much! This community is quite helpful :)
For now it is sufficient, we took a look at the code and I believe we shouldn't have too much problems figuring it out. But if anything, I will return with questions :D

Comment 12

[bryant rojas]

Attachment: Patch that restores search engine after entering and exiting PB mode.

Guide to patch:
creates _searchEngineValue. When entering private browsing mode, the current engine is saved to _searchEngineValue. When exiting private browsing mode, the currentEngine attribute is set to _searchEngineValue.

Comment 13

[bryant rojas]

Attachment: test file

Javascript test file that test whether or not the search engine had been restored back to the original after exiting private browsing mode.

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 618865 [details] [diff] [review]
Patch that restores search engine after entering and exiting PB mode.

Thanks Bryant! I'm excited that you also included a test, although it's more helpful including the diff that adds a new test file instead of just the test() method.

Comment 15

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

Rather than holding a reference to the engine object, you could hold a reference to its name, and then use getEngineByName() to set currentEngine after the transition back.

Comment 16

[(no longer active)]

Comment on attachment 618865 [details] [diff] [review]
Patch that restores search engine after entering and exiting PB mode.

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

This patch looks fine, with Gavin's comments addressed.  Thanks!

Comment 17

[(no longer active)]

Comment on attachment 618866 [details]
test file

Can you please submit this as part of the patch itself?

Comment 18

[bryant rojas]

Attachment: Patch#2: Addressed gavin's comments and added test to patch.

Also changed the var name from _searchEngineValue to _searchEngineName.

Comment 19

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

You should probably guard against getEngineByName returning null, for example if the user removes the previously selected engine while in private browsing mode (setting currentEngine will throw in that case).

Comment 20

[bryant rojas]

Attachment: Gaurds against getEngineByName returning a null

If getEngineByName happens to return a null, currentEngine will be set to the defaultEngine. And if it doesn't return a null, the currentEngine will be set to what it was before PBmode (using name).

Comment 21

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

Comment on attachment 620841 [details] [diff] [review]
Gaurds against getEngineByName returning a null

I'm sorry that I keep noticing new issues every time I look at this, but... it doesn't really make sense to do this in browser.js, because the search engine's "currentEngine" is an app-global setting, and not per-window. So it seems like it would probably be better to handle this in the search service itself. Or, to keep things simple, maybe just do it in the private browsing service.

Comment 22

[bryant rojas]

Haha no no, this is quite educational for me, I shall look into them :}

Comment 23

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

I'm not sure if you've been on irc.mozilla.org yet, but Ehsan and I both hang out there, you can ping us if you have any questions.

Comment 24

[bryant rojas]

Attachment: Patch #4: Moved code over to PrivateBrowsingService.js

Think I got it this time around :} Sorry for the slow turnaround time! 
Code very similar to previous patches.
Not sure if I should create a new test or the previous works fine?

First stated the variable, _searchEngineName. 
When the transition to PBmode is happening, get the Browser Search Service, then _searchEngineName gets set to the currentEngine's name. 

On exit of PBmode revert currentEngine back to _searchEngineName. Null check intact.

Comment 25

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

Comment on attachment 621794 [details] [diff] [review]
Patch #4: Moved code over to PrivateBrowsingService.js

>diff --git a/browser/base/content/test/browser_bug628082.js b/browser/base/content/test/browser_bug628082.js

>\ No newline at end of file

Throw a newline in there to shut this warning up :)

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js

>+      var bss = Cc["@mozilla.org/browser/search-service;1"]
>+                .getService(Ci.nsIBrowserSearchService);

You can just use Services.search.

>+        if(bss.getEngineByName(this._searchEngineName) != null)
>+          bss.currentEngine = bss.getEngineByName(this._searchEngineName);

use |let engine = bss.getEngineByName(this._searchEngineName);| to avoid calling getEngineByName twice.

I haven't paid close attention to where in nsPrivateBrowsingService you're adding this code, ehsan might have better guidance there.

Comment 26

[(no longer active)]

Comment on attachment 621794 [details] [diff] [review]
Patch #4: Moved code over to PrivateBrowsingService.js

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

Your patch generally looks good.  Please address the comments below, and submit the final version and ask me for review.  Thanks!

::: browser/base/content/test/browser_bug628082.js
@@ +1,1 @@
> +function test() {

Please move this test to browser/components/privatebrowsing/test/browser, rename it to something like browser_privatebrowsing_searchengine.js and also add its name to the Makefile.in file in that directory.  Also, please add a license header similar to the rest of the tests in that directory.

@@ +10,5 @@
> +  pb.privateBrowsingEnabled = true;
> +
> +  var newEngine = Services.search.getVisibleEngines();
> + 
> +  for(var i=0; i<newEngine.length; i++) {

Nit: space before (, and before and after <.

@@ +11,5 @@
> +
> +  var newEngine = Services.search.getVisibleEngines();
> + 
> +  for(var i=0; i<newEngine.length; i++) {
> +    if(newEngine[i] != current) {

Nit: space before (.

Comment 27

[(no longer active)]

(also, please address Gavin's comments as well.  :-)

Comment 28

[bryant rojas]

Attachment: Patch #5: Fixed nitpicks, added test file to makefile and created new test.

I tried using Services.search, however when I tried entering private browsing mode, firefox seemed to get caught in some loop then crashed on me. Tried it several times but it kept happening. I then called the service, and left Services.search on the pbmode exit, but it wouldn't exit out of pbmode. Couldn't figure out why it was happening so I stuck with calling the service. Cc["@mozilla.org/browser/search-service;1"].getService(Ci.nsIBrowserSearchService);

Comment 29

[bryant rojas]

Attachment: Patch #6: Altered test file to not use google search engine.

My previous patch file,test file, assumed that the google search engine would be installed. Took it out :}

Comment 30

[(no longer active)]

Comment on attachment 622459 [details] [diff] [review]
Patch #6: Altered test file to not use google search engine.

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

Looks great, thanks!  Have you tested this patch on the try server?

Comment 31

[bryant rojas]

Not yet, currently trying to figure it out

Comment 32

[(no longer active)]

See https://wiki.mozilla.org/ReleaseEngineering/TryServer.  You'll need level 1 access for the try server, if you don't have it, please file a bug and CC me on it.

Comment 33

[Josh Matthews [:jdm]]

It's a shame this apparently got lost in the shuffle, but now that we have per-window private browsing this doesn't seem to be an issue any longer.