Closed Bug 1455755 Opened 6 years ago Closed 6 years ago

consider moving proxyConfig to proxy namespace

Categories

(WebExtensions :: Request Handling, defect)

58 Branch
defect
Not set
normal

Tracking

(firefox-esr60 verified, firefox60+ verified, firefox61 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr60 --- verified
firefox60 + verified
firefox61 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

Due to bug 1455705, this is also a prime opportunity to move proxyConfig into the proxy namespace.  Fx59 is busted regardless.  If we can uplift this into 60 we'll have proxy settings under the proxy namespace and permission.  60 is also esr, so it's a bit of a double win if this change is something we want to do.
Attachment #8969801 - Flags: feedback?(aswan)
Attachment #8969801 - Flags: feedback?(aswan)
Attachment #8969801 - Flags: feedback?(aswan) → review?(aswan)
Comment on attachment 8969801 [details]
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings,

https://reviewboard.mozilla.org/r/238640/#review244506

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:153
(Diff revision 2)
> +const checkUnsupported = (name, unsupportedPlatforms) => {
> +  if (unsupportedPlatforms.includes(AppConstants.platform)) {
> +    throw new Error(
> +      `${AppConstants.platform} is not a supported platform for the ${name} setting.`);
> +  }
> +};

It looks like the only use of this is to not support proxy settings on Android.  Lets just move the settings part under browser/ and ditch this.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:362
(Diff revision 2)
>      }
>      await ExtensionSettingsStore.initialize();
>      return ExtensionSettingsStore.getLevelOfControl(id, storeType, name);
>    },
> +
> +  getSettingsAPI(extension, name, callback, storeType, readOnly = false, unsupportedPlatforms = []) {

This could really use a documentation comment

::: toolkit/components/extensions/parent/ext-proxy.js:172
(Diff revision 2)
>          onError,
>  
>          // TODO Bug 1388619 deprecate onProxyError.
>          onProxyError: onError,
> +
> +        settings: Object.assign(

what is the call to Object.assign() for?

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_settings.js:16
(Diff revision 2)
>    promiseStartupManager,
>  } = AddonTestUtils;
>  
>  AddonTestUtils.init(this);
>  
> -createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
> +createAppInfo("xpcshell@tests.example.com", "XPCShell", "1", "42");

why this change?

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_settings.js:29
(Diff revision 2)
> +add_task(async function startup() {
> +});

what is this for?
Attachment #8969801 - Flags: review?(aswan)
Comment on attachment 8969801 [details]
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings,

https://reviewboard.mozilla.org/r/238640/#review244506

> what is the call to Object.assign() for?

getSettingsAPI returns an object with get/set/clear methods.  This is getting the default implementations, then overriding that with a custom set method.  It's pretty much how all of browserSettings works.  I'm fine with changing that, but would prefer to do so in a seperate bug.
Based on lots of discussion with Product and engineering, it seems uplifting this change in 60 makes sense. It will lead to less webext developer and end-user confusion when dealing with proxy API. It is also the right thing to do w.r.t W3C standardization, so as to avoid redoing it in a future version.

Shane has added automated tests and I have requested Krupa to help with some focused testing during RC week while we try to uplift this in 60.b16 (gtb tomm).
Comment on attachment 8969801 [details]
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings,

https://reviewboard.mozilla.org/r/238640/#review245382
Attachment #8969801 - Flags: review?(aswan) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b7a347f011cb
Move browserSettings.proxyConfig to proxy.settings, r=aswan
Backed out changeset b7a347f011cb (bug 1455755) for browser chrome failure on browser/components/preferences/in-content/tests/browser_extension_controlled.js. CLOSED TREE 

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=175566717&repo=autoland&lineNumber=2950

INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_extension_controlled.js | The connection settings description is as expected. - 
10:55:14     INFO - Extension loaded
10:55:14     INFO - Buffered messages logged at 10:54:39
10:55:14     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
10:55:14     INFO - Console message: [JavaScript Error: "TypeError: browser.browserSettings.proxyConfig is undefined" {file: "moz-extension://97c8dbfe-9ed9-8945-832a-ea54c4f5fec8/%7Baedaf2e3-3279-3147-b16a-5b9a337ecb15%7D.js" line: 2}]
10:55:14     INFO - background@moz-extension://97c8dbfe-9ed9-8945-832a-ea54c4f5fec8/%7Baedaf2e3-3279-3147-b16a-5b9a337ecb15%7D.js:2:5
10:55:14     INFO - @moz-extension://97c8dbfe-9ed9-8945-832a-ea54c4f5fec8/%7Baedaf2e3-3279-3147-b16a-5b9a337ecb15%7D.js:1:11
10:55:14     INFO - 
10:55:14     INFO - Buffered messages finished
10:55:14     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Test timed out - 
10:55:14     INFO - Not taking screenshot here: see the one that was previously logged
10:55:14     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Extension left running at test shutdown - 
10:55:14     INFO - Stack trace:
10:55:14     INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:109
10:55:14     INFO - chrome://mochikit/content/browser-test.js:nextTest:675
10:55:14     INFO - GECKO(1985) | MEMORY STAT | vsize 4729MB | residentFast 685MB | heapAllocated 98MB
10:55:14     INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_extension_controlled.js | took 45080ms
10:55:14     INFO - Not taking screenshot here: see the one that was previously logged
10:55:14     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Found a tab after previous test timed out: about:preferences#general - 
10:55:14     INFO - checking window state

Push with the failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b7a347f011cb8c1eb15e8357f2dae0f97eef9bd8

Backout:
https://hg.mozilla.org/integration/autoland/rev/1d99f96c0aec2f65e4d17dd3d11a46615f1d690a
Flags: needinfo?(mixedpuppy)
Comment on attachment 8969801 [details]
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings,

Added mstriemer as additional reviewer for changes in browser/components/preferences/
Flags: needinfo?(mixedpuppy)
Comment on attachment 8969801 [details]
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings,

https://reviewboard.mozilla.org/r/238640/#review245468
Attachment #8969801 - Flags: review?(mstriemer) → review+
Attached file proxyTesting.xpi
Here's a simple proxy extension that allows you to set the http proxy.  

The browser action panel has a link to a list of public ssl proxies.  Don't trust them, but good enough to test if you can find one that works.  I listed one in the panel that worked in the US for me.

Test prep:

- start firefox with test profile
- install extension
- open console to see extension logging
- open about:preferences

Some testing suggestions:

1. about:preferences is updated
- open panel and set proxy to test:1234, click set
- verify about:prefs is now controlled by extension
- in panel click reset
- verify about:prefs is not controlled by extension

2. proxy works
- set proxy to test:1234 per above
- open a tab to any page
- you should see an error page that states proxy server cannot be found.  This lets us know the proxy service actually made use of the proxy preferences we set.
- in the console you should see a message like:
  "proxied https://somewebpage through test:1234"

3. proxy works for real, if you can find a working proxy server
- set proxy to address:port for the proxy
- open a tab to any page
- in the console you should see a (or several) message like:
  "proxied https://somewebpage through address:port"
- the web page would have loaded properly of course
- reset the proxy in the panel
- reload the tab
- in the console you should see a message like:
  "direct request for http://somewebpage"
Flags: needinfo?(kraj)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s def9947775693e5bc7b19c2c7965fc22637dfcaa -d fc793911d371: rebasing 460462:def994777569 "Bug 1455755 Move browserSettings.proxyConfig to proxy.settings, r=aswan,mstriemer" (tip)
merging browser/components/preferences/in-content/extensionControlled.js
merging browser/components/preferences/in-content/tests/browser_extension_controlled.js
merging toolkit/components/extensions/ExtensionPreferencesManager.jsm
merging toolkit/components/extensions/parent/ext-browserSettings.js
merging toolkit/components/extensions/parent/ext-proxy.js
merging toolkit/components/extensions/schemas/browser_settings.json
merging toolkit/components/extensions/schemas/proxy.json
merging toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
merging toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js and toolkit/components/extensions/test/xpcshell/test_ext_proxy_config.js to toolkit/components/extensions/test/xpcshell/test_ext_proxy_config.js
merging toolkit/components/extensions/test/xpcshell/test_ext_proxy_settings.js
merging toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
warning: conflicts while merging browser/components/preferences/in-content/extensionControlled.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0fb8736b547fabf8fb42cbd92f97954d04c4bc9c -d fc793911d371: rebasing 460463:0fb8736b547f "Bug 1455755 Move browserSettings.proxyConfig to proxy.settings, r=aswan,mstriemer" (tip)
merging browser/components/preferences/in-content/extensionControlled.js
merging browser/components/preferences/in-content/tests/browser_extension_controlled.js
merging toolkit/components/extensions/ExtensionPreferencesManager.jsm
merging toolkit/components/extensions/parent/ext-browserSettings.js
merging toolkit/components/extensions/parent/ext-proxy.js
merging toolkit/components/extensions/schemas/browser_settings.json
merging toolkit/components/extensions/schemas/proxy.json
merging toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
merging toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js and toolkit/components/extensions/test/xpcshell/test_ext_proxy_config.js to toolkit/components/extensions/test/xpcshell/test_ext_proxy_config.js
merging toolkit/components/extensions/test/xpcshell/test_ext_proxy_settings.js
merging toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
warning: conflicts while merging browser/components/preferences/in-content/extensionControlled.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd589df323a908de450258d46381efe19ec4afca
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings, r=aswan, mstrimer
https://hg.mozilla.org/mozilla-central/rev/bd589df323a9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please request uplift ASAP for this to make 60.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Comment on attachment 8969801 [details]
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings,

Approval Request Comment
[Feature/Bug causing the regression]: 1425535
[User impact if declined]: A bunch of this was discussed in email, here's just one.  Users will be asked for permission to allow extensions to "modify browser settings" rather than the more appropriate "control browser proxy settings." Potential for user confusion and dissatisfaction exists.  
[Is this code covered by automated tests?]: yes (see also below)
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: An extension and tests have been provided for additional QE
[List of other uplifts needed for the feature/fix]:  1455705
[Is the change risky?]: moderate
[Why is the change risky/not risky?]: There is a good set of tests on this api now, including additional tests added to bug  1455705 
[String changes made/needed]: none
Flags: needinfo?(mixedpuppy)
Attachment #8969801 - Flags: approval-mozilla-beta?
Comment on attachment 8969801 [details]
Bug 1455755 Move browserSettings.proxyConfig to proxy.settings,

approved for 60.0b16 and 60.0 rc1
Attachment #8969801 - Flags: approval-mozilla-release+
Attachment #8969801 - Flags: approval-mozilla-beta?
Attachment #8969801 - Flags: approval-mozilla-beta+
beta try run in comment 25
For dev-doc: move docs for browserSettings.proxyConfig to proxy.settings.  browserSettings.proxyConfig should probably be documented as moved somehow, and that it is non-functional in fx59.
Keywords: dev-doc-needed
> that it is non-functional in fx59
Will it be aliased for the time being? For how long?
https://hg.mozilla.org/releases/mozilla-beta/rev/fb9171950de8 (FIREFOX_60b_RELBRANCH)

Leaving 60 as affected until the m-r push.
(In reply to erosman from comment #29)
> > that it is non-functional in fx59
> Will it be aliased for the time being? For how long?

no.  it didn't work except for autoconfig.  best to pull the band aide off fast.
> no.  it didn't work except for autoconfig
Oh .. I see .. because of the scheme issue.

Well, my add-on uses autoconfig only so no problem there but now we get into 2 separate APIs (namespace), one for FF59 and one for FF60+
(In reply to erosman from comment #32)
> > no.  it didn't work except for autoconfig
> Oh .. I see .. because of the scheme issue.
> 
> Well, my add-on uses autoconfig only so no problem there but now we get into
> 2 separate APIs (namespace), one for FF59 and one for FF60+

That's why I cc'd you here, but you're the only known user of it.  It should be simple to work around and one extension isn't enough to warrant a more complicated cycle on this.
I understand. 

My question was that will FF59-60 stay as is (browserSettings.proxyConfig) and FF61 will ONLY have 'proxy.settings'?

Will if affect the upcoming FF60 on 2018-05-09?

I am currently on 61.0a1 (2018-04-18) (64-bit) and still using browserSettings.proxyConfig
(In reply to erosman from comment #34)
> I understand. 
> 
> My question was that will FF59-60 stay as is (browserSettings.proxyConfig)
> and FF61 will ONLY have 'proxy.settings'?
> 
> Will if affect the upcoming FF60 on 2018-05-09?
> 
> I am currently on 61.0a1 (2018-04-18) (64-bit) and still using
> browserSettings.proxyConfig

Starting with 60 only proxy.settings.  One reason of several for 60 is ESR.
I tested all scenarios described in https://bugzilla.mozilla.org/show_bug.cgi?id=1455755#c15 and everything worked as expected, but I was not able to find a working proxy server for https sites(I tried more than 40 proxies including the one which worked in Shane's example). Please note that I was able to open some http sites via proxy.

I will attach a postfix video. 

Please let me know if any other testing is needed.
Flags: needinfo?(kraj) → needinfo?(mixedpuppy)
Attached image Postfix video
I've re-verified on nightly and esr60 (osx) that ssl access works for me through the same steps I outlined, using the proxy I listed.  I suspect a firewall or perhaps the proxy itself is denying access for you when using ssl.
Flags: needinfo?(mixedpuppy)
I also just verified with 60 in mozilla-release off of treeherder.
Thanks Shane!
If ssl works on FF60, esr60 and Nightly I will mark the bug as verified since all other tests were passed for all fixed versions.
Status: RESOLVED → VERIFIED
Which Nightly has the new namespace (in order to test it)?

Until Docs are updated, will the methods remain the same?

browser.browserSettings.proxyConfig.get({}).then()
to:
browser.proxy.settings.get({}).then()

browser.browserSettings.proxyConfig.set({value: {...})
to: (with same properties in value?)
browser.proxy.settings.set({value: {....})
Testing 61.0a1 (2018-04-28) (64-bit) Win

I was going to test clear() but the clear() doesn't seem to be working properly with PAC, unless clear({}) is not the correct method.
I was going according to:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_proxy_config.js#278

I set the PAC with browser.proxy.settings.set(..);

browser.proxy.settings.clear({}); doesn't clear it. The PAC is still active, the PAC URL filled and "Do not prompt for authentication if password is saved" ticked.

However, it clears the Extension Info showing "An extension, FoxyPAC, is controlling how Nightly connects to the internet."

browser.proxy.settings.set({value: { proxyType: 'system' }}); works fine though.
If there are followup issues please create new bugs.
I dont know if that was a bug or I was doing it wrong. If it is a bug, I will file a new one.
Depends on: 1457976
I've moved the docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy/settings and filed a PR for the compat data: https://github.com/mdn/browser-compat-data/pull/2023. Please let me know if this covers it.
Flags: needinfo?(mixedpuppy)
> A BrowserSetting object that can be used to change the browser's proxy settings.

I think above (BrowserSetting object) needs updating. :)

In order to avoid confusion, it would also be good to mention somewhere that:

For Firefox 59 (only): browser.browserSettings.proxyConfig
For Firefox 60+: browser.proxy.setting
(In reply to erosman from comment #49)

> For Firefox 59 (only): browser.browserSettings.proxyConfig

Specifically to comment that the API was unstable and should be avoided.
Flags: needinfo?(mixedpuppy)
> I think above (BrowserSetting object) needs updating. :)

No, it is a BrowserSetting object. One of these: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting

> In order to avoid confusion, it would also be good to mention somewhere that:
...
> Specifically to comment that the API was unstable and should be avoided.

That is in the PR for the compat data: https://github.com/mdn/browser-compat-data/pull/2023.
> No, it is a BrowserSetting object. One of these: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting

That 'may' be confusing since the object doesnt belong to the API of the same name i,e,
BrowserSetting object -> browser.browserSettings

While the object belongs to browser.proxy.setting API
The BrowserSetting type is used (now) in at least three different namespaces - privacy, browserSettings, proxy - and possibly more that I have forgotten. It's a generic type representing, well, a browser setting, that's available for use inside any other namespace, AFAIK.

It's called BrowserSetting in the docs because it's modeled on ChromeSetting (https://developer.chrome.com/extensions/types#type-ChromeSetting). Although looking at the code it seems like we actually call it "Setting" internally (https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/types.json#23). I'm open to changing the docs to that that if it's the consensus view, but I think that should be discussed in a different bug.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.