Closed Bug 1582740 Opened 6 years ago Closed 5 years ago

The "Always check if Firefox is your default browser" checkbox status can be changed even when Firefox is the default browser (due to delay before we enable/disable the checkbox)

Categories

(Firefox :: Settings UI, defect, P1)

71 Branch
defect

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox-esr78 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 --- fixed

People

(Reporter: daniel.kolsis, Assigned: jaws)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image DefaultBrowser.gif

Affected versions

  • Firefox 68.1.0esr
  • Firefox 69.0.8
  • Firefox 70.b8
  • Firefox 71.0a1

Affected platforms

  • Windows 10 64bit.
  • Windows 7 64bit.
  • macOS 10.13.
  • Ubuntu 16.04 64bit.

Steps to reproduce

  1. Launch Firefox.
  2. Access the about:preferences page.
  3. Click the "Make Default" button. (in order to lock the "Always check if Firefox is your default browser checkbox).
  4. Refresh the about:preferences page.
  5. Quickly uncheck the "Always check if Firefox is your default browser" checkbox.

Expected result

  • Unchecking the "Always check if Firefox is your default browser" checkbox is not possible as long as it is locked.
  • The browser.shell.checkDefaultBrowser pref remains set to true and refreshing the about:preferences page doesn't save the actions performed in step 5.

Actual result

  • The user is able to check the "Always check if Firefox is your default browser" checkbox if the about:preferences page is refreshed even though it is displayed as locked.
  • The browser.shell.CheckDefaultBrowser pref changes to false and the actions performed in step 5 are saved.

Regression range

Last good revision: e4b9b1084292686d3eb50ba0cadd85950824c955 (2019-01-25)
First bad revision: bb2895bfd1bc3d83c309e904dbe74e0c60c3fac9 (2019-01-26)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e4b9b1084292686d3eb50ba0cadd85950824c955&tochange=bb2895bfd1bc3d83c309e904dbe74e0c60c3fac9

Additional notes

  • For further information regarding this issue, please observe the attached screencast.
  • Note: Bug 1532701 was not fixed, so we created another one.

Changing the confusing summary - this has nothing to do with preference locking in the lockPref enterprise sense.

Out of the bugs in the regression window, bug 1522694 looks the most plausible, but even then I don't understand how that bug would have changed something here. Dave, can you have a look?

Flags: needinfo?(dtownsend)
Summary: The "Always check if Firefox is your default browser" checkbox status can be changed even though it is locked → The "Always check if Firefox is your default browser" checkbox status can be changed even when Firefox is the default browser (due to delay before we enable/disable the checkbox)

It's not in the regression range (though it's pretty close), but my money is on bug 1520350.

When loading a preferences pane we create the content from a template and then call the preferences API to update the UI from prefs: https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/browser/components/preferences/in-content/preferences.js#68
The preferences API does this asynchronously: https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/toolkit/content/preferencesBindings.js#140
And so we end up calling a pane's init function before the UI elements have the correct state: https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/browser/components/preferences/in-content/preferences.js#70
So when we first decide whether to disable the checkbox or not its checked state is not correct: https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/browser/components/preferences/in-content/main.js#1193-1194

We then start polling for the default browser check. The first poll happens one second after init at which point the chekbox is correctly checked and so we disable the checkbox: https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/browser/components/preferences/in-content/main.js#316-334

I'm not really sure what is going on with the locked state, haven't looked into that too much.

We really shouldn't be calling a category's init function until after the preferences have been applied. Preferences.updateAllElements should return the promise it creates and we should await on it before calling init().

Flags: needinfo?(dtownsend)
Regressed by: 1520350

(In reply to Dave Townsend [:mossop] (he/him) from comment #2)

We really shouldn't be calling a category's init function until after the preferences have been applied. Preferences.updateAllElements should return the promise it creates and we should await on it before calling init().

Yeah, not doing this seems bad. :-(

Jared, are you still busy with skyline things or would you have time to look into this?

Flags: needinfo?(jaws)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Priority: -- → P1

Gijs, are you able to take this over or find someone who can?

Assignee: jaws → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)

Gijs, do you have an update on this one? Thanks

Flags: needinfo?(gijskruitbosch+bugs)

Sorry, I don't yet - I took this over from Jared when he went on leave. I think the current state is sloppy but not critical to fix, so it's taken a backseat to my other priorities.

Per discussion on the bug and on phabricator for the revision Jared had up, I think fixing this is prone to causing other regressions because some of the code surrounding how we initialize things in the prefs is fragile, so I think at this point it's unlikely that we'll have something for 71. Let me know if you think the priority here should be higher...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(pascalc)
See Also: → 1526274

Too late for 72, but we can still take a fix for 73 or 74.
I'm marking this fix-optional in order to remove it from weekly regression triage.

Jared, do you have cycles to take this back? :-)

Flags: needinfo?(jaws)
Assignee: gijskruitbosch+bugs → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Attachment #9097425 - Attachment is obsolete: true
Has Regression Range: --- → yes
Flags: needinfo?(pascalc)

I've filed bug 1615348 to get a full fix in here related to comment #2.

See Also: → 1615348
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/edcb067bb8ea Default the 'Always check' checkbox to disabled until we load the default browser state to prevent accidental user changes. r=Gijs

Backed out for failures on browser_defaultbrowser_alwayscheck.js

backout: https://hg.mozilla.org/integration/autoland/rev/0520df0edda86a58efec5b14520a8bdc6b29d2d3

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=edcb067bb8ea668f4e63dff10f577b758a00dd3b&selectedJob=288764718

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=288764718&repo=autoland&lineNumber=7335

[task 2020-02-13T17:19:32.535Z] 17:19:32 INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js | 'Always Check is disabled because it's locked - true == true -
[task 2020-02-13T17:19:32.536Z] 17:19:32 INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js | The pref is locked and so doesn't get changed - true == true -
[task 2020-02-13T17:19:32.537Z] 17:19:32 INFO - Leaving test bound clicking_make_default_checks_alwaysCheck_checkbox
[task 2020-02-13T17:19:32.538Z] 17:19:32 INFO - Entering test bound make_default_disabled_until_prefs_are_loaded
[task 2020-02-13T17:19:32.538Z] 17:19:32 INFO - Buffered messages finished
[task 2020-02-13T17:19:32.539Z] 17:19:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js | 'Always Check' is disabled on page load - false == true -
[task 2020-02-13T17:19:32.539Z] 17:19:32 INFO - Stack trace:
[task 2020-02-13T17:19:32.539Z] 17:19:32 INFO - resource://testing-common/content-task.js line 110 > eval:null:5
[task 2020-02-13T17:19:32.539Z] 17:19:32 INFO - resource://testing-common/content-task.js:null:111
[task 2020-02-13T17:19:32.540Z] 17:19:32 INFO - GECKO(1950) | [Child 2105: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x7ffba4899000 == 0 [pid = 2105] [id = {f18084a7-f101-463a-a763-1aa7da068cba}] [url = about:blank]
[task 2020-02-13T17:19:32.541Z] 17:19:32 INFO - GECKO(1950) | [Child 2105: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 1 (0x7ffba534c100) [pid = 2105] [serial = 57] [outer = (nil)] [url = about:blank]
[task 2020-02-13T17:19:32.542Z] 17:19:32 INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js | 'Always Check' is enabled after default browser updated - true == true -

Flags: needinfo?(jaws)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jaws, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa7fc31eff7a Default the 'Always check' checkbox to disabled until we load the default browser state to prevent accidental user changes. r=Gijs,preferences-reviewers
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: