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

VERIFIED FIXED in Firefox 59

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: transvaluation, Assigned: myk)

Tracking

({regression})

59 Branch
Firefox 61
Unspecified
All
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 59+, firefox-esr52 unaffected, firefox59 verified, firefox60 verified, firefox61+ verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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

a year ago
I can also reproduce this on Windows10.
Status: UNCONFIRMED → NEW
status-firefox59: --- → affected
status-firefox60: --- → affected
status-firefox61: --- → affected
status-firefox-esr52: --- → unaffected
Component: Untriaged → Preferences
Ever confirmed: true
Keywords: regression
OS: Unspecified → All

Updated

a year ago
status-firefox-esr52: unaffected → ?

Comment 2

a year ago
[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
Blocks: 1379338
status-firefox-esr52: ? → unaffected
tracking-firefox61: --- → ?

Comment 3

a year ago
Myk, do you have cycles to look into this?
Flags: needinfo?(myk)
(Assignee)

Comment 4

a year ago
Yes, looking…
Assignee: nobody → myk
Flags: needinfo?(myk)

Comment 5

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

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

Comment 6

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

Comment 8

a year ago
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.
Tentatively assigning P1 as a recent regression while we investigate and confirm.
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
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?)
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)

Comment 13

a year ago
mozreview-review
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.
Attachment #8959398 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de57fef11c80
ensure prefs inited before autoconfig reload button updated; r=Gijs

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de57fef11c80
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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?
tracking-firefox61: ? → +
Flags: qe-verify+
Flags: needinfo?(myk)
Flags: in-testsuite+
(Assignee)

Comment 18

a year ago
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
Attachment #8959398 - Flags: approval-mozilla-beta?
(Assignee)

Comment 19

a year ago
(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.
Flags: needinfo?(myk)
Comment on attachment 8959398 [details]
Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

fix for proxy preferences UI issue, beta60+
Attachment #8959398 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 21

a year ago
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.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified

Comment 22

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4f2e5b33fec1
status-firefox60: affected → fixed
Comment on attachment 8959398 [details]
Bug 1445991 - ensure prefs inited before autoconfig reload button updated;

This seems like a low-risk change, Release59+
Attachment #8959398 - Flags: approval-mozilla-release+

Comment 24

a year ago
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.
status-firefox60: fixed → verified

Updated

a year ago
Flags: qe-verify+

Comment 25

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/7cf9c0cfefd3
status-firefox59: affected → fixed
Added to 59.0.2 release notes
relnote-firefox: --- → 59+

Comment 27

a year ago
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.
status-firefox59: fixed → verified
You need to log in before you can comment on or make changes to this bug.