388287 - Proxy settings not used if port is 0, dialog should warn user

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Jarod]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4

Setting a manual proxy to 127.0.0.1 and the port as 0, the traffic requests are handled normally, and an error is not presented. Even though there is not a proxy server running on the local machine.

Reproducible: Always

Steps to Reproduce:
1.Set a manual proxy server to 127.0.0.1 port 0
2.Browse to a website
Actual Results:  
The website is loaded like normal.

Expected Results:  
An error reporting a connection to the proxy server could not be made.

Comment 1

[Marco Bonardo [:mak]]

port 0 is invalid, so if it is inserted it's discarded and proxy is not used...
Probably the dialog could be modified so the user cannot insert a value <=0

Comment 2

[benc]

I see that FF3 has a new port number widget in the UI. That widget could display "" for the pref value "0"...

Comment 3

[Manish Goregaokar [:manishearth]]

Attachment: Patch 0.1

Fixed: 

Set minimum value to 1 (maximum value was already set). Also set default value to 80. (Should the SOCKS default value be 8080?)

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 795106 [details] [diff] [review]
Patch 0.1

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

::: browser/components/preferences/connection.xul
@@ +82,5 @@
>                <hbox align="center">
>                  <textbox id="networkProxyHTTP" flex="1"
>                           preference="network.proxy.http" onsyncfrompreference="return gConnectionsDialog.readHTTPProxyServer();"/>
>                  <label value="&port.label;" accesskey="&HTTPport.accesskey;" control="networkProxyHTTP_Port"/>
> +                <textbox id="networkProxyHTTP_Port" type="number" min="1" max="65535" value="80" size="5"

First off, thanks for the patch. I think we should consider a different approach though.

The problem with setting a minimum is that it then becomes the new default for <textbox type="number"> which likely doesn't address the underlying problem that the bug is trying to solve: the user sets a hostname but no port.

I believe you're trying to workaround this by specifying defaults other than "1" but as far as I know, the ports for proxy servers vary widely so I don't think a default of 80 helps the user.

I think we should ideally default to an empty field and use HTML5 form validation to indicate when the port specified is invalid. This will be easier once bug 344616 is fixed.

In the meantime, if we don't want to wait, I think we should leave the default as 0 but add custom validation when a port is 0 and a proxy host is specified. It doesn't seem like setCustomValidity works on the anonymous html:input element unfortunately so this will require figuring out the UX. Perhaps simply focusing the first invalid port field when OK is clicked will be enough?

I'm open to other ideas.

Comment 5

[Manish Goregaokar [:manishearth]]

Thanks for replying.

Well, to me the issue was primarily that using port 0 just ignores the proxy settings, and instead of giving a "Cannot connect to proxy server" or "Proxy server refusing connections" error, one will get a more generic "Connection timed out" or "Could not find" type error (depending on how the network is set up in the case of DIRECT connections).

So if one has not specified the port, one gets a generic error that could mean _anything_ -- Internet down, Intranet down, Proxy details wrong, etc. And while the error messages mention "If your computer is connected to a firewall or proxy, make sure that FF is permitted to access the web" (which gives one the impression that they need to call the sysad), they don't explicitly mention checking proxy settings -- because this error is not supposed to occur due to a misconfigured FF proxy.

On the other hand, the "Proxy server refusing connections" error explicitly mentions "Check the proxy settings to make sure that they are correct". This is pretty clear, so if a user with proxy port 8080 forgets to change the "default" 80, they will get an error that tells them exactly where to look. Currently, that doesn't happen, but with the above fix, it does. Besides, when given proxy settings, the port is almost always mentioned.



--------

Of course, it would be nice to improve the UX to cover the most probable issues arising from bad input. I'll try making a "focus on the bad element" patch. It seems simple enough; I just need to add a clause somewhere here[1] that checks for port 0 and focuses + returns false in that case.

Another idea is to "disable the OK button". This is the behavior that I'm used to on Ubuntu. This may be more confusing, however, since there is no indication of *where* the error is.

HTML5 form validation is a great idea, but I don't think it works in XUL. Ideally, we ought to add an extra attribute "validate" which contains a regex (or a function, perhaps), and the form does not submit if the element doesn't validate. HTML5-esque validation with custom messages and all would be even better.


[1]: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/connection.js#7

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Patch 0.2 (Simple validation with focus on error)

If (at least) one of the ports is zero while enabled, the form submission will be cancelled, with cursor focusing on the first problematic port.

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.1 (Simple validation with focus on error)

