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)
Tracking
()
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)
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
- Launch Firefox.
- Access the about:preferences page.
- Click the "Make Default" button. (in order to lock the "Always check if Firefox is your default browser checkbox).
- Refresh the about:preferences page.
- 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.
Comment 1•6 years ago
|
||
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?
Comment 2•6 years ago
|
||
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().
Comment 3•6 years ago
|
||
(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?
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
Gijs, are you able to take this over or find someone who can?
Updated•6 years ago
|
Comment 6•6 years ago
|
||
Gijs, do you have an update on this one? Thanks
Comment 7•6 years ago
|
||
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...
![]() |
||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 10•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 11•6 years ago
|
||
I've filed bug 1615348 to get a full fix in here related to comment #2.
Comment 12•6 years ago
|
||
![]() |
||
Comment 13•6 years ago
|
||
Backed out for failures on browser_defaultbrowser_alwayscheck.js
backout: https://hg.mozilla.org/integration/autoland/rev/0520df0edda86a58efec5b14520a8bdc6b29d2d3
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 -
Comment 14•6 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Comment 15•5 years ago
|
||
![]() |
||
Comment 16•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•