Closed Bug 388287 Opened 12 years ago Closed 6 years ago

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

Categories

(Firefox :: Preferences, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: jarod, Assigned: manishearth)

References

Details

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

Attachments

(2 files, 11 obsolete files)

14.17 KB, patch
MattN
: review+
Details | Diff | Splinter Review
2.15 KB, patch
MattN
: review+
Details | Diff | Splinter Review
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.
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
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Component: General → Preferences
Ever confirmed: true
QA Contact: general → preferences
Summary: Proxy setting allows traffic to flow, even if proxy server is not present. → Proxy settings not used if port is 0, dialog should warn user
I see that FF3 has a new port number widget in the UI. That widget could display "" for the pref value "0"...
Attached patch Patch 0.1 (obsolete) — Splinter Review
Fixed: 

Set minimum value to 1 (maximum value was already set). Also set default value to 80. (Should the SOCKS default value be 8080?)
Attachment #795106 - Flags: review?(mnoorenberghe+bmo)
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.
Attachment #795106 - Flags: review?(mnoorenberghe+bmo) → review-
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
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.
Attachment #795130 - Flags: review?(mnoorenberghe+bmo)
Oops, I submitted the reverse diff. Fixed now.
Attachment #795130 - Attachment is obsolete: true
Attachment #795130 - Flags: review?(mnoorenberghe+bmo)
Attachment #795166 - Flags: review?(mnoorenberghe+bmo)
Attachment #795166 - Attachment description: Patch 0.2 (Simple validation with focus on error) → Patch 1.1 (Simple validation with focus on error)
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.
Attachment #795166 - Flags: review?(mnoorenberghe+bmo) → review-
Assignee: nobody → manishsmail
Status: NEW → ASSIGNED
Depends on: 474527
Whiteboard: [mentor=MattN][lang=js]
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.
(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.
Attached patch Patch 1.2 (obsolete) — Splinter Review
Attachment #795106 - Attachment is obsolete: true
Attachment #795166 - Attachment is obsolete: true
Attachment #797732 - Flags: review?(mnoorenberghe+bmo)
(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.
Attachment #797732 - Attachment description: proxerr.patch → Patch 1.2
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
Attached patch Patch 1.3 (obsolete) — Splinter Review
Added condition that an empty proxy address will not throw an error; given that people may want to leave the SOCKS/FTP sections empty.
Attachment #797732 - Attachment is obsolete: true
Attachment #797732 - Flags: review?(mnoorenberghe+bmo)
Attachment #798311 - Flags: review?(mnoorenberghe+bmo)
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.
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 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?
Attachment #798311 - Flags: review?(MattN+bmo) → feedback+
Fixed nits.

Working on the tests now.
Attachment #798311 - Attachment is obsolete: true
Attachment #8368446 - Flags: feedback?(MattN+bmo)
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
Attachment #8368446 - Flags: feedback?(MattN+bmo) → feedback+
Attached patch Basic mochitest (obsolete) — Splinter Review
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.
Attachment #8379489 - Flags: feedback?(MattN+bmo)
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.
Attachment #8379489 - Flags: feedback?(MattN+bmo) → feedback+
(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.
(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 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.
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.
Attached patch Improved mochitest (obsolete) — Splinter Review
I made it work with the preferences dialog (as opposed to the incontent page), fixed the nits, and added the extra test.
Attachment #8379489 - Attachment is obsolete: true
Attachment #8381894 - Flags: review?(MattN+bmo)
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?
Attachment #8381894 - Flags: review?(MattN+bmo) → review+
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 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
Attachment #8381894 - Attachment is obsolete: true
Attachment #8382790 - Flags: feedback?(MattN+bmo)
Attached patch Intermediate patch #2 (obsolete) — Splinter Review
Attachment #8382790 - Attachment is obsolete: true
Attachment #8382790 - Flags: feedback?(MattN+bmo)
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
Attachment #8382790 - Attachment is obsolete: false
Attachment #8382790 - Attachment is obsolete: true
Attachment #8382790 - Flags: feedback+
Attached patch Untested patch (obsolete) — Splinter Review
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)
Attachment #8382796 - Attachment is obsolete: true
Tested patch, it works. Hopefully addressed all the issues that were brought up :)
Attachment #8382821 - Attachment is obsolete: true
Attachment #8383013 - Flags: review?(MattN+bmo)
Sorry for the delay, I'll try get to this tomorrow. I didn't get to it yet due to travel.
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?
Flags: needinfo?(manishearth)
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.
Attachment #8383013 - Flags: review?(MattN+bmo) → review+
Running tests.

https://tbpl.mozilla.org/?tree=Try&rev=44be9b96e0f6
Flags: needinfo?(manishearth)
Attachment #8368446 - Attachment is obsolete: true
Comment on attachment 8390045 [details] [diff] [review]
Final functionality patch 1.5

All tests passed, except for an unrelated timeout.
Attachment #8390045 - Flags: review?(MattN+bmo)
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
Attachment #8390045 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/dce96be376bd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.