Closed Bug 628082 Opened 12 years ago Closed 10 years ago

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

Categories

(Firefox :: Private Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: hmmwhatsthisdo, Assigned: bryant.rjs)

Details

(Whiteboard: [mentor=ehsan][lang=js])

Attachments

(3 files, 4 obsolete files)

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.
Version: unspecified → 3.6 Branch
Confirmed with Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre ID:20110122154308.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 3.6 Branch → Trunk
Assignee: nobody → ehsan
Whiteboard: [post-2.0]
Confirmed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10 ID:20110121161358
Still confirmed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0 ID:20110318052756.
Can confirm that this happens on Linux, not only Windows, on latest nightly.
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [post-2.0] → [mentor=ehsan][lang=js]
hi, me and a buddy are going to work on this if there's no one else on it :)
Great! Your first step should be to ensure that the bug is still reproducible by following the steps in comment 0.
Assignee: ehsan → bryant.rjs
Great thanks! Yes the bug is still reproducible. Could you help us get started on this?
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.
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).  :-)
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
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.
Attachment #618865 - Flags: review?
Attached file 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.
Attachment #618866 - Flags: review?
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.
Attachment #618865 - Flags: review? → review?(ehsan)
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 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!
Attachment #618865 - Flags: review?(ehsan)
Attachment #618866 - Attachment mime type: application/octet-stream → text/plaiin
Attachment #618866 - Attachment mime type: text/plaiin → text/plain
Comment on attachment 618866 [details]
test file

Can you please submit this as part of the patch itself?
Attachment #618866 - Flags: review?
Also changed the var name from _searchEngineValue to _searchEngineName.
Attachment #619814 - Flags: review?
Attachment #618865 - Attachment is obsolete: true
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).
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).
Attachment #620841 - Flags: review?(gavin.sharp)
Attachment #619814 - Flags: review?
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.
Attachment #620841 - Flags: review?(gavin.sharp) → review-
Haha no no, this is quite educational for me, I shall look into them :}
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.
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.
Attachment #621794 - Attachment description: Moved code over to PrivateBrowsingService.js → Patch #4: Moved code over to PrivateBrowsingService.js
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.
Attachment #621794 - Flags: feedback+
Attachment #619814 - Attachment is obsolete: true
Attachment #620841 - Attachment is obsolete: true
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 (.
(also, please address Gavin's comments as well.  :-)
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);
Attachment #622450 - Flags: review?(ehsan)
My previous patch file,test file, assumed that the google search engine would be installed. Took it out :}
Attachment #622450 - Attachment is obsolete: true
Attachment #622459 - Flags: review?(ehsan)
Attachment #622450 - Flags: review?(ehsan)
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?
Attachment #622459 - Flags: review?(ehsan) → review+
Not yet, currently trying to figure it out
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.
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.