Closed Bug 1048513 Opened 10 years ago Closed 10 years ago

location bar: In Private Browsing mode Firefox shouldn't save browser.fixup.domainwhitelist. entries

Categories

(Firefox :: Private Browsing, defect)

33 Branch
x86
Linux
defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.2
Tracking Status
firefox33 + verified
firefox34 + verified

People

(Reporter: 121212212121, Assigned: alexbardas, Mentored)

References

Details

(Keywords: privacy, Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

1. Start Firefox Aurora with the permanent Private Browsing mode enabled or open a new Private Window.
2. Type 123 into the location bar
3. Firefox asks "Did you mean to go to 123?" then click on "Yes, take me to 123"
4. Close the Private Window or close the Firefox (permanent mode)
5. Open Firefox (permanent mode) and type into the location bar about:config
6. Type browser.fixup.domainwhitelist.
7. And there is browser.fixup.domainwhitelist.123


Actual results:

Firefox saves the whitelist in Private Browsing mode.


Expected results:

In Private Browsing mode Firefox shouldn't save browser.fixup.domainwhitelist. entries or at least says it to the User.
Blocks: 693808
Points: --- → 2
QA Whiteboard: [qa+]
Component: Untriaged → General
Flags: firefox-backlog+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Mentor: gijskruitbosch+bugs
I'm not really convinced this falls on the "history" rather than the "bookmarks" side of the fence. These are normally local servers anyway, so I'm not really sure how privacy-sensitive the information is, either (if you alias hosts in your hosts file, it'd leave traces there, so I'm kind of assuming this kind of information wouldn't actually reveal anything about the user that the relevant actor wouldn't know already either through the hosts or the local DNS results). Ehsan?
Flags: needinfo?(ehsan)
Adding a bookmark is a relatively clear "I want to create a permanent record of this URL" action. "Yes, take me to 123" does not clearly imply "...and record its address permanently on disk".
I agree with Gavin, this is really not obvious.  What we're saving here is essentially what the user has typed in the location bar, IINM, so it's actually pretty privacy sensitive.
Component: General → Private Browsing
Flags: needinfo?(ehsan)
Right then. In order to fix this, the code here:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=782db4943ae5&mark=833-835#833

will need to be adjusted to check if the window is private (using PrivateBrowsingUtils.isWindowPrivate(window) ), and not set the pref if it is private.

You can easily check the behaviour is correct by changing the pref for "localhost" (search through about:config for localhost and you'll see it) to `false`, and then typing "localhost" in the location bar in a private window and hitting enter. Once you then click "Yes" on the notification bar that shows up, the pref should *not* be reset to its default value of `true`.

You should ideally also add a test in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js . This should open a new private window and wait for it to have completely loaded, and then run the same kind of test we currently do for "localhost", except it should verify that no pref would be saved when clicking "yes".


Note that this will mean that in private windows, no matter what you do, unless you've whitelisted the host from a non-private window, you will always be sent to a search page when typing in a single word that could be a host. You could workaround by using "//word" instead of "word", on Windows. On Mac/Linux you'd need to use "://word" instead.

If we wanted a way to persist the decision to whitelist a host for the lifetime of the private window "properly" (ie ensuring no request would make it out to the search provider if you used the same input a second or third or nth time after clicking "yes" the first time), that would involve significant rearchitecturing of the docshell code (probably to do some kind of polling check to the originating window to see if it's OK?), which I'm not sure would be worth the extra effort considering how much of an edgecase it would be.
Whiteboard: [good first bug][lang=js]
(In reply to :Gijs Kruitbosch from comment #4)
> Note that this will mean that in private windows, no matter what you do,
> unless you've whitelisted the host from a non-private window, you will
> always be sent to a search page when typing in a single word that could be a
> host. You could workaround by using "//word" instead of "word", on Windows.
> On Mac/Linux you'd need to use "://word" instead.
> 
> If we wanted a way to persist the decision to whitelist a host for the
> lifetime of the private window "properly" (ie ensuring no request would make
> it out to the search provider if you used the same input a second or third
> or nth time after clicking "yes" the first time), that would involve
> significant rearchitecturing of the docshell code (probably to do some kind
> of polling check to the originating window to see if it's OK?), which I'm
> not sure would be worth the extra effort considering how much of an edgecase
> it would be.

I don't think this is worth fixing.  At least not until a significant number of people have complained.  I am willing to bet that they won't, since today was the first time I learned about this feature.  ;-)
Assignee: nobody → abardas
Status: NEW → ASSIGNED
The trickiest part was to test it (if I don't wait for the `pageshow` event, the tests don't pass anymore).

I've also modified 2 methods from head.js, but the new changes are backwards compatible.
Attachment #8470094 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8470094 [details] [diff] [review]
bug1048513_in_private_browsing_mode_browser.fixup.domainwhitelist_should_not_be_saved.diff

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

This is nice! The browser.js change is perfect.

But yes, the test changes were always going to be the hardest - I'm sorry about the trouble; maybe I should have included more details there. The review is a little long now. I hope it doesn't seem too daunting; let me know if any of my comments are unclear (and note also that my suggestions were untested, so it's possible that some might not work, in which case don't hesitate to just let me know and leave something the way it is for the next review).

By the by, do you have try server access to check the new test changes run fine on our automation infrastructure?

::: browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js
@@ +10,5 @@
>      notificationObserver.disconnect();
>    }
>  });
>  
> +function promisePageShown(aBrowser, aLocation) {

aLocation seems to be unused here? Make sure you update the callers, too. :-)

@@ +46,5 @@
>    }
>    return deferred.promise;
>  }
>  
> +function* runURLBarSearchTest(aWindow, valueToOpen, expectSearch, expectNotification) {

good news, we have default parameter values! If you make aWindow the last param and default to window:

function* runURLBarSearchTest(valueToOpen, expectSearch, expectNotification, aWindow=window)

then you shouldn't need to change all the callers. :-)

@@ +77,3 @@
>    return function* test_navigate_single_host() {
>      const pref = "browser.fixup.domainwhitelist.localhost";
> +    let win = (isPrivate === true) ? yield whenNewWindowLoaded({private: true}) : gBrowser.ownerGlobal;

s/gBrowser.ownerGlobal/window/, should work, I think?

Also, I think just: let win = isPrivate ? ...

is fine here.

@@ +90,5 @@
>      notification.querySelector(".notification-button-default").click();
>  
>      // check pref value
>      let prefValue = Services.prefs.getBoolPref(pref);
> +    Assert.equal(prefValue, !isPrivate, "Pref should have the correct state.");

s/Assert.equal/is/, because that's what the rest of the test uses, and the jury's still out on whether we generally prefer Assert over mochitest's own assertions.

This is a little nitpicky, for which I'm sorry, but I've grown warier of immediately trying to switch to the newfangled stuff after things like bug 1048288, so I'd prefer not to change this here, especially for something that'll need to be uplifted to Aurora.

@@ +98,4 @@
>  
>      // Now try again with the pref set.
> +    let tab = browser.selectedTab = browser.addTab();
> +    yield promisePageShown(browser, hostName);

Is this actually necessary here?

@@ +102,5 @@
> +    // In a private window, the notification should appear again.
> +    yield* runURLBarSearchTest(win, hostName, isPrivate, isPrivate);
> +    browser.removeTab(tab);
> +    if (isPrivate) {
> +      win.close();

This should yield for the window's unload handler a la:

http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/head.js#251

if there isn't something like this in browser/base/content/test/general/head.js, feel free to cargo cult that function. :-(

@@ +108,4 @@
>    }
>  }
>  
> +add_task(get_test_function_for_localhost_with_hostname("localhost", true));

Nit: do this after checking the non-private cases.

@@ +108,5 @@
>    }
>  }
>  
> +add_task(get_test_function_for_localhost_with_hostname("localhost", true));
> +add_task(get_test_function_for_localhost_with_hostname("localhost", false));

Nit: don't bother updating these with an explicit "false"; JS doesn't mind you passing fewer arguments.

@@ +114,5 @@
>  
>  add_task(function* test_navigate_invalid_url() {
>    let tab = gBrowser.selectedTab = gBrowser.addTab();
> +  let url = "mozilla is awesome";
> +  yield promisePageShown(gBrowser, url);

This didn't need a pageshow listener before... why does it need one now?

::: browser/base/content/test/general/head.js
@@ +150,5 @@
>      win.removeEventListener("load", onLoad, false);
> +    if (typeof aCallback == "function") {
> +      aCallback(win);
> +    } else {
> +      deferred.resolve(win);

Hmmmmmm. Instead, why not:

function promiseNewWindowLoaded(aOptions) {
  let deferred = Promise.defer();
  whenNewWindowLoaded(aOptions, deferred.resolve);
  return deferred.promise;
}

and use that one where you wanted to use this one with a promise? :-)


Note that I *suspect* the pageshow problem is that this function only waits for browser load, not for delayed-startup-finished. See  http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/head.js#230 for the slightly more convoluted promise version which does that. Might be worth checking if cargo-culting that will let you omit the pageshow handling.

@@ +343,5 @@
>   * @param aExpectedURL
>   *        The URL of the document that is expected to load.
>   * @return promise
>   */
> +function waitForDocLoadAndStopIt(aExpectedURL, aBrowser) {

I think just aBrowser=gBrowser here would do the same as the next statement :-)

@@ +344,5 @@
>   *        The URL of the document that is expected to load.
>   * @return promise
>   */
> +function waitForDocLoadAndStopIt(aExpectedURL, aBrowser) {
> +  let browser = (typeof aBrowser !== "undefined") ? aBrowser : gBrowser;

... if not, let browser = aBrowser || gBrowser; will be fine, no need to do strict checks.
Attachment #8470094 - Flags: review?(gijskruitbosch+bugs) → feedback+
Made the proposed changes. I'll shortly push on try.
Attachment #8470094 - Attachment is obsolete: true
Attachment #8470145 - Flags: review?(gijskruitbosch+bugs)
Here is the try status:

https://tbpl.mozilla.org/?tree=Try&rev=0e44688c2976

It seems that a test timed out on a linux machine.
Comment on attachment 8470145 [details] [diff] [review]
bug1048513_in_private_browsing_mode_browser.fixup.domainwhitelist_should_not_be_saved.diff

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

This looks fine to me, besides the nits below.

Regarding the timeout... as far as I can tell the test was just unlucky in taking so long, it seems to have gotten "stuck" in the beginning waiting for a bunch of debug test cleanup, and it didn't at all look stuck at the point where the "Test Timed Out" bit hit, if you look in the log there are logs with the right timestamp right there. Going over the code, I also didn't spot something that would indicate anything went wrong with the test itself (although I could obviously have missed something...).

This is unfortunate, because the not so nice fix is to call SimpleTest.requestLongerTimeout(2), and then it should more reliably succeed. The better fix would be to split the private browsing test off into its own test file, but I feel bad asking you to do that after all the refactoring you did to fit it in this one. I've retriggered the bc1 run that failed a few times... if they succeed, I'm tempted to suggest just chucking in the requestLongerTimeout for safety (run it locally to make sure I got the syntax right from memory, please!) and land it like that. If not... we might have to split off the test and investigate what's going wrong. :-(

One thing I did notice in the bc1 run debug output was that there's a race condition in the existing code, in principle unrelated to this patch, which causes these error messages to be printed:

12:13:51     INFO -  10555 INFO Console message: [JavaScript Warning: "ReferenceError: reference to undefined property docshellRef.document" {file: "chrome://browser/content/browser.js" line: 10534}]
12:13:51     INFO -  10556 INFO Console message: [JavaScript Error: "aDocument is undefined" {file: "chrome://browser/content/tabbrowser.xml" line: 346}]

when addressing the nits here, can you do a simple falsy-check (ie if (!docshellRef.document)) before the relevant line in browser.js and return early?

::: browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js
@@ +66,3 @@
>    return function* test_navigate_single_host() {
>      const pref = "browser.fixup.domainwhitelist.localhost";
> +    let win = isPrivate === true ? yield promiseOpenAndLoadWindow({private: true}, true) : window;

Nit: As I noted earlier, just checking if isPrivate is truthy should be fine here.

::: browser/base/content/test/general/head.js
@@ +150,5 @@
>      aCallback(win);
>    }, false);
>  }
>  
> +function promiseNewWindowLoaded(aOptions) {

Nit: this seems to be unused. :-)

@@ +166,5 @@
> +  win.close();
> +  return deferred.promise;
> +}
> +
> +function promiseOpenAndLoadWindow(aOptions, aWaitForDelayedStartup=false) {

I'm going to assume this was copy-pasted appropriately. :-)
Attachment #8470145 - Flags: review?(gijskruitbosch+bugs) → review+
I've addressed the nits from the previous review and added an requestLongerTimeout(2) for the last test. I'll push them on try right now to see if it helped.
Attachment #8470145 - Attachment is obsolete: true
Attachment #8470926 - Flags: review?(gijskruitbosch+bugs)
(In reply to Alex Bardas :alexbardas from comment #11)
> Created attachment 8470926 [details] [diff] [review]
> bug1048513_in_private_browsing_mode_browser.fixup.
> domainwhitelist_should_not_be_saved.diff
> 
> I've addressed the nits from the previous review and added an
> requestLongerTimeout(2) for the last test. I'll push them on try right now
> to see if it helped.

Hmm, looks like no. Also, I was informed earlier today that the timestamps in the log might be a lie. :-(

That's unfortunate because it probably means my inferences about why the test was timing out were wrong.

So let's look at this again. We're timing out on the test after creating and closing the window. This might point to a focus issue (although I'm lost as to why there would be a focus issue specific to Linux64...).

You might be able to use waitForFocus on the first window, after the window.close ?

let deferredFocus = Promise.defer();
SimpleTest.waitForFocus(deferredFocus.resolve, window);
yield deferredFocus.promise;

or something? Maybe/hopefully. Not sure what else could be up, although you could also go back to adding the pageshow listener to see if that helps... Did you do a try run with that version of the patch, and was that green?
Comment on attachment 8470926 [details] [diff] [review]
bug1048513_in_private_browsing_mode_browser.fixup.domainwhitelist_should_not_be_saved.diff

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

(normally, when you get r+, you don't need to re-request review on patches that just include the comments that your reviewer made, but I had a look anyway :-) )

::: browser/base/content/browser.js
@@ +769,5 @@
>    } catch (ex) {
>      return;
>    }
>  
> +  if (!docshellRef || !docshellRef.document)

Nit: don't need !docshellRef, because if that was null we would have returned in the previous try...catch.

::: browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js
@@ +89,5 @@
> +    // In a private window, the notification should appear again.
> +    yield* runURLBarSearchTest(hostName, isPrivate, isPrivate, win);
> +    browser.removeTab(tab);
> +    if (isPrivate) {
> +      yield promiseWindowClosed(win);

To be clear, the extra yield for focus in the main window should go here. :-)

@@ +101,3 @@
>  
>  add_task(function* test_navigate_invalid_url() {
> +  requestLongerTimeout(2);

This didn't help, so I guess we can remove this again... :-\
Attachment #8470926 - Flags: review?(gijskruitbosch+bugs) → review+
Try waiting for main window focus.
Attachment #8470926 - Attachment is obsolete: true
Attachment #8471078 - Flags: review+
I pushed it again here: https://tbpl.mozilla.org/?tree=Try&rev=e43895dc8d5c. If it still doesn't work I'll try different approaches locally. Unfortunately, I don't have a linux machine but I'll submit a request for one :).
It seems that the last patch worked on Linux.
(In reply to Alex Bardas :alexbardas from comment #17)
> It seems that the last patch worked on Linux.

Sweet! I'll land this in a bit, when I'm done with bugmail catchup.
Flags: needinfo?(gijskruitbosch+bugs)
remote:   https://hg.mozilla.org/integration/fx-team/rev/8fb770b10ae8
Flags: needinfo?(gijskruitbosch+bugs) → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/8fb770b10ae8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Iteration: --- → 34.2
Depends on: 1052750
QA Contact: andrei.vaida
Verified fixed on Nightly 34.0a1 (2014-08-13) using Windows 7 64-bit, Mac OS X 10.9.4 and Ubuntu 14.04 LTS 32-bit. 

Leaving the [qa+] keyword in place until this fix lands on Aurora.
Status: RESOLVED → VERIFIED
Alex, can we have an uplift request for aurora? (33). Thanks
Flags: needinfo?(abardas)
Comment on attachment 8471078 [details] [diff] [review]
Do not save browser.fixup.domainwhitelist.* while in private mode.

Approval Request Comment
[Feature/regressing bug #]: bug 693808
[User impact if declined]: private browsing mode saves visited hostnames if you click the info bar to open them instead of a search
[Describe test coverage new/current, TBPL]: has an automated test
[Risks and why]: low, small targeted fix; automated test should have fix from bug 1052750 uplifted, too!
[String/UUID change made/needed]: no
Attachment #8471078 - Flags: approval-mozilla-aurora?
Flags: needinfo?(abardas)
Attachment #8471078 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora 33.0a2 (2014-08-21) as well, using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.3.
QA Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.