Closed Bug 1505330 Opened 1 year ago Closed 9 months ago

Single lockPref blocks all proxy configuration page

Categories

(Firefox :: Preferences, defect, P3)

60 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 69
Tracking Status
firefox-esr60 - wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- verified
firefox69 --- verified

People

(Reporter: filippo.martinelli, Assigned: mkaply)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

In my network I have a shared global_settings.js which sets some preferences.
One row is like the following:
lockPref("network.proxy.no_proxies_on", "some hosts")


Actual results:

Choosing Option -> Proxy Server settings everything is grayed out (disabled) and cannot change anything.


Expected results:

Only the "no_proxies_on" should be disabled.
about:config confirms that only "no_proxies_on" is locked.
Component: Untriaged → Preferences
This definitely should work that way. Was this working on previous versions of Firefox?
(In reply to Mike Kaply [:mkaply] from comment #1)
> This definitely should work that way. Was this working on previous versions
> of Firefox?

Mike, for clarification, by "this definitely should work that way", what are you referring to as "this"? Are you saying that it is expected for the full proxy page to get disabled or only the specific "no_proxies_on"?
Flags: needinfo?(mozilla)
It should only block the one thing that is locked, not everything.
Flags: needinfo?(mozilla)
setecastronomy, can you please use mozregression to find when this broke? See http://mozilla.github.io/mozregression/ 

There is a --profile option that you can provide which will tell mozregression to use a specific profile for testing with. The default option of --profile-persistence is to clone the profile on each attempt so you won't have to worry about corrupting your main profile.
Flags: needinfo?(filippo.martinelli)
From code inspection, I believe this is the result of bug 1429593. That updates the network settings for add-on control, with this code:

  // Update the UI to show/hide the extension controlled message for
  // proxy settings.
  async updateProxySettingsUI() {
    let isLocked = API_PROXY_PREFS.some(
      pref => Services.prefs.prefIsLocked(pref));

    function setInputsDisabledState(isControlled) {
      let disabled = isLocked || isControlled;
      for (let element of gConnectionsDialog.getProxyControls()) {
        element.disabled = disabled;
      }
      if (!isLocked) {
        gConnectionsDialog.proxyTypeChanged();
      }
    }

    if (isLocked) {
      // An extension can't control this setting if any pref is locked.
      hideControllingExtension(PROXY_KEY);
      setInputsDisabledState(false);
    } else {
      handleControllingExtension(PREF_SETTING_TYPE, PROXY_KEY)
        .then(setInputsDisabledState);
    }
  },

As you can see, if any prefs are locked, add-ons can't control this setting - but that code also disables every single one of the proxy controls.

Bob, can you take a look if that read is right?
Blocks: 1429593
Flags: needinfo?(filippo.martinelli) → needinfo?(bob.silverberg)
This'll affect enterprise users so we should get this fixed ASAP.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Priority: -- → P1
While this is a regression per se, the particular case in this bug didn't work in 52 either.

The typical use-case for proxy settings is to lock the user switching the type, not to lock the individual fields.

So since the user can't switch, there's no need to lock the field.

If we ever change this so this field lives outside the individual prefs, we'll need to make sure this locks properly.

I do still think what was done here is incorrect. One pref locked shouldn't cause everything to be locked. But this dialog has bigger issues.
Gijs, your interpretation looks correct to me. Please note that I'm no longer on the WebExtensions engineering team, so if this needs some attention I would suggest needinfoing someone still on the team.
Flags: needinfo?(bob.silverberg)
(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> Gijs, your interpretation looks correct to me. Please note that I'm no
> longer on the WebExtensions engineering team, so if this needs some
> attention I would suggest needinfoing someone still on the team.

Shane?
Flags: needinfo?(mixedpuppy)
too late for 64 at this point.
I haven't ever looked at this code, but given comment 7 it seems like only the top level should be locked.  I'm guessing that reducing what getProxyControls returns (in the test as well) would address this.

OTOH, it does seem appropriate to lock all these prefs *if an extension is controlling the prefs*.  In that case, I'm not certain the user should have access to any "sub prefs" in the dialog.  So we might need updateProxySettingsUI to be a little smarter about when an extension is in control vs. something else.
Flags: needinfo?(mixedpuppy)
Just went through the dialog, it's all strange.  It seems I can change a couple values in there when an extension is in control, but not others.  I'm not really sure what the behavior should be, but I think we need to address the dialog in general.
I don't think we need to rush this into ESR given that it didn't work in ESR52 or ESR60. But if you come up with a solution and think it should be uplifted, please do request ESR uplift.
I'm going to clear the tracking flags on this.

AIUI, pre bug 1429593 the pref was successfully locked, but the UI was confusing because it didn't disable fields properly.

Post that bug, the pref is still successfully locked, but the UI is confusing because it disables too many fields.

Would take a patch, but given (1) the long term bustage and (2) that bug 1429593 has been shipping in ESR60 since May 9 (or September 5 if you go by the ESR52-EOL date) it's not a priority.
Priority: P1 → P3
Since this is triaged and has a priority set, marking this fix-optional to remove it from regression triage. 
Happy to still take a patch in nightly.
Duplicate of this bug: 1548856
Assignee: nobody → mozilla
Status: NEW → ASSIGNED

Mike, If I unlock the one preference that is causing this issue, the configuration becomes corrupt and does not not work. Is there any news of when this issue with be resolved as it looks like it has been an issue for some time. Thanks

Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/18cac1e9e359
Only disable explicitly locked preferences. r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69

Comment on attachment 9069379 [details]
Bug 1505330 - Only disable explicitly locked preferences.

Beta/Release Uplift Approval Request

  • User impact if declined: Enterprises who lock one or two prefs in preferences end up locking them all.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Steps are in bug. Unzip autoconfig.zip into Firefox directory. Verify all networking prefs are locked before fix.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Only affects locked prefs (enterprise).

This is a regression and it affects enterprise, would be nice to have.

  • String changes made/needed:
Attachment #9069379 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

I have managed to reproduce the issue using Fx65.0a1 buildID: 20181107220128.

The issue is verified fixed using Fx69.0a1 on macOS 10.14, Ubuntu 18.04 x64 and Windows 10 x64. The "lockPref("network.proxy.no_proxies_on", "some hosts")" pref no longer blocks the entire proxy settings page, only the "No proxy for" section.

However there is a small problem. When changing the proxy settings between Manual proxy configuration, Use system proxy settings, Auto-detect proxy settings for this network the 'No proxy for' field becomes active. The weirder behavior is that after inputting any value in, the field becomes read-only again. This wouldn't necessarily be a blocker, because none of the changes to the field are saved, but I consider this confusing enough for the user.
@mkaply Should this be fixed before the uplift?

Flags: needinfo?(mozilla)
Flags: needinfo?(mozilla)
Attachment #9069379 - Flags: approval-mozilla-beta?

@mkaply Should this be fixed before the uplift?

Yes, I'll take a look. My guess is the behavior is related to us making that field not tied to the state of the network proxy type anymore.

Comment on attachment 9069379 [details]
Bug 1505330 - Only disable explicitly locked preferences.

See earlier request:

https://bugzilla.mozilla.org/show_bug.cgi?id=1505330#c21

Attachment #9069379 - Flags: approval-mozilla-beta?

Tracking information states you will not be fixing this bug in ESR 60, please can you confirm it will be resolved in the new release of Firefox ESR. Thanks

Tracking information states you will not be fixing this bug in ESR 60, please can you confirm it will be resolved in the new release of Firefox ESR.

I've requested uplift to Firefox 68 which means it will be in the next ESR.

What was the resolution from comments 22/23?

Flags: needinfo?(mozilla)

(In reply to Julien Cristau [:jcristau] from comment #27)

What was the resolution from comments 22/23?

There's a patch for it in bug 1557717 but that bounced due to issues with the test and our blocking outside network connections (as all our test frameworks use a localhost proxy to deal with network things, so changing proxy type prefs in a test is liable to break the test, because we start making outside network connections that we otherwise don't)

Blocks: 1557717

That patch is now in (without test). I'll ask for uplift for that as well.

Flags: needinfo?(mozilla)

Comment on attachment 9069379 [details]
Bug 1505330 - Only disable explicitly locked preferences.

fix for proxy preferences, approved for 68.0b10

Attachment #9069379 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Tested the uplift fix on an intermediary Fx68.0b10 buildID: 20190612231006 from treeherder on macOS 10.13, Ubuntu 18.04 and windows 10 x64. Only the 'No proxies for' option is locked in the UI.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.