767047 - User's original network proxy settings can be overwritten by dialog

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Jason Duell]

We make an effort to save the user's original FTP/HTTPS/etc proxy settings when we (possibly temporarily) overwrite them while the "use this [HTTP] proxy for all protocols" checkbox is set.  We save them into 'backup' prefs, so we can restore them if/when the checkbox gets unset.  However our storage logic blindly backs up the pref every time the dialog is opened/closed, so it will overwrite the backed-up user value with the HTTP proxy value if you open/close the dialog again.

Steps to reproduce:

1) Go to Prefs | Advanced | Network | "Configure how Firefox connects to Internet"

2) Set "manual proxy settings", and enter different values for HTTP (http.com) and FTP (ftp.com).  Save.

3) Open dialog again and select "use this proxy for all protocols".  FTP is greyed out and set to HTTP proxy value now (correct).  Hit OK.   Look in about:config and you'll see 'network.proxy.backup.ftp' is set to 'ftp.com' (also correct).

4) Enter the dialog again: change nothing, just hit "OK".  The backup setting for FTP has now been overwritten to "http.com" (BUG), and that's what is restored if you open the dialog again and uncheck "use this proxy for all protocols".

I believe the fix here is to only set/restore the backup values during the checkbox onChange.

Comment 1

[Jayesh]

Hi,

I am ready to work on this bug... Have understood the bug very well... Being new to "open source contribution" can any one please suggest me how to move forward towards this bug..

I will surely going through the things...

Thanks,
Jayesh

Comment 2

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Fixed user's network settings being replaced

Comment 3

[Nikhil Johny]

The patch by Mahdi Dibaiee, seems to solve the problem. If there is something else can i take it up?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8379676 [details] [diff] [review]
Fixed user's network settings being replaced

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

Thanks for the patch. This seems to work.

Nit: "Fix" is not normally useful in a commit message. The message is a bit vague because it doesn't describe how the problem was fixed and makes the problem sound worse than it is. A better commit message would be: "Bug 767047 - Don't overwrite existing backup proxy preferences"

Could you add a test for this fix? Instructions below:
1) hg cp browser/components/preferences/in-content/tests/browser_connection.js browser/components/preferences/in-content/tests/browser_proxy_backup.js
2) Add browser_proxy_backup.js to browser/components/preferences/in-content/tests/browser.ini
3) In the "test" function of browser_proxy_backup.js, set the prefs appropriately to reproduce the scenario after step 3 of comment 0.
4) Remove runConnectionTests and its reference in the file. 
5) In the domwindowclosed observer, check that the backup pref wasn't overwritten like step 4 of comment 0 mentions.
You can run the test with ./mach mochitest-browser browser/components/preferences/in-content/tests/browser_proxy_backup.js
Please ensure the test fails without this patch and passes with it. You can add your test in a new patch file to make this easier to verify.

Thanks. If you have any questions, flag me for needinfo or feedback (on a WIP patch) to ensure I see it.

Comment 5

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Don't replace existing proxy backup preferences + test; r=MattN

Hi,

Thanks for your tips. I've changed the commit message, and added the tests.

The test works with the patch, and fails without it.

Thanks again.

Comment 6

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Comment on attachment 8381403 [details] [diff] [review]
Don't replace existing proxy backup preferences + test; r=MattN

># HG changeset patch
># Parent 7010ab83a06ebfa3202088bf0609eb0e01e43dc7
># User Mahdi Dibaiee <[e-mail]>
>Bug 767047 - Don't overwrite existing proxy backup preferences; r=MattN
>
>diff --git a/browser/components/preferences/connection.js b/browser/components/preferences/connection.js
>--- a/browser/components/preferences/connection.js
>+++ b/browser/components/preferences/connection.js
>@@ -20,18 +20,18 @@ var gConnectionsDialog = {
>     var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
>     if (shareProxiesPref.value) {
>       var proxyPrefs = ["ssl", "ftp", "socks"];
>       for (var i = 0; i < proxyPrefs.length; ++i) {
>         var proxyServerURLPref = document.getElementById("network.proxy." + proxyPrefs[i]);
>         var proxyPortPref = document.getElementById("network.proxy." + proxyPrefs[i] + "_port");
>         var backupServerURLPref = document.getElementById("network.proxy.backup." + proxyPrefs[i]);
>         var backupPortPref = document.getElementById("network.proxy.backup." + proxyPrefs[i] + "_port");
>-        backupServerURLPref.value = proxyServerURLPref.value;
>-        backupPortPref.value = proxyPortPref.value;
>+        backupServerURLPref.value = backupServerURLPref.value || proxyServerURLPref.value;
>+        backupPortPref.value = backupPortPref.value || proxyPortPref.value;
>         proxyServerURLPref.value = httpProxyURLPref.value;
>         proxyPortPref.value = httpProxyPortPref.value;
>       }
>     }
>     
>     this.sanitizeNoProxiesPref();
>     
>     return true;
>diff --git a/browser/components/preferences/in-content/tests/browser.ini b/browser/components/preferences/in-content/tests/browser.ini
>--- a/browser/components/preferences/in-content/tests/browser.ini
>+++ b/browser/components/preferences/in-content/tests/browser.ini
>@@ -2,14 +2,15 @@
> support-files =
>   head.js
>   privacypane_tests_perwindow.js
> 
> [browser_advanced_update.js]
> [browser_bug410900.js]
> [browser_bug731866.js]
> [browser_connection.js]
>+[browser_proxy_backup.js]
> [browser_healthreport.js]
> skip-if = (!healthreport) || (os == 'linux' && debug)
> [browser_privacypane_1.js]
> [browser_privacypane_3.js]
> [browser_privacypane_5.js]
> [browser_privacypane_8.js]
>diff --git a/browser/components/preferences/in-content/tests/browser_proxy_backup.js b/browser/components/preferences/in-content/tests/browser_proxy_backup.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/preferences/in-content/tests/browser_proxy_backup.js
>@@ -0,0 +1,94 @@
>+/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+Components.utils.import("resource://gre/modules/Services.jsm");
>+
>+function test() {
>+  waitForExplicitFinish();
>+
>+  // network.proxy.type needs to be backed up and restored because mochitest
>+  // changes this setting from the default
>+  let oldNetworkProxyType = Services.prefs.getIntPref("network.proxy.type");
>+  registerCleanupFunction(function() {
>+    Services.prefs.setIntPref("network.proxy.type", oldNetworkProxyType);
>+    Services.prefs.clearUserPref("browser.preferences.instantApply");
>+    Services.prefs.clearUserPref("network.proxy.share_proxy_settings");
>+    Services.prefs.clearUserPref("network.proxy.http");
>+    Services.prefs.clearUserPref("network.proxy.http_port");
>+    Services.prefs.clearUserPref("network.proxy.socks");
>+    Services.prefs.clearUserPref("network.proxy.socks_port");
>+  });
>+
>+  let connectionURI = "chrome://browser/content/preferences/connection.xul";
>+  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
>+                          .getService(Components.interfaces.nsIWindowWatcher);
>+
>+  // instantApply must be true, otherwise connection dialog won't save
>+  // when opened from in-content prefs
>+  Services.prefs.setBoolPref("browser.preferences.instantApply", true);
>+
>+  // set a shared proxy and a timer, we have to open & close the connections dialog 2 times
>+  Services.prefs.setIntPref("network.proxy.type", 1);
>+  Services.prefs.setBoolPref("network.proxy.share_proxy_settings", true);
>+  Services.prefs.setCharPref("network.proxy.http", "testitnow.com");
>+  Services.prefs.setIntPref("network.proxy.http_port", 1200);
>+  Services.prefs.setCharPref("network.proxy.socks", "127.0.0.1");
>+  Services.prefs.setIntPref("network.proxy.socks_port", 9050);
>+  let times = 0;
>+
>+  // this observer is registered after the pref tab loads
>+  let observer = {
>+    observe: function(aSubject, aTopic, aData) {
>+
>+      if (aTopic == "domwindowopened") {
>+        // when connection window loads, run tests, acceptDialog() and increase times
>+        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
>+        win.addEventListener("load", function winLoadListener() {
>+          win.removeEventListener("load", winLoadListener, false);
>+          if (win.location.href == connectionURI) {
>+            ok(true, "connection window opened");
>+            win.document.documentElement.acceptDialog();
>+            times++;
>+          }
>+        }, false);
>+      } else if (aTopic == "domwindowclosed") {
>+        // finish up when connection window closes
>+        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
>+        if (win.location.href == connectionURI) {
>+          windowWatcher.unregisterNotification(observer);
>+          ok(true, "connection window closed");
>+          // the proxy backup should not be replaced
>+          is(Services.prefs.getCharPref("network.proxy.backup.socks"), "127.0.0.1", "Shared proxy backup shouldn't be replaced");
>+
>+          gBrowser.removeCurrentTab();
>+          // after the second run, finish the test
>+          if( times < 2 ) {
>+            open_preferences(function tabOpened(aContentWindow) {
>+              is(gBrowser.currentURI.spec, "about:preferences", "about:preferences loaded");
>+              windowWatcher.registerNotification(observer);
>+              gBrowser.contentWindow.gAdvancedPane.showConnections();
>+            });
>+          }
>+          else {
>+            finish();
>+          }
>+        }
>+      }
>+
>+    }
>+  }
>+
>+  /*
>+  The connection dialog alone won't save onaccept since it uses type="child",
>+  so it has to be opened as a sub dialog of the main pref tab.
>+  Open the main tab here.
>+  */
>+  open_preferences(function tabOpened(aContentWindow) {
>+    is(gBrowser.currentURI.spec, "about:preferences", "about:preferences loaded");
>+    windowWatcher.registerNotification(observer);
>+    gBrowser.contentWindow.gAdvancedPane.showConnections();
>+  });
>+}
>+

Comment 7

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Attachment: Don't overwrite existing proxy backup preferences; r=MattN

Sorry about the last post, it was a mistake.

I forgot to mention the reviewer name in the commit.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8382410 [details] [diff] [review]
Don't overwrite existing proxy backup preferences; r=MattN

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

I ended up making the changes for you so I will push this to try to check that it passes on all platforms and then I push it to inbound.

Thank you very much for your patch!

::: browser/components/preferences/in-content/tests/browser.ini
@@ +6,5 @@
>  [browser_advanced_update.js]
>  [browser_bug410900.js]
>  [browser_bug731866.js]
>  [browser_connection.js]
> +[browser_proxy_backup.js]

Nit: alphabetical order

::: browser/components/preferences/in-content/tests/browser_proxy_backup.js
@@ +28,5 @@
> +  // instantApply must be true, otherwise connection dialog won't save
> +  // when opened from in-content prefs
> +  Services.prefs.setBoolPref("browser.preferences.instantApply", true);
> +
> +  // set a shared proxy and a timer, we have to open & close the connections dialog 2 times

Opening the dialog twice was relying on implementation details that the backup would be created even when the share checkbox wasn't toggled. I simplified the test to set the backup prefs in advance so it's simply testing the specific issue with the dialog opening and closing once.

@@ +31,5 @@
> +
> +  // set a shared proxy and a timer, we have to open & close the connections dialog 2 times
> +  Services.prefs.setIntPref("network.proxy.type", 1);
> +  Services.prefs.setBoolPref("network.proxy.share_proxy_settings", true);
> +  Services.prefs.setCharPref("network.proxy.http", "testitnow.com");

We try to avoid using new domains in tests to avoid accidentally referring to a real website and perhaps making network requests to their servers. It's best to stick to ones on the proxy list at http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt such as example.com

@@ +59,5 @@
> +        if (win.location.href == connectionURI) {
> +          windowWatcher.unregisterNotification(observer);
> +          ok(true, "connection window closed");
> +          // the proxy backup should not be replaced
> +          is(Services.prefs.getCharPref("network.proxy.backup.socks"), "127.0.0.1", "Shared proxy backup shouldn't be replaced");

The port should also be checked

@@ +63,5 @@
> +          is(Services.prefs.getCharPref("network.proxy.backup.socks"), "127.0.0.1", "Shared proxy backup shouldn't be replaced");
> +
> +          gBrowser.removeCurrentTab();
> +          // after the second run, finish the test
> +          if( times < 2 ) {

Remove the spaces surrounding the condition and put a space after |if|

@@ +64,5 @@
> +
> +          gBrowser.removeCurrentTab();
> +          // after the second run, finish the test
> +          if( times < 2 ) {
> +            open_preferences(function tabOpened(aContentWindow) {

We don't need to open about:preferences again, do we? Only open the connection dialog.

@@ +70,5 @@
> +              windowWatcher.registerNotification(observer);
> +              gBrowser.contentWindow.gAdvancedPane.showConnections();
> +            });
> +          }
> +          else {

Nit: cuddle the else like so: } else {

Comment 9

[Matthew N. [:MattN]]

Attachment: v.2 Don't overwrite existing proxy backup preferences; r=MattN

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f33f5502869b

Comment 10

[Matthew N. [:MattN]]

I meant to include debug builds: https://tbpl.mozilla.org/?tree=Try&rev=90e38b3a0352

Comment 11

[Matthew N. [:MattN]]

Tests passed on try so I pushed it to the fx-team repo for integration:

https://hg.mozilla.org/integration/fx-team/rev/a1e0b4161121

Thanks

Comment 12

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

Hi, thank you for mentoring me on this bug Matthew.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a1e0b4161121

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0


Verified fixed on latest Aurora 30.0a2 (buildID: 20140424004002)