1445991 - After 59.0 update Options Network Proxy Settings Reload button is grayed out and not selectable

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[transvaluation]

User Agent: Mozilla/5.0 (iPad; CPU OS 11_2_6 like Mac OS X) AppleWebKit/604.5.6 (KHTML, like Gecko) Version/11.0 Mobile/15D100 Safari/604.1

Steps to reproduce:

Upgrade to version 59.0
Navigate to Options:Network Proxy Connection Settings
Reload button next to Auotomatic proxy configuration URL is grayed out even though it’s radio button is selected and the field contains an address


Actual results:

Button is grayed out


Expected results:

Button should be selectable to reload the proxy settings from the URL specfied

Comment 1

[Alice0775 White]

I can also reproduce this on Windows10.

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: unable reload pac file

Steps To Reproduce:
1. Open preferences about:preferences > Network Proxy > click [Settings...] button
2. Select radio button "Auotomatic proxy configuration URL"
3. Type valid URL such as file:///C:/Windows/proxycfg.pac and click [OK] button
4. Close the preferences tab

5. Open preferences again about:preferences > Network Proxy
6. Observe status of [Settings...] button


Actual Results:
Disabled

Expected Results:
Enabled, can be clicked


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d760cf06ca40056077e2a354b8b69350a5765ae3&tochange=0adedb70b7883ab39239b8531d07f1308ffc10c7

Regressed by: 0adedb70b788	Myk Melez — Bug 1379338 - scriptify preferences XBL; r=jaws

Comment 3

[:Gijs (he/him)]

Myk, do you have cycles to look into this?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Yes, looking…

Comment 5

[Alice0775 White]

oops, error in str comment #2
6. Observe status of [Settings...] button

[corrected]
6. Click [Settings...] button
7. Observe status of [Reload button

Comment 6

[Myk Melez [:myk] [@mykmelez]]

I can reproduce on release (59) but not beta (60) nor nightly (61), so this seems to have been fixed since it regressed.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

This seems to have been fixed between https://ftp.mozilla.org/pub/firefox/nightly/2018/03/2018-03-01-10-01-39-mozilla-central/ and https://ftp.mozilla.org/pub/firefox/nightly/2018/03/2018-03-01-22-00-50-mozilla-central/.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Looking at the changelog, the fix for bug 1437335 might've been the change that fixed this.  I'm now bisecting between those two nightly builds to confirm.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Tentatively assigning P1 as a recent regression while we investigate and confirm.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

Reorder the construction of Preference objects so network.proxy.autoconfig_url gets constructed before network.proxy.type, which will ensure that networkProxyAutoconfigURL is set to the value of network.proxy.autoconfig_url before network.proxy.type construction triggers the call to updateReloadButton.

Review commit: https://reviewboard.mozilla.org/r/228218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228218/

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Before bug 1379338, gConnectionsDialog.updateReloadButton would run twice when the dialog is opened, first on "change" of the network.proxy.type <preference> and a second time on "paneload" via a stack like this:

> updateReloadButton (connection.js#109)
> proxyTypeChanged (connection.js#83)
> readProxyType (connection.js#115)
> anonymous (SOURCEundefined#3)
> setElementValue (preferences.xml#445)
> onxblpaneload (preferences.xml#1340)
> _fireEvent (preferences.xml#780)
> refwindow_XBL_Constructor (preferences.xml#663)

The first time it runs is actually too early, before networkProxyAutoconfigURL gets set to the value of network.proxy.autoconfig_url, so updateReloadButton will sometimes incorrectly disable the button.

The second time, however, networkProxyAutoconfigURL has been set to the right value, so updateReloadButton correctly sets the button state.

After bug 1379338, updateReloadButton only runs once when the dialog is opened, on "change" of the network.proxy.type Preference.

As before, that's too early, before networkProxyAutoconfigURL gets set, so updateReloadButton will sometimes incorrectly disable the button.  However, since bug 1379338 removed the paneload handler, updateReloadButton doesn't run a second time, so it never corrects that mistake.

The fix for bug 1437335 causes the button state to get set correctly again, because it calls updateReloadButton a second time if there isn't an extension controlling the proxy settings.  But that isn't really a fix, as it merely conceals the bug.  It's also presumably more than we'd want to land on the release branch, if we decide that this is significant enough to uplift a fix.

I see two options here:

1. Re-introduce the equivalent of the old "paneload" handler and run updateReloadButton a second time on window load.

2. Reorder the construction of Preference objects so network.proxy.autoconfig_url gets initialized before network.proxy.type, which will ensure that networkProxyAutoconfigURL is set correctly before network.proxy.type initialization triggers the call to updateReloadButton.

I've opted for the latter option, which seems like a more minimal fix.  I've also added a test to confirm the fix.  Note that the test passes with or without the fix, given bug 1437335, but I've confirmed that it fails before bug 1437335, which means it should fail again in the future if something else regresses the behavior.

(Kris: to confirm, the ban on |uses-unsafe-cpows| doesn't extend to |eslint-disable-next-line mozilla/no-cpows-in-tests|, does it?)

Comment 12

[Kris Maglione [:kmag]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #11)
> (Kris: to confirm, the ban on |uses-unsafe-cpows| doesn't extend to
> |eslint-disable-next-line mozilla/no-cpows-in-tests|, does it?)

It does not. I have a patch in bug 1443964 to remove that rule, but I haven't decided whether to land it as-is or just update the rule to fix the false positives.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8959398 [details]
Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

https://reviewboard.mozilla.org/r/228218/#review234142

Nice, thanks!

::: browser/components/preferences/in-content/tests/browser_connection_bug1445991.js:4
(Diff revision 1)
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Nit: convention is to also add `"use strict";`

::: browser/components/preferences/in-content/tests/browser_connection_bug1445991.js:5
(Diff revision 1)
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +add_task(async function() {

Nit: Please name the function and add a brief comment before the `add_task` call explaining what we're testing.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8959398 [details]
Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228218/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de57fef11c80
ensure prefs inited before autoconfig reload button updated; r=Gijs

Comment 16

[Andrei Ciure[:aciure]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance so we can get some wider testing on it. Also, are there any functional workarounds for users on 59 here (like restarting the browser) or is this functionality completely broken?

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8959398 [details]
Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

Approval Request Comment

[Feature/Bug causing the regression]: bug 1379338

[User impact if declined]: Users who have configured an "automatic proxy configuration URL" and want to reload their proxy settings from the URL will find the Reload button disabled in the Connection Settings dialog when they select Preferences/Options > Network Proxy > Settings…

[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No

[Why is the change risky/not risky?]:
1. it's the minimal fix (a one-line change, modulo the new test);
2. it only affects the Network Proxy settings dialog;
3. it includes a new test to confirm the fix.

[String changes made/needed]: None

Comment 19

[Myk Melez [:myk] [@mykmelez]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Please request Beta approval on this when you get a chance so we can get
> some wider testing on it.

Ok, I've done so.

> Also, are there any functional workarounds for
> users on 59 here (like restarting the browser) or is this functionality
> completely broken?

The functional workaround is to select a different proxy radio button (f.e. "No Proxy" or "Use system proxy settings") and then re-select the "Automatic proxy configuration URL" radio button, at which point the Reload button will become enabled, and the user will be able to press it.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8959398 [details]
Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

fix for proxy preferences UI issue, beta60+

Comment 21

[Hani Yacoub, Desktop QA]

Build ID: 20180320100122
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Verified as fixed on Firefox Nightly 61.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4f2e5b33fec1

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8959398 [details]
Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

This seems like a low-risk change, Release59+

Comment 24

[Hani Yacoub, Desktop QA]

Build ID: 20180322152034
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0

Verified as fixed on Firefox Beta 60.0b6 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/7cf9c0cfefd3

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to 59.0.2 release notes

Comment 27

[Hani Yacoub, Desktop QA]

Build ID: 20180323154952
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

Verified as fixed on Firefox 59.0.2 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.