Steps to reproduce: 1. Register a PAC script via browser.proxy.registerProxyScript('pac.js'): // this is pac.js function FindProxyForURL(host, url) { // Some unreachable proxy, followed by "DIRECT". return 'PROXY 127.0.0.1:12345; DIRECT ignore-this-work-around-for-bugzil.la/1355198'; } 2. Visit a URL (e.g. example.com) (MAKE SURE THAT THIS PAGE IS NOT IN YOUR BROWSER CACHE). Expected behavior: - The actual page is shown (=direct request). Actual behavior: - The request is blocked, "The proxy server is refusing connections" Additional information: https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/toolkit/components/extensions/ProxyScriptContext.jsm#220 Instead of "return null", it should return ProxyService.newProxyInfo('direct', '', '', 0, PROXY_TIMEOUT_SEC, null); (or maybe defaultProxyInfo from applyFilter?) Note that if the return value is "DIRECT", that the default handler is used, because "proxyInfo" is null, and then "proxyInfo || defaultProxyInfo;" results in a fall back to the defaultProxyInfo (https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/toolkit/components/extensions/ProxyScriptContext.jsm#144).

Comment on attachment 8882460 [details] Bug 1377325 - Refactor ProxyScriptContext.js https://reviewboard.mozilla.org/r/153584/#review159188 ::: toolkit/components/extensions/ProxyScriptContext.jsm:145 (Diff revision 1) > + if (rule.type === PROXY_TYPES.DIRECT) { > + break; > + } > + rules.push(rule); nit: please move this out of the try/catch. ::: toolkit/components/extensions/ProxyScriptContext.jsm:151 (Diff revision 1) > + break; > + } > + rules.push(rule); > + } catch (e) { > + this.extension.emit("proxy-error", { > + message: "FindProxyForURL: " + (e.message || e), nit: I don't think `e.message` will ever be undefined. Do you see a case where it will? ::: toolkit/components/extensions/ProxyScriptContext.jsm:157 (Diff revision 1) > let proxyInfo = this.createProxyInfo(rules); > > return proxyInfo || defaultProxyInfo; Based on my suggestion below, this would be simplified to `return this.createProxyInfo(rules, defaultProxyInfo)`. ::: toolkit/components/extensions/ProxyScriptContext.jsm:163 (Diff revision 1) > * Creates a new proxy info object using the return value of FindProxyForURL. > * > * @param {Array<string>} rules The list of proxy rules returned by FindProxyForURL. > * (e.g. ["PROXY 1.2.3.4:8080", "SOCKS 1.1.1.1:9090", "DIRECT"]) We should update this jsdoc now that the structure of the `rules` param has changed. ::: toolkit/components/extensions/ProxyScriptContext.jsm:170 (Diff revision 1) > * @param {Array<string>} rules The list of proxy rules returned by FindProxyForURL. > * (e.g. ["PROXY 1.2.3.4:8080", "SOCKS 1.1.1.1:9090", "DIRECT"]) > * @returns {nsIProxyInfo} The proxy info to apply for the given URI. > */ > createProxyInfo(rules) { > - if (!rules.length) { > + let proxyInfo = null; I think we should pass in defaultProxyInfo and set proxyInfo to defaultProxyInfo here - that way the last failover proxy is always defaultProxyInfo. ::: toolkit/components/extensions/ProxyScriptContext.jsm:175 (Diff revision 1) > - if (!rules.length) { > - return null; > + let proxyInfo = null; > + while (rules.length) { > + let {type, host, port} = rules.pop(); > + proxyInfo = ProxyService.newProxyInfo( > + type, host, port, 0, PROXY_TIMEOUT_SEC, proxyInfo); > + if (type === "DIRECT") { nit: please use PROXY_TYPES.DIRECT here.

Comment on attachment 8882460 [details] Bug 1377325 - Refactor ProxyScriptContext.js https://reviewboard.mozilla.org/r/153584/#review159922 ::: toolkit/components/extensions/ProxyScriptContext.jsm:157 (Diff revision 1) > let proxyInfo = this.createProxyInfo(rules); > > return proxyInfo || defaultProxyInfo; This change is done in the next commit, because doing so results in a behavioral change in functionality. ::: toolkit/components/extensions/ProxyScriptContext.jsm:175 (Diff revision 1) > - if (!rules.length) { > - return null; > + let proxyInfo = null; > + while (rules.length) { > + let {type, host, port} = rules.pop(); > + proxyInfo = ProxyService.newProxyInfo( > + type, host, port, 0, PROXY_TIMEOUT_SEC, proxyInfo); > + if (type === "DIRECT") { `type` should have been compared with lowercase `"direct"` (`PROXY_TYPES.DIRECT` is also lowercase "direct"). There is something wrong with this logic, I think that you should first iterate forwards over the array and stop at the first occurrence of "direct", and then iterate backwards as currently done. Otherwise, if you have `PROXY xxx; DIRECT; PROXY yyy`, then the implementation will start with `yyy` and then stop, instead of starting with `xxx` (please add a test for this too).

Comment on attachment 8882461 [details] Bug 1377325 - Refactor ProxyScriptContext.js https://reviewboard.mozilla.org/r/153586/#review160894 Clearing review until the refactoring in the first commit is finished.

I think this patch is supplanted by other (forthcoming) patches for related bugs.

This is addressed in bug 1381290