Closed Bug 1445991 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Settings UI, defect, P1)

59 Branch
Unspecified
All
defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
relnote-firefox --- 59+
firefox-esr52 --- unaffected
firefox59 --- verified
firefox60 --- verified
firefox61 + verified

People

(Reporter: transvaluation, Assigned: myk)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
I can also reproduce this on Windows10.
Status: UNCONFIRMED → NEW
Component: Untriaged → Preferences
Ever confirmed: true
Keywords: regression
OS: Unspecified → All
[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
Myk, do you have cycles to look into this?
Flags: needinfo?(myk)
Yes, looking…
Assignee: nobody → myk
Flags: needinfo?(myk)
oops, error in str comment #2
6. Observe status of [Settings...] button

[corrected]
6. Click [Settings...] button
7. Observe status of [Reload button
I can reproduce on release (59) but not beta (60) nor nightly (61), so this seems to have been fixed since it regressed.
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
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 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+
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de57fef11c80
ensure prefs inited before autoconfig reload button updated; r=Gijs
https://hg.mozilla.org/mozilla-central/rev/de57fef11c80
Status: NEW → RESOLVED
Closed: 6 years ago
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?
Flags: qe-verify+
Flags: needinfo?(myk)
Flags: in-testsuite+
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?
(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+
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
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+
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.
Flags: qe-verify+
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.
You need to log in before you can comment on or make changes to this bug.