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

VERIFIED FIXED in Firefox 59

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
a month ago
24 days 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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month 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 month 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 month ago
status-firefox-esr52: unaffected → ?

Comment 2

a month 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 month ago
Myk, do you have cycles to look into this?
Flags: needinfo?(myk)
(Assignee)

Comment 4

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

Comment 5

a month 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 month 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 month 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 month 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 month 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 month 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 month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de57fef11c80
Status: NEW → RESOLVED
Last Resolved: a month 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 month 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 month 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

29 days 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

29 days 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

27 days 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

27 days ago
Flags: qe-verify+

Comment 25

27 days 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

24 days 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.