Oops, I submitted the reverse diff. Fixed now.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 795166 [details] [diff] [review]
Patch 1.1 (Simple validation with focus on error)

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

Have you considered whether we need to validate the port if the server field is empty?

Were you able to manually test this patch? If not, we can help you get setup with that in #introduction on irc.mozilla.org.

::: a/browser/components/preferences/connection.js
@@ +18,5 @@
>      var httpProxyURLPref = document.getElementById("network.proxy.http");
>      var httpProxyPortPref = document.getElementById("network.proxy.http_port");
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
> +    if (httpProxyPortPref.value == 0) {
> +      document.getElementById("network.proxy.http_port").focus(); // (validation)  If the port is 0, focus on the port and cancel submission

This is focusing the <preference> element but it should be focusing the <textbox> AFAICT as it doesn't work for me.
Please move the comment to a new line above this one.

I think it would be cleaner to add a new loop over proxyPrefs before the existing loop below to validate the ports. That way we don't need to duplicate the check and focus call.

@@ +19,5 @@
>      var httpProxyPortPref = document.getElementById("network.proxy.http_port");
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
> +    if (httpProxyPortPref.value == 0) {
> +      document.getElementById("network.proxy.http_port").focus(); // (validation)  If the port is 0, focus on the port and cancel submission
> +      return false;

This doesn't work due to bug 474527. Do you want to try to update that patch to work with the tip of mozilla-central? I'm pretty sure Gavin won't mind.

Comment 9

[Manish Goregaokar [:manishearth]]

Yeah, I hadn't been able to test it yet. However, I just recently got a working build (I wanted to fix Bug 910670, and thought that it was high time I started testing these things), so I'll test this out on that build later today after fixing the issues listed above..



AFAICT  bug 474527 only applies to custom (extension) prefwindows, but I'll test it out anyway. Can't guarantee that I'll be able to fix it, as I'm still new to the codebase.

Comment 10

[Matthew N. [:MattN]]

(In reply to Manish Goregaokar [:manishearth] from comment #9)
> Yeah, I hadn't been able to test it yet. However, I just recently got a
> working build (I wanted to fix Bug 910670, and thought that it was high time
> I started testing these things), so I'll test this out on that build later
> today after fixing the issues listed above..

OK, cool.

> AFAICT  bug 474527 only applies to custom (extension) prefwindows, but I'll
> test it out anyway. Can't guarantee that I'll be able to fix it, as I'm
> still new to the codebase.

I updated the summary, it applies to all prefWindows. There is an existing patch so you can start by just making that apply cleanly again and then running the new tests locally with mach to see if they fail.

Comment 11

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.2

Comment 12

[Manish Goregaokar [:manishearth]]

(In reply to Matthew N. [:MattN] from comment #10)

I *think* I fixed the other bug. However, running `./mach mochitest-chrome toolkit/content/tests/chrome/test_preferences_beforeaccept.xul` gives me the error "Mochitest ERROR | Timed out while waiting for server startup." This doesn't seem to be an error with the changes I made, just with my method of running the tests.

I have run mach build on the entire directory, and I have run make on the various object directories that were changed.

Comment 13

[Manish Goregaokar [:manishearth]]

Somehow make wasn't quite cutting it, but `./mach build toolkit/content/tests/chrome` followed by  `./mach mochitest-chrome toolkit/content/tests/chrome/test_preferences_beforeaccept.xul` worked.


Patch 1.2 above should work, as long as attachment 797731 [details] [diff] [review] (Patch 1.0) on bug 474527 is applied first

Comment 14

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.3

Added condition that an empty proxy address will not throw an error; given that people may want to leave the SOCKS/FTP sections empty.

Comment 15

[Manish Goregaokar [:manishearth]]

Updates on this?

Comment 16

[Manish Goregaokar [:manishearth]]

Matt: re: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=388287&attachment=795166

I think you looked at the wrong patch (https://bugzilla.mozilla.org/attachment.cgi?id=798311&action=diff)

I had tested this before by queueing it on top of Gavin's patch with changes (the last patch I had put up on bug 474527), it worked then. Ought to work now, too, but I'll have to check it again.

Comment 17

[Manish Goregaokar [:manishearth]]

Tested new patch while applied to tip, it seems to work

 * It halts closing if the port is 0
 * It does not halt if the host field is empty
 * It does not halt in case of disabled prefs (by selecting a different proxy pref option or by using the "use for all protocols") button.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 798311 [details] [diff] [review]
Patch 1.3

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

Please add a test for this behaviour. See browser/components/preferences/in-content/tests/browser_connection.js for a test of the same dialog that you can copy and modify.

::: browser/components/preferences/connection.js
@@ +18,5 @@
>      var httpProxyURLPref = document.getElementById("network.proxy.http");
>      var httpProxyPortPref = document.getElementById("network.proxy.http_port");
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
> +    var proxyPrefs =  ["http","ssl", "ftp", "socks"];
> +    

Nit: please remove the trailing whitespace you added here and elsewhere in your changes. You can look into having your editor highlight this for you.

@@ +19,5 @@
>      var httpProxyPortPref = document.getElementById("network.proxy.http_port");
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
> +    var proxyPrefs =  ["http","ssl", "ftp", "socks"];
> +    
> +    //Validation: If the port is 0, focus on the port  and cancel submission

Nit: Add a space after // in comments (below too)

@@ +20,5 @@
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
> +    var proxyPrefs =  ["http","ssl", "ftp", "socks"];
> +    
> +    //Validation: If the port is 0, focus on the port  and cancel submission
> +    for (var i = 0; i < proxyPrefs.length; ++i) {

Please use a for…of loop since we now support them. It will get rid of the magic number in "i==0" inside.

@@ +22,5 @@
> +    
> +    //Validation: If the port is 0, focus on the port  and cancel submission
> +    for (var i = 0; i < proxyPrefs.length; ++i) {
> +	  var proxyPortPref = document.getElementById("network.proxy." + proxyPrefs[i] + "_port")
> +	  var proxyPref = document.getElementById("network.proxy." + proxyPrefs[i])

Nit: Fix the indentation here

@@ +38,2 @@
>        for (var i = 0; i < proxyPrefs.length; ++i) {
> +        var proxyPortPref = document.getElementById("network.proxy." + proxyPrefs[i] + "_port");

Did you intentionally move this line?

Comment 19

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.4: nits fixed; no tests yet

Fixed nits.

Working on the tests now.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8368446 [details] [diff] [review]
Patch 1.4: nits fixed; no tests yet

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

This is much better.

::: browser/components/preferences/connection.js
@@ +19,5 @@
>      var httpProxyPortPref = document.getElementById("network.proxy.http_port");
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
> +    var proxyPrefs =  ["http","ssl", "ftp", "socks"];
> +
> +    // Validation: If the port is 0, focus on the port  and cancel submission

Nit: There are two spaces between "port" and "and".

@@ +20,5 @@
>      var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
> +    var proxyPrefs =  ["http","ssl", "ftp", "socks"];
> +
> +    // Validation: If the port is 0, focus on the port  and cancel submission
> +    for (let prefName of  proxyPrefs) {

Just inline the proxyPrefs array here to avoid the variable name collision.
Nit: there are two spaces here.

@@ +22,5 @@
> +
> +    // Validation: If the port is 0, focus on the port  and cancel submission
> +    for (let prefName of  proxyPrefs) {
> +      var proxyPortPref = document.getElementById("network.proxy." + prefName + "_port")
> +      var proxyPref = document.getElementById("network.proxy." + prefName)

Use |let| instead of |var| here.

@@ +25,5 @@
> +      var proxyPortPref = document.getElementById("network.proxy." + prefName + "_port")
> +      var proxyPref = document.getElementById("network.proxy." + prefName)
> +      // Only worry about ports which are currently active. If the share option is on, then ignore all ports except the HTTP port
> +      if (proxyPref.value != "" && proxyPortPref.value == 0 && (prefName=="http" || !shareProxiesPref.value)) {
> +         document.getElementById("networkProxy"+prefName.toUpperCase()+"_Port").focus();

Nit: put spaces around operators such as " + " and " == ".

@@ +43,5 @@
>          backupPortPref.value = proxyPortPref.value;
>          proxyServerURLPref.value = httpProxyURLPref.value;
>          proxyPortPref.value = httpProxyPortPref.value;
>        }
> +    }  

Nit: there is still a whitespace change here

Comment 21

[Manish Goregaokar [:manishearth]]

Attachment: Basic mochitest

This is a basic mochitest (requires the other patch to be applied first to work)

It spawns the dialog multiple times and puts it through various combinations of the preferences, and tries to close it. The test will fail if the dialog closes when it's not supposed to. The test will timeout if the dialog fails to close when it should.

If necessary I can easily add more testcases to the runConnectionTestsGen function, I wasn't sure which I need to add.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8379489 [details] [diff] [review]
Basic mochitest

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

::: browser/components/preferences/tests/browser.ini
@@ +5,5 @@
>  
>  [browser_advanced_update.js]
>  [browser_bug410900.js]
>  [browser_bug705422.js]
> +[browser_connection_bug388287.js]

Note that there is also the in-content directory which has the tests duplicated (unfortunately). We can deal with that once we have this r+ on this version but keep it in mind.

::: browser/components/preferences/tests/browser_connection_bug388287.js
@@ +1,1 @@
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */

This isn't our usual mode line from https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

@@ +1,2 @@
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public

FYI: You can put tests in the public domain. See http://www.mozilla.org/MPL/headers/

@@ +7,5 @@
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +// From browser/components/preferences/in-content/test/head.js
> +function open_preferences(aCallback) {
> +  gBrowser.selectedTab = gBrowser.addTab("about:preferences");

Um, I guess this is a test for the in-content preferences, but it's in the wrong directory?

@@ +22,5 @@
> +
> +
> +function test() {
> +  waitForExplicitFinish();
> +  let connectionTests=runConnectionTestsGen();

Nit: put spaces around operators here and throughout the file

@@ +28,5 @@
> +  let final=false;
> +  let connectionURI = "chrome://browser/content/preferences/connection.xul";
> +  let closeable=false;
> +  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
> +                       .getService(Components.interfaces.nsIWindowWatcher);

You can use Services.ww instead and then you may not need the local variable

@@ +46,5 @@
> +      } else if (aTopic == "domwindowclosed") {
> +        // Check if the window should have closed, and respawn another window for further testing
> +        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> +        if (win.location.href == connectionURI) {
> +          ok(closeable,"Connection dialog closed");

Nit: Spaces between arguments please

@@ +68,5 @@
> +
> +
> +  // The actual tests to run, in a generator
> +  function* runConnectionTestsGen() {
> +    let doc,connectionWin,proxyTypePref,sharePref,httpPref,httpPortPref,ftpPref,ftpPortPref;

Nit: Put spaces after the commas

@@ +104,5 @@
> +    ftpPortPref.value = 0;
> +    doc.documentElement.acceptDialog();
> +
> +    // From now on, the dialog should close since we are giving it legitimate inputs
> +    // Test will timeout if the onbeforeaccept kicks in erroneously

Can you double-check that this fails if the window doesn't actually close? e.g. just comment out a doc.documentElement.acceptDialog(); below.

@@ +107,5 @@
> +    // From now on, the dialog should close since we are giving it legitimate inputs
> +    // Test will timeout if the onbeforeaccept kicks in erroneously
> +    closeable=true;
> +
> +    // Both ports 80, share on

Can you make one with httpPref.value = ""; and httpPortPref.value = 0; and make sure it is accepted? This is to make sure we ignore the port when no host is set as I didn't see that covered.

@@ +133,5 @@
> +    doc.documentElement.acceptDialog();
> +    yield null;
> +  }
> +
> +  // this observer is registered after the pref tab loads

I think this is supposed to go above the observer object.

Comment 23

[Manish Goregaokar [:manishearth]]

(In reply to Matthew N. [:MattN] from comment #22)
> Note that there is also the in-content directory which has the tests
> duplicated (unfortunately). We can deal with that once we have this r+ on
> this version but keep it in mind.



> This isn't our usual mode line from
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#Mode_Line
> [snip]
> FYI: You can put tests in the public domain. See
> http://www.mozilla.org/MPL/headers/
> [snip]
> Um, I guess this is a test for the in-content preferences, but it's in the
> wrong directory?

So, I'd copied the test from one of the tests in the incontent folder, causing the above issues. It seemed better to have just one window popping up instead of dealing with the prefwidow too. I can make it work with non-incontent prefwindows if necessary.

(Note that the connection pref pane has no incontent counterpart, it is just popped up when incontent is on)


I'll put up a new patch in a while.

Comment 24

[Matthew N. [:MattN]]

(In reply to Manish Goregaokar [:manishearth] from comment #23)
> (Note that the connection pref pane has no incontent counterpart, it is just
> popped up when incontent is on)

I'm not sure what you mean by that. I can get to the proxy dialog in both the current and in-content preferences.

Comment 25

[Manish Goregaokar [:manishearth]]

(In reply to Matthew N. [:MattN] from comment #24)
> (In reply to Manish Goregaokar [:manishearth] from comment #23)
> > (Note that the connection pref pane has no incontent counterpart, it is just
> > popped up when incontent is on)
> 
> I'm not sure what you mean by that. I can get to the proxy dialog in both
> the current and in-content preferences.

In the sense that the connection pane is *always* a popup, regardless of the incontent pref.

Comment 26

[Manish Goregaokar [:manishearth]]

Tested the removal of an acceptDialog(). It takes 20 seconds, but it gives an error:

 0:20.81 TEST-PASS | chrome://mochitests/content/browser/browser/components/preferences/tests/browser_connection_bug388287.js | Connection dialog closed
 0:21.52 TEST-PASS | chrome://mochitests/content/browser/browser/components/preferences/tests/browser_connection_bug388287.js | Connection dialog closed
 0:49.34 Failed to retrieve MOZ_UPLOAD_DIR env var
 0:49.34 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/preferences/tests/browser_connection_bug388287.js | Test timed out


the failed test takes a whole minute to clean up after itself, and it emits a bunch of other fail errors on the way. So that works.

Comment 27

[Manish Goregaokar [:manishearth]]

Attachment: Improved mochitest

I made it work with the preferences dialog (as opposed to the incontent page), fixed the nits, and added the extra test.

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 8381894 [details] [diff] [review]
Improved mochitest

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

Thanks, This looks good! Would you mind doing doing the in-content version? Just fix the nits below, copy the test and add to the other ini file.

::: browser/components/preferences/tests/browser_connection_bug388287.js
@@ +9,5 @@
> +  let connectionTests = runConnectionTestsGen();
> +  connectionTests.next();
> +  let final = false;
> +  let connectionURI = "chrome://browser/content/preferences/connection.xul";
> +  let preferencesURI = "chrome://browser/content/preferences/preferences.xul";

Nit: use const instead of let for the URLs. Also, I realize you inherited this from browser_connection.js but we normally call strings "URLs" and leave "URIs" for nsIURI's so change these to "-URL"

@@ +10,5 @@
> +  connectionTests.next();
> +  let final = false;
> +  let connectionURI = "chrome://browser/content/preferences/connection.xul";
> +  let preferencesURI = "chrome://browser/content/preferences/preferences.xul";
> +  let closeable = false;

Nit: put closeable and final adjacent to each other since they server similar purposes

@@ +12,5 @@
> +  let connectionURI = "chrome://browser/content/preferences/connection.xul";
> +  let preferencesURI = "chrome://browser/content/preferences/preferences.xul";
> +  let closeable = false;
> +  let prefWin;
> +  // this observer is registered after the pref tab loads

Nit: newline above please

@@ +15,5 @@
> +  let prefWin;
> +  // this observer is registered after the pref tab loads
> +  let observer = {
> +    observe: function(aSubject, aTopic, aData) {
> +

Nit: remove this newline

@@ +17,5 @@
> +  let observer = {
> +    observe: function(aSubject, aTopic, aData) {
> +
> +      if (aTopic == "domwindowopened") {
> +        // when connection window loads, proceed forward in test

Nit: When the connection window loads, proceed with the test.

@@ +20,5 @@
> +      if (aTopic == "domwindowopened") {
> +        // when connection window loads, proceed forward in test
> +        let win = aSubject.QueryInterface(Components.interfaces.nsIDOMWindow);
> +        win.addEventListener("load", function winLoadListener() {
> +          win.removeEventListener("load", winLoadListener, false);

Nit: remove the optional third argument.

@@ +24,5 @@
> +          win.removeEventListener("load", winLoadListener, false);
> +          if (win.location.href == connectionURI) {
> +            // If this is a connection window, run the next test
> +            connectionTests.next(win);
> +          }else if(win.location.href == preferencesURI) {

Nit: put spaces before |else| and after |if|

@@ +29,5 @@
> +            // If this is the preferences window, initiate the tests by showing the connection pane
> +            prefWin = win;
> +            prefWin.gAdvancedPane.showConnections();
> +
> +            // Since the above method immediately triggers the observer chain, 

Nit: trailing space

@@ +34,5 @@
> +            // the cleanup below won't happen until all the tests finish successfully.
> +            prefWin.close();
> +            finish();
> +          }
> +        }, false);

Nit: remove the optional third argument

@@ +42,5 @@
> +        if (win.location.href == connectionURI) {
> +          ok(closeable, "Connection dialog closed");
> +
> +          // Last close event, don't respawn
> +          if(final){

Nit: missing spaces before and after the brackets

@@ +51,5 @@
> +          // Open another connection pane for the next test
> +          prefWin.gAdvancedPane.showConnections();
> +        }
> +      }
> +

Nit: remove blank line

@@ +93,5 @@
> +    ftpPortPref.value = 0;
> +    doc.documentElement.acceptDialog();
> +
> +    // From now on, the dialog should close since we are giving it legitimate inputs
> +    // Test will timeout if the onbeforeaccept kicks in erroneously

Nit: put a period at the end of the sentences. s/Test/The test/

@@ +114,5 @@
> +
> +    // HTTP host empty, port 0 with share off
> +    setDoc(yield null);
> +    proxyTypePref.value = 1;
> +    sharePref.value = true;

The comment says "share off" but this looks on to me.

@@ +125,5 @@
> +    proxyTypePref.value = 0;
> +    sharePref.value = true;
> +    httpPref.value = "localhost";
> +    httpPortPref.value = 0;
> +    final = true; // This is the final test, don't spawn another connection window

Perhaps add a new line above this one to make it stand out more as something that needs to move down if a test gets added?

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8381894 [details] [diff] [review]
Improved mochitest

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

::: browser/components/preferences/tests/browser_connection_bug388287.js
@@ +3,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function test() {

I just realized you forgot the cleanup function from browser_connection.js to make sure network.proxy.type gets reset

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8381894 [details] [diff] [review]
Improved mochitest

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

::: browser/components/preferences/tests/browser.ini
@@ +3,5 @@
>    head.js
>    privacypane_tests_perwindow.js
>  
>  [browser_advanced_update.js]
> +[browser_connection_bug388287.js]

This list should be alphabetical order

Comment 31

[Manish Goregaokar [:manishearth]]

Attachment: Intermediate patch -- fixed mochitest, no incontent test yet

Comment 32

[Manish Goregaokar [:manishearth]]

Attachment: Intermediate patch #2

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8382790 [details] [diff] [review]
Intermediate patch -- fixed mochitest, no incontent test yet

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

::: browser/components/preferences/tests/browser_connection_bug388287.js
@@ +17,5 @@
> +  // The changed preferences need to be backed up and restored because this mochitest
> +  // changes them setting from the default
> +  let oldNetworkProxyType = Services.prefs.getIntPref("network.proxy.type");
> +  let oldNetworkProxyShare = Services.prefs.getIntPref("network.proxy.share_proxy_settings");
> +  let oldNetworkProxyHTTP = Services.prefs.getCharPref("network.proxy.http");

As discussed on IRC, it's probably fine to just clearUserPref on most of these.

Also, use a loop to cleanup all types including the backups sine you are toggling the share preference which will write to them.

@@ +61,5 @@
> +          ok(closeable, "Connection dialog closed");
> +
> +          // Last close event, don't respawn
> +          if(final){
> +            Services.ww.unregisterNotification(observer);

Maybe add this line to registerCleanupFunction too but you may need to wrap it in a try…catch

Comment 34

[Manish Goregaokar [:manishearth]]

Attachment: Untested patch

This patch *should* work, but I've not been able to test it yet since for some reason my mach doesn't copy new tests on incremental builds (so this may take a while)

Comment 35

[Manish Goregaokar [:manishearth]]

Attachment: Mochitests for both incontent and normal prefs

Tested patch, it works. Hopefully addressed all the issues that were brought up :)

Comment 36

[Matthew N. [:MattN]]

Sorry for the delay, I'll try get to this tomorrow. I didn't get to it yet due to travel.

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8368446 [details] [diff] [review]
Patch 1.4: nits fixed; no tests yet

Manish, can you upload the fixed version of the patch and flag me for review?

Comment 38

[Matthew N. [:MattN]]

Comment on attachment 8383013 [details] [diff] [review]
Mochitests for both incontent and normal prefs

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

r=me assuming tests pass. I'll push them to try when the updated patch is attached.

Sorry for the delay.

Comment 39

[Manish Goregaokar [:manishearth]]

Running tests.

https://tbpl.mozilla.org/?tree=Try&rev=44be9b96e0f6

Comment 40

[Manish Goregaokar [:manishearth]]

Attachment: Final functionality patch 1.5

Comment 41

[Manish Goregaokar [:manishearth]]

Comment on attachment 8390045 [details] [diff] [review]
Final functionality patch 1.5

All tests passed, except for an unrelated timeout.

Comment 42

[Matthew N. [:MattN]]

Comment on attachment 8390045 [details] [diff] [review]
Final functionality patch 1.5

Thanks Manish!

Pushed with minor style fixes: https://hg.mozilla.org/integration/fx-team/rev/dce96be376bd

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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