Closed
Bug 1420514
Opened 7 years ago
Closed 6 years ago
Remove or hide the update features in the preferences - remove autoupdate
Categories
(Firefox :: Settings UI, enhancement, P3)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Firefox 63
People
(Reporter: Sylvestre, Assigned: bytesized)
References
Details
(Whiteboard: [ux feature][To know how to disable updates, please see comment #c137])
User Story
Rationale: The "Never check for Updates" option is easy to enable and forget about. Once enabled this exposes users to severe security issues and it seems unwise to expose this feature in the preferences? This also contributes to orphaned users. Requirements: - Remove "Never check for Updates" option - Keep "Check for updates but let me chose to install them" so users can remain in control of what gets installed on their devices. - Updates can be disabled via the policy engine
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
chutten
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
robert.strong.bugs
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
robert.strong.bugs
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
lina
:
review+
chutten
:
review+
jmaher
:
review+
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
Felipe
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
robert.strong.bugs
:
review+
|
Details |
We should remove (or move under an advanced option) the options to disable updates. I don't see any good in this option as we fix critical issues in every release of Firefox. Of course, the options should remain available under a pref but not available to lambda users.
Reporter | ||
Updated•7 years ago
|
Summary: Remove or hide the update features → Remove or hide the update features in the preferences
Reporter | ||
Updated•7 years ago
|
Comment 2•7 years ago
|
||
Punting to RT to look into. I don't know what to think, I'd need to see data to better understand if this is worth doing. I'm mildly more interested in hiding the pref under advanced.
Flags: needinfo?(jgriffiths) → needinfo?(rtestard)
Comment 3•7 years ago
|
||
(In reply to Jeff Griffiths (:canuckistani) (:β‘οΈ) from comment #2) > I'm mildly more interested in hiding the pref under advanced. Side note, we don't have an "Advanced" in Prefs anymore, though we could create one.
Priority: -- → P3
Comment 4•7 years ago
|
||
(In reply to Jeff Griffiths (:canuckistani) (:β‘οΈ) from comment #2) > Punting to RT to look into. I don't know what to think, I'd need to see data > to better understand if this is worth doing. > > I'm mildly more interested in hiding the pref under advanced. I updated the user story to clarify what's needed here. I'll discuss this with Ddurst team in Q1 planning.
User Story: (updated)
Flags: needinfo?(rtestard)
Reporter | ||
Updated•7 years ago
|
Severity: major → normal
Priority: P1 → P3
![]() |
||
Updated•7 years ago
|
Whiteboard: [ux feature]
Assignee | ||
Updated•6 years ago
|
Blocks: update-prefs
Assignee | ||
Updated•6 years ago
|
User Story: (updated)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ksteuber
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7c65b369b8c2e62bc76241f2f5f95222b38f366
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80de33dacfd0e9cf3becc7759224d64b8b098d57
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddfa757cbc37a38dbc8c704e18d5dd679887927b
Assignee | ||
Comment 8•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23cb056f7db216e27aa65aa63a39ae1dc17ce8f
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bd6f5d13e34cf2090696e6c11fe397fb9f2ba89
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7dcd7ca9ae66d726dade8cfbd5770f12719e207
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41c602f93d0ceaa01067aca908528ae18aaa42c1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8979379 -
Flags: review?(felipc)
Attachment #8979379 -
Flags: review?(ato)
Attachment #8979643 -
Flags: review?(robert.strong.bugs)
Attachment #8979377 -
Flags: review?(chutten)
Attachment #8979644 -
Flags: review?(ato)
Attachment #8979645 -
Flags: review?(felipc)
Attachment #8979646 -
Flags: review?(robert.strong.bugs)
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8979377 [details] Bug 1420514 - Remove telemetry probes tracking app.update.enabled https://reviewboard.mozilla.org/r/245538/#review251932 LGTM
Attachment #8979377 -
Flags: review?(chutten) → review+
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8979378 [details] Bug 1420514 - Remove the UI for app.update.enabled in about:preferences https://reviewboard.mozilla.org/r/245540/#review252238
Attachment #8979378 -
Flags: review?(jaws) → review+
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness https://reviewboard.mozilla.org/r/245788/#review252864 ::: js/src/tests/user.js:1 (Diff revision 1) > -user_pref("app.update.enabled", false); > +user_pref("app.update.disabledForTesting", true); I canβt review changes to this file. ::: testing/geckodriver/src/prefs.rs:19 (Diff revision 1) > > // Disable automatic downloading of new releases > ("app.update.auto", Pref::new(false)), > > // Disable automatically upgrading Firefox > - ("app.update.enabled", Pref::new(false)), > + ("app.update.disabledForTesting", Pref::new(true)), geckodriver is a program used out-of-tree by end-users and to maintain backwards compatibility with earlier Firefoxen, we need to keep the old app.update.enabled preference around until 62 reaches stable. Can you add app.update.enabled back in with a comment that it can be removed when 62 becomes stable? ::: testing/geckodriver/src/prefs.rs:20 (Diff revision 1) > // Disable automatic downloading of new releases > ("app.update.auto", Pref::new(false)), > > // Disable automatically upgrading Firefox > - ("app.update.enabled", Pref::new(false)), > + ("app.update.disabledForTesting", Pref::new(true)), > + ("security.turn_off_all_security_so_that_viruses_can_take_over_this_computer", Pref::new(true)), Where did this come from? We removed this in the past because it got removed from Gecko. ::: testing/marionette/client/marionette_driver/geckoinstance.py:505 (Diff revision 1) > > > class DesktopInstance(GeckoInstance): > desktop_prefs = { > # Disable application updates > - "app.update.enabled": False, > + "app.update.disabledForTesting": True, For the same reasons as with geckodriver, the Marionette Python client is used for Firefox update tests and needs to be backwards compatible with earlier Firefoxen back to ESR. Can you add app.update.enabled back with a note saying it can be removed when 62 becomes ESR? ::: testing/marionette/components/marionette.js:68 (Diff revision 1) > // Disable automatically upgrading Firefox. > // > // This should also be set in the profile prior to starting Firefox, > // as it is picked up at runtime. > - ["app.update.enabled", false], > + ["app.update.disabledForTesting", true], > + ["security.turn_off_all_security_so_that_viruses_can_take_over_this_computer", true], Why is this needed? ::: testing/raptor/test/test_raptor.py:26 (Diff revision 1) > assert isinstance(raptor.profile, BaseProfile) > if app != 'firefox': > return > > - # This pref is set in mozprofile > - firefox_pref = 'user_pref("app.update.enabled", false);' > + # These prefs are set in mozprofile > + firefox_prefs = [ For testing/raptor you should request r? from jmaher, as Iβm not a peer. ::: testing/talos/talos/unittests/test_talosconfig_browser_config.json:1 (Diff revision 1) > -{'deviceroot': '', 'dirs': {}, 'repository': 'http://hg.mozilla.org/releases/mozilla-release', 'buildid': '20131205075310', 'results_log': 'pathtoresults_log', 'symbols_path': None, 'bcontroller_config': 'pathtobcontroller', 'host': '', 'browser_name': 'Firefox', 'sourcestamp': '39faf812aaec', 'remote': False, 'child_process': 'plugin-container', 'branch_name': '', 'browser_version': '26.0', 'extra_args': '', 'develop': True, 'preferences': {'browser.display.overlaynavbuttons': False, 'extensions.getAddons.get.url': 'http://127.0.0.1/extensions-dummy/repositoryGetURL', 'dom.max_chrome_script_run_time': 0, 'network.proxy.type': 1, 'extensions.update.background.url': 'http://127.0.0.1/extensions-dummy/updateBackgroundURL', 'network.proxy.http': 'localhost', 'plugins.update.url': 'http://127.0.0.1/plugins-dummy/updateCheckURL', 'dom.max_script_run_time': 0, 'extensions.update.enabled': False, 'browser.safebrowsing.keyURL': 'http://127.0.0.1/safebrowsing-dummy/newkey', 'media.navigator.permission.disabled': True, 'app.update.enabled': False, 'extensions.blocklist.url': 'http://127.0.0.1/extensions-dummy/blocklistURL', 'browser.EULA.override': True, 'extensions.checkCompatibility': False, 'talos.logfile': 'pathtofile', 'browser.safebrowsing.gethashURL': 'http://127.0.0.1/safebrowsing-dummy/gethash', 'extensions.hotfix.url': 'http://127.0.0.1/extensions-dummy/hotfixURL', 'dom.disable_window_move_resize': True, 'network.proxy.http_port': 80, 'browser.dom.window.dump.enabled': True, 'extensions.update.url': 'http://127.0.0.1/extensions-dummy/updateURL', 'browser.chrome.dynamictoolbar': False, 'browser.link.open_newwindow': 2, 'browser.cache.disk.smart_size.first_run': False, 'security.turn_off_all_security_so_that_viruses_can_take_over_this_computer': True, 'dom.disable_open_during_load': False, 'extensions.getAddons.search.browseURL': 'http://127.0.0.1/extensions-dummy/repositoryBrowseURL', 'browser.cache.disk.smart_size.enabled': False, 'extensions.getAddons.getWithPerformance.url': 'http://127.0.0.1/extensions-dummy/repositoryGetWithPerformanceURL', 'hangmonitor.timeout': 0, 'dom.send_after_paint_to_content': True, 'security.fileuri.strict_origin_policy': False, 'media.capturestream_hints.enabled': True, 'extensions.update.notifyUser': False, 'extensions.blocklist.enabled': False, 'browser.bookmarks.max_backups': 0, 'browser.shell.checkDefaultBrowser': False, 'media.peerconnection.enabled': True, 'dom.disable_window_flip': True, 'security.enable_java': False, 'browser.warnOnQuit': False, 'media.navigator.enabled': True, 'browser.safebrowsing.updateURL': 'http://127.0.0.1/safebrowsing-dummy/update', 'dom.allow_scripts_to_close_windows': True, 'extensions.webservice.discoverURL': 'http://127.0.0.1/extensions-dummy/discoveryURL'}, 'test_timeout': 1200, 'title': 'qm-pxp01', 'error_filename': 'pathtoerrorfile', 'webserver': 'localhost:15707', 'browser_path':ffox_path, 'port': 20701, 'browser_log': 'browser_output.txt', 'process': 'firefox.exe', 'xperf_path': 'C:/Program Files/Microsoft Windows Performance Toolkit/xperf.exe', 'extensions': ['pathtopageloader'], 'fennecIDs': '', 'env': {'NO_EM_RESTART': '1'}, 'init_url': 'http://localhost:15707/getInfo.html', 'browser_wait': 5} > +{'deviceroot': '', 'dirs': {}, 'repository': 'http://hg.mozilla.org/releases/mozilla-release', 'buildid': '20131205075310', 'results_log': 'pathtoresults_log', 'symbols_path': None, 'bcontroller_config': 'pathtobcontroller', 'host': '', 'browser_name': 'Firefox', 'sourcestamp': '39faf812aaec', 'remote': False, 'child_process': 'plugin-container', 'branch_name': '', 'browser_version': '26.0', 'extra_args': '', 'develop': True, 'preferences': {'browser.display.overlaynavbuttons': False, 'extensions.getAddons.get.url': 'http://127.0.0.1/extensions-dummy/repositoryGetURL', 'dom.max_chrome_script_run_time': 0, 'network.proxy.type': 1, 'extensions.update.background.url': 'http://127.0.0.1/extensions-dummy/updateBackgroundURL', 'network.proxy.http': 'localhost', 'plugins.update.url': 'http://127.0.0.1/plugins-dummy/updateCheckURL', 'dom.max_script_run_time': 0, 'extensions.update.enabled': False, 'browser.safebrowsing.keyURL': 'http://127.0.0.1/safebrowsing-dummy/newkey', 'media.navigator.permission.disabled': True, 'app.update.disabledForTesting': True, 'extensions.blocklist.url': 'http://127.0.0.1/extensions-dummy/blocklistURL', 'browser.EULA.override': True, 'extensions.checkCompatibility': False, 'talos.logfile': 'pathtofile', 'browser.safebrowsing.gethashURL': 'http://127.0.0.1/safebrowsing-dummy/gethash', 'extensions.hotfix.url': 'http://127.0.0.1/extensions-dummy/hotfixURL', 'dom.disable_window_move_resize': True, 'network.proxy.http_port': 80, 'browser.dom.window.dump.enabled': True, 'extensions.update.url': 'http://127.0.0.1/extensions-dummy/updateURL', 'browser.chrome.dynamictoolbar': False, 'browser.link.open_newwindow': 2, 'browser.cache.disk.smart_size.first_run': False, 'security.turn_off_all_security_so_that_viruses_can_take_over_this_computer': True, 'dom.disable_open_during_load': False, 'extensions.getAddons.search.browseURL': 'http://127.0.0.1/extensions-dummy/repositoryBrowseURL', 'browser.cache.disk.smart_size.enabled': False, 'extensions.getAddons.getWithPerformance.url': 'http://127.0.0.1/extensions-dummy/repositoryGetWithPerformanceURL', 'hangmonitor.timeout': 0, 'dom.send_after_paint_to_content': True, 'security.fileuri.strict_origin_policy': False, 'media.capturestream_hints.enabled': True, 'extensions.update.notifyUser': False, 'extensions.blocklist.enabled': False, 'browser.bookmarks.max_backups': 0, 'browser.shell.checkDefaultBrowser': False, 'media.peerconnection.enabled': True, 'dom.disable_window_flip': True, 'security.enable_java': False, 'browser.warnOnQuit': False, 'media.navigator.enabled': True, 'browser.safebrowsing.updateURL': 'http://127.0.0.1/safebrowsing-dummy/update', 'dom.allow_scripts_to_close_windows': True, 'extensions.webservice.discoverURL': 'http://127.0.0.1/extensions-dummy/discoveryURL'}, 'test_timeout': 1200, 'title': 'qm-pxp01', 'error_filename': 'pathtoerrorfile', 'webserver': 'localhost:15707', 'browser_path':ffox_path, 'port': 20701, 'browser_log': 'browser_output.txt', 'process': 'firefox.exe', 'xperf_path': 'C:/Program Files/Microsoft Windows Performance Toolkit/xperf.exe', 'extensions': ['pathtopageloader'], 'fennecIDs': '', 'env': {'NO_EM_RESTART': '1'}, 'init_url': 'http://localhost:15707/getInfo.html', 'browser_wait': 5} For testing/talos you should also request review from jmaher. ::: testing/tps/tps/testrunner.py:59 (Diff revision 1) > 'MOZ_NO_REMOTE': '1', > 'XPCOM_DEBUG_BREAK': 'warn', > } > > default_preferences = { > - 'app.update.enabled': False, > + 'app.update.disabledForTesting': True, Please request review from whimboo on testing/tps. ::: toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py (Diff revision 1) > - def toggle_update_pref(self): > - value = self.marionette.get_pref('app.update.enabled') > - self.marionette.enforce_gecko_prefs({'app.update.enabled': not value}) Iβm not a peer of Telemetry, so I canβt review this.
Attachment #8979644 -
Flags: review?(ato) → review-
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8979645 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in enterprise policy tests https://reviewboard.mozilla.org/r/245790/#review253064
Attachment #8979645 -
Flags: review?(felipc) → review+
![]() |
||
Comment 29•6 years ago
|
||
mozreview-review |
Comment on attachment 8979379 [details] Bug 1420514 - Remove app.update.enabled from prefs files https://reviewboard.mozilla.org/r/245542/#review253808 ::: browser/app/profile/firefox.js (Diff revision 3) > // user of the failure. User initiated update checks always notify the user of > // the failure. > pref("app.update.backgroundMaxErrors", 10); > > -// Whether or not app updates are enabled > -#ifdef MOZ_UPDATER Please add a comment in bug 1427471 about this being removed. ::: mobile/android/app/mobile.js (Diff revision 3) > pref("app.update.autodownload", "wifi"); > pref("app.update.url.android", "https://aus5.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%MOZ_VERSION%/update.xml"); > > #ifdef MOZ_UPDATER > /* prefs used specifically for updating the app */ > -pref("app.update.enabled", false); Android has its own update mechanism and it may resuse this pref so please check with an Android dev if it is ok to remove this. https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#29
Attachment #8979379 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Updated•6 years ago
|
Attachment #8979644 -
Flags: review?(chutten)
Assignee | ||
Comment 31•6 years ago
|
||
Adding :chutten to review toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness https://reviewboard.mozilla.org/r/245788/#review254130 r+ on toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/testcase.py for removing dead code.
Attachment #8979644 -
Flags: review?(chutten) → review+
Assignee | ||
Updated•6 years ago
|
Attachment #8979644 -
Flags: review?(jmaher)
Attachment #8979644 -
Flags: review?(hskupin)
Assignee | ||
Comment 33•6 years ago
|
||
@jmaher - Could you please review these files: js/src/tests/user.js testing/raptor/test/test_raptor.py testing/talos/talos/unittests/test_talosconfig_browser_config.json @whimboo - Could you please review testing/tps/tps/testrunner.py Sorry that I did not break up that commit better. I'm afraid that if I broke it up now, I would lose the r+'s that I have already gotten.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness https://reviewboard.mozilla.org/r/245788/#review252864 > Where did this come from? We removed this in the past because it > got removed from Gecko. As far as I know, this continues to be used to determine whether Firefox is currently running automated tests. See https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/xpconnect/src/xpcpublic.h#642 > Why is this needed? This is needed so that `Cu.isInAutomation` returns `true`, allowing us to properly determine if Firefox is currently running automated tests.
Comment 42•6 years ago
|
||
mozreview-review |
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness https://reviewboard.mozilla.org/r/245788/#review254206 I believe you got all the proper harnesses.
Attachment #8979644 -
Flags: review?(jmaher) → review+
Comment 43•6 years ago
|
||
So what are downstreams supposed to do now to avoid firefox from trying to upgrade itself when it's managed by a package manager? Set app.update.disabledForTesting?
Assignee | ||
Comment 44•6 years ago
|
||
No. `app.update.disabledForTesting` will only be available in automated testing. Frankly, I have very little understanding of how these downstreams operate, so I am not sure what is and isn't possible, but I will tell you what options they have. I think that you are referring to *nix distributions, so please correct me if I am wrong, since this information will differ for Windows and macOS. One option is to set `app.update.auto` to `false`. This would prevent automatic updates, but would prompt the user to let Firefox install any updates that could be downloaded and installed. Another option is to set an enterprise policy that would prevent app update by putting a JSON file in the install directory. I don't recommend this for a number of reasons. It doesn't feel like the right way to solve this problem. It would also result in a banner in about:preferences saying something like "Some preferences are being controlled by your organization". The thing is though, we typically don't update Linux installations that were installed via a package manager. I have been told that we have no real way of updating Firefox installations on Linux unless we can already write to the installation directory because we have no reliable, cross-platform way of elevating our privileges. Since the installation directory is typically writable only by root, but Firefox is not typically run by root, Firefox never updates. I have no idea how package manager installations work, but if you wanted to formalize this, maybe you could just end the installation by removing all write permissions from the installation directory so that it could not update, even if run as root. @mhowell - I haven't spent much time looking at update on Linux; could you confirm that what I have written about it here is correct?
Flags: needinfo?(mhowell)
Comment 45•6 years ago
|
||
(In reply to Kirk Steuber [:bytesized] from comment #44) > No. `app.update.disabledForTesting` will only be available in automated > testing. Frankly, I have very little understanding of how these downstreams > operate, so I am not sure what is and isn't possible, but I will tell you > what options they have. What do you mean by "only available in automated testing"? For mozregression, I think we want this setting enabled under most circumstances (mozregression is typically downloading possible bisection candidates while the current build is being tested, and having Firefox pointlessly download an update for an obsolete build would interfere with that).
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness https://reviewboard.mozilla.org/r/245788/#review254314 ::: testing/geckodriver/src/prefs.rs:20 (Diff revision 2) > // Disable automatic downloading of new releases > ("app.update.auto", Pref::new(false)), > > // Disable automatically upgrading Firefox > + ("app.update.disabledForTesting", Pref::new(true)), > + ("security.turn_off_all_security_so_that_viruses_can_take_over_this_computer", Pref::new(true)), This is still present since Andreas reviewed the piece of code and no explanation has been given yet. ::: testing/marionette/client/marionette_driver/geckoinstance.py:506 (Diff revision 2) > > class DesktopInstance(GeckoInstance): > desktop_prefs = { > # Disable application updates > + "app.update.disabledForTesting": True, > + "security.turn_off_all_security_so_that_viruses_can_take_over_this_computer": True, Same here. ::: testing/mozbase/mozprofile/mozprofile/profile.py:435 (Diff revision 2) > class FirefoxProfile(Profile): > """Specialized Profile subclass for Firefox""" > > preferences = { # Don't automatically update the application > - 'app.update.enabled': False, > + 'app.update.disabledForTesting': True, > + 'security.turn_off_all_security_so_that_viruses_can_take_over_this_computer': True, Same here. ::: testing/tps/tps/testrunner.py:60 (Diff revision 2) > 'XPCOM_DEBUG_BREAK': 'warn', > } > > default_preferences = { > - 'app.update.enabled': False, > + 'app.update.disabledForTesting': True, > + 'security.turn_off_all_security_so_that_viruses_can_take_over_this_computer': True, Sorry, but I'm not a valid reviewer for TPS. Best is always to check the Bugzilla component who reviews patches at the moment. AFAIR you could ask Kit Cambridge.
Attachment #8979644 -
Flags: review?(hskupin) → review-
Comment 47•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #43) > So what are downstreams supposed to do now to avoid firefox from trying to > upgrade itself when it's managed by a package manager? Set > app.update.disabledForTesting? In addition to Kirk's reply, distributions operating a package manager do not tend to package Mozilla builds, and the best way to handle this when using a custom build is to disable update support at build time, as is most often what is already done today. (In reply to Kirk Steuber [:bytesized] from comment #44) > @mhowell - I haven't spent much time looking at update on Linux; could you > confirm that what I have written about it here is correct? It's correct, to the extent that you can use the word "typical" to really describe anything covering all of Linux; I think it's more common for manually installed Linux builds to end up in a home directory, in which case they are writable and updates work fine.
Flags: needinfo?(mhowell)
Assignee | ||
Comment 48•6 years ago
|
||
(In reply to William Lachance (:wlach) (use needinfo!) from comment #45) > What do you mean by "only available in automated testing"? We determine if we are in automated testing based on whether the pref `security.turn_off_all_security_so_that_viruses_can_take_over_this_computer` is set to true and the environment variable "MOZ_DISABLE_NONLOCAL_CONNECTIONS" is set. This is existing functionality that is available via the `xpc::IsInAutomation` C++ function or the `Cu.isInAutomation` JS function. See the implementation of `xpc::IsInAutomation`: https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/xpconnect/src/xpcpublic.h#642
Assignee | ||
Updated•6 years ago
|
Attachment #8979644 -
Flags: review?(ato) → review?(kit)
Assignee | ||
Comment 49•6 years ago
|
||
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness K
Attachment #8979644 -
Flags: review- → review?(ato)
Assignee | ||
Comment 50•6 years ago
|
||
Kit - If possible, could you please review testing/tps/tps/testrunner.py Sorry that I did not break up that commit better. I'm afraid that if I broke it up now, I would lose the r+'s that I have already gotten.
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness https://reviewboard.mozilla.org/r/245788/#review254484 Looks good! As far as TPS goes, review from a test or automation peer is totally sufficient for small tweaks like this, especially if they're part of a refactor that makes similar changes elsewhere...but, of course, you're also welcome to ask for review if you'd like. :-) Thanks!
Attachment #8979644 -
Flags: review?(kit) → review+
Comment 52•6 years ago
|
||
(In reply to Kirk Steuber [:bytesized] from comment #48) > is set to true and the environment variable > "MOZ_DISABLE_NONLOCAL_CONNECTIONS" is set. This is existing functionality > that is available via the `xpc::IsInAutomation` C++ function or the > `Cu.isInAutomation` JS function. Please note that this doesn't apply to any existing harness. Especially Marionette doesn't support this environment variable yet, and for firefox-ui-tests we specifically don't want to have it set because the tier-2 tests have to access remote content.
Comment 53•6 years ago
|
||
mozreview-review |
Comment on attachment 8979644 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in the test harness https://reviewboard.mozilla.org/r/245788/#review254722 r+wc that you drop the virus pref from prefs.rs.
Attachment #8979644 -
Flags: review?(ato) → review+
Assignee | ||
Comment 54•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=363493063ddb50bbce079368e85e8fe52f1cf0ec
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
||
Comment 58•6 years ago
|
||
mozreview-review |
Comment on attachment 8979643 [details] Bug 1420514 - Remove pref app.update.enabled from the update mechanism https://reviewboard.mozilla.org/r/245786/#review256662 ::: browser/base/content/aboutDialog-appUpdater.js:139 (Diff revision 2) > return this.um.activeUpdate && > this.um.activeUpdate.state == "downloading"; > }, > > // true when updating is disabled by an administrator. > - get updateDisabledAndLocked() { > + get updateDisabledByAdmin() { I'm ok with updateDisabledByAdmin for the name but perhaps updateDisabledByPolicy would be better. If it is changed it would be good to change the panel name from adminDisabled to policyDisabled. ::: browser/base/content/aboutDialog-appUpdater.js:145 (Diff revision 2) > - !Services.policies.isAllowed("appUpdate")); > }, > > // true when updating is enabled. > get updateEnabled() { > - try { > + return !Services.policies || Services.policies.isAllowed("appUpdate"); Would be better to just combine updateEnabled with updateDisabledByAdmin or updateDisabledByPolicy if you change the name and call that instead. ::: browser/components/nsBrowserGlue.js:737 (Diff revision 2) > _checkForOldBuildUpdates() { > // check for update if our build is old > + let updateService = Cc["@mozilla.org/updates/update-service;1"]. > + getService(Ci.nsIApplicationUpdateService); > if (AppConstants.MOZ_UPDATER && > - Services.prefs.getBoolPref("app.update.enabled") && > + updateService.canCheckForUpdates && This is going to cause the update service to initialize on every startup. I think this check might not be necessary if nsUpdateService.js handles it properly which I think it does. This will also give us more data for this case in telemetry. ::: toolkit/mozapps/update/content/updates.js:568 (Diff revision 2) > - > - if (Services.prefs.getBoolPref(PREF_APP_UPDATE_ENABLED, true)) > + let updateService = Cc["@mozilla.org/updates/update-service;1"]. > + getService(Ci.nsIApplicationUpdateService); > + if (updateService.canCheckForUpdates) > document.getElementById("noUpdatesAutoEnabled").hidden = false; > else > document.getElementById("noUpdatesAutoDisabled").hidden = false; When canCheckForUpdates is false this will display the following message which I don't think is approprite with this change since a user won't be able to easily enable update checking when it is disabled by policy. There are no updates available. Please check again later or enable &brandShortName;'s automatic update checking. Also, canCheckForUpdates has several other checks beyond just checking the pref. I suspect you just want the policy check above vs. canCheckForUpdates and a new string for communicating this state. ::: toolkit/mozapps/update/nsIUpdateService.idl:278 (Diff revision 2) > * @param force > * Forces the checker to check for updates, regardless of the > * current value of the user's update settings. This is used by > * any piece of UI that offers the user the imperative option to > * check for updates now, regardless of their update settings. > - * force will not work if the system administrator has locked > + * force will not work if updates are disabled by policy. nit: please change the comment to something along the lines of: Specifying true for the force param will not check for updates when updates are disabled by policy. ::: toolkit/mozapps/update/nsUpdateService.js:35 (Diff revision 2) > const PREF_APP_UPDATE_DOWNLOAD_MAXATTEMPTS = "app.update.download.maxAttempts"; > const PREF_APP_UPDATE_ELEVATE_NEVER = "app.update.elevate.never"; > const PREF_APP_UPDATE_ELEVATE_VERSION = "app.update.elevate.version"; > const PREF_APP_UPDATE_ELEVATE_ATTEMPTS = "app.update.elevate.attempts"; > const PREF_APP_UPDATE_ELEVATE_MAXATTEMPTS = "app.update.elevate.maxAttempts"; > -const PREF_APP_UPDATE_ENABLED = "app.update.enabled"; > +const PREF_APP_UPDATE_DISABLED_FOR_TEST = "app.update.disabledForTesting"; Are all of the cases where update checks shouldn't be performed in automation handled in the other patches? ::: toolkit/mozapps/update/nsUpdateService.js (Diff revision 2) > // UPDATE_LAST_NOTIFY_INTERVAL_DAYS_NOTIFY > AUSTLMY.pingLastUpdateTime(this._pingSuffix); > // Histogram IDs: > - // UPDATE_NOT_PREF_UPDATE_ENABLED_EXTERNAL > - // UPDATE_NOT_PREF_UPDATE_ENABLED_NOTIFY > - AUSTLMY.pingBoolPref("UPDATE_NOT_PREF_UPDATE_ENABLED_" + this._pingSuffix, UPDATE_NOT_PREF_UPDATE_ENABLED_etc. need to be removed or commented out in UpdateTelemetry.jsm ::: toolkit/mozapps/update/nsUpdateService.js:2064 (Diff revision 2) > AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_NO_OS_ABI); > } else if (!validUpdateURL) { > AUSTLMY.pingCheckCode(this._pingSuffix, > AUSTLMY.CHK_INVALID_DEFAULT_URL); > - } else if (!Services.prefs.getBoolPref(PREF_APP_UPDATE_ENABLED, true)) { > + } else if (this.disabledByAdmin) { > AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_PREF_DISABLED); Please create a new code instead of re-using CHK_PREF_DISABLED ::: toolkit/mozapps/update/nsUpdateService.js (Diff revision 2) > } else if (!hasUpdateMutex()) { > AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_NO_MUTEX); > - } else if (!gCanCheckForUpdates) { > + } else if (!this.canCheckForUpdates) { > AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_UNABLE_TO_CHECK); > - } else if (!this.backgroundChecker._enabled) { > - AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_DISABLED_FOR_SESSION); CHK_DISABLED_FOR_SESSION needs to be removed or commented out in UpdateTelemetry.jsm ::: toolkit/mozapps/update/nsUpdateService.js:2311 (Diff revision 2) > + get disabledForTesting() { > + return Cu.isInAutomation && > + Services.prefs.getBoolPref(PREF_APP_UPDATE_DISABLED_FOR_TEST, false); > + }, > + > + get disabledByAdmin() { As previously mentioned, I'm ok with disabledByAdmin for the name but perhaps disabledByPolicy would be better. ::: toolkit/mozapps/update/nsUpdateService.js:2965 (Diff revision 2) > if (!listener) { > throw Cr.NS_ERROR_NULL_POINTER; > } > > - if (!this.enabled && !force) { > + let UpdateServiceInstance = UpdateServiceFactory.createInstance(); > + if (UpdateServiceInstance.disabledByAdmin || Please add a comment that canCheckForUpdates also calls disabledByAdmin. I've spent a decent amount of time trying to recall why it doesn't return early when canCheckForUpdates is false and force is true. I am fairly certain it is so we show errors during a check performed via the UI. Please add a comment to this affect.
Attachment #8979643 -
Flags: review?(robert.strong.bugs) → review-
![]() |
||
Comment 59•6 years ago
|
||
mozreview-review |
Comment on attachment 8979643 [details] Bug 1420514 - Remove pref app.update.enabled from the update mechanism https://reviewboard.mozilla.org/r/245786/#review257364 ::: toolkit/mozapps/update/nsUpdateService.js:35 (Diff revision 2) > const PREF_APP_UPDATE_DOWNLOAD_MAXATTEMPTS = "app.update.download.maxAttempts"; > const PREF_APP_UPDATE_ELEVATE_NEVER = "app.update.elevate.never"; > const PREF_APP_UPDATE_ELEVATE_VERSION = "app.update.elevate.version"; > const PREF_APP_UPDATE_ELEVATE_ATTEMPTS = "app.update.elevate.attempts"; > const PREF_APP_UPDATE_ELEVATE_MAXATTEMPTS = "app.update.elevate.maxAttempts"; > -const PREF_APP_UPDATE_ENABLED = "app.update.enabled"; > +const PREF_APP_UPDATE_DISABLED_FOR_TEST = "app.update.disabledForTesting"; nit: please name this pref PREF_APP_UPDATE_DISABLEDFORTESTING
![]() |
||
Comment 60•6 years ago
|
||
mozreview-review |
Comment on attachment 8979646 [details] Bug 1420514 - Replace app.update.enabled with app.update.disabledForTesting in update tests https://reviewboard.mozilla.org/r/245792/#review257366 ::: toolkit/mozapps/update/tests/data/shared.js:20 (Diff revision 3) > const PREF_APP_UPDATE_CHANNEL = "app.update.channel"; > const PREF_APP_UPDATE_DOORHANGER = "app.update.doorhanger"; > const PREF_APP_UPDATE_DOWNLOADPROMPTATTEMPTS = "app.update.download.attempts"; > const PREF_APP_UPDATE_DOWNLOADPROMPTMAXATTEMPTS = "app.update.download.maxAttempts"; > const PREF_APP_UPDATE_DOWNLOADBACKGROUNDINTERVAL = "app.update.download.backgroundInterval"; > -const PREF_APP_UPDATE_ENABLED = "app.update.enabled"; > +const PREF_APP_UPDATE_DISABLED = "app.update.disabledForTesting"; Please name this pref PREF_APP_UPDATE_DISABLEDFORTESTING
Attachment #8979646 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 61•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8979643 [details] Bug 1420514 - Remove pref app.update.enabled from the update mechanism https://reviewboard.mozilla.org/r/245786/#review256662 > When canCheckForUpdates is false this will display the following message which I don't think is approprite with this change since a user won't be able to easily enable update checking when it is disabled by policy. > There are no updates available. Please check again later or enable &brandShortName;'s automatic update checking. > > Also, canCheckForUpdates has several other checks beyond just checking the pref. > > I suspect you just want the policy check above vs. canCheckForUpdates and a new string for communicating this state. As discussed on IRC, this code should not actually run when app update is disabled by policy, and the "noUpdateAutoDisabled" message should really be disaplayed based on the value of `app.update.auto` rather than `app.update.enabled`. Therefore I will change this code to check that pref instead. > Are all of the cases where update checks shouldn't be performed in automation handled in the other patches? They are. > UPDATE_NOT_PREF_UPDATE_ENABLED_etc. need to be removed or commented out in UpdateTelemetry.jsm This is done in the subsequent patch titled "Remove telemetry probes tracking app.update.enabled" > Please create a new code instead of re-using CHK_PREF_DISABLED This is also done in the patch titled "Remove telemetry probes tracking app.update.enabled" > CHK_DISABLED_FOR_SESSION needs to be removed or commented out in UpdateTelemetry.jsm Also done in the patch titled "Remove telemetry probes tracking app.update.enabled" > Please add a comment that canCheckForUpdates also calls disabledByAdmin. > > I've spent a decent amount of time trying to recall why it doesn't return early when canCheckForUpdates is false and force is true. I am fairly certain it is so we show errors during a check performed via the UI. Please add a comment to this affect. I wrote a slightly different comment documenting the reasoning behind why I wrote the code like that. Let me know if you don't like it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8979378 -
Flags: review+ → review?(jaws)
![]() |
||
Comment 76•6 years ago
|
||
mozreview-review |
Comment on attachment 8979643 [details] Bug 1420514 - Remove pref app.update.enabled from the update mechanism https://reviewboard.mozilla.org/r/245786/#review258906 I'm r+ing this since I'm ok with this for the most part. I'd like to get your take on throwing for disabledByPolicy in checkForUpdates. ::: toolkit/mozapps/update/content/updates.js:20 (Diff revision 3) > const XMLNS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > > const PREF_APP_UPDATE_BACKGROUNDERRORS = "app.update.backgroundErrors"; > const PREF_APP_UPDATE_CERT_ERRORS = "app.update.cert.errors"; > const PREF_APP_UPDATE_ELEVATE_NEVER = "app.update.elevate.never"; > -const PREF_APP_UPDATE_ENABLED = "app.update.enabled"; > +const PREF_APP_UPDATE_AUTO = "app.update.auto"; nit: these are sorted so please add it to the top. ::: toolkit/mozapps/update/nsUpdateService.js:2969 (Diff revision 3) > + // seem a bit odd, but it is conceptually correct. The |force| parameter > + // is specified when the user manually requested an update check, so it is > + // meant to override anything preventing update checks (including > + // user settings that might prevent update). However, enterprise policies > + // are meant to override the user's actions. Therefore, we must exit early > + // update is disabled by policy, but any other considerations can be nit: I'd like this comment to be more concise. Also, "Therefore, we must exit early update is disabled by policy, but any other considerations can be overridden by |force|." I suspect you meant, we must exit early *when* update is disabled by policy, etc. ::: toolkit/mozapps/update/nsUpdateService.js:2971 (Diff revision 3) > + // meant to override anything preventing update checks (including > + // user settings that might prevent update). However, enterprise policies > + // are meant to override the user's actions. Therefore, we must exit early > + // update is disabled by policy, but any other considerations can be > + // overridden by |force|. > + if (UpdateServiceInstance.disabledByPolicy || What do you think of throwing when disabledByPolicy is true for foreground checks? if (force && UpdateServiceInstance.disabledByPolicy) { throw etc... } if (!force && !UpdateServiceInstance.canCheckForUpdates) { return; }
Attachment #8979643 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 77•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8979643 [details] Bug 1420514 - Remove pref app.update.enabled from the update mechanism https://reviewboard.mozilla.org/r/245786/#review258906 > What do you think of throwing when disabledByPolicy is true for foreground checks? > > if (force && UpdateServiceInstance.disabledByPolicy) { > throw etc... > } > > if (!force && !UpdateServiceInstance.canCheckForUpdates) { > return; > } That seems fine. I'll change it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8979378 -
Flags: review?(jaws)
![]() |
||
Comment 84•6 years ago
|
||
(In reply to Kirk Steuber [:bytesized] from comment #77) > Comment on attachment 8979643 [details] > Bug 1420514 - Remove pref app.update.enabled from the update mechanism > > https://reviewboard.mozilla.org/r/245786/#review258906 > > > What do you think of throwing when disabledByPolicy is true for foreground checks? > > > > if (force && UpdateServiceInstance.disabledByPolicy) { > > throw etc... > > } > > > > if (!force && !UpdateServiceInstance.canCheckForUpdates) { > > return; > > } > > That seems fine. I'll change it. Please add a LOG message as well.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8979378 -
Flags: review?(jaws)
Comment 91•6 years ago
|
||
mozreview-review |
Comment on attachment 8979378 [details] Bug 1420514 - Remove the UI for app.update.enabled in about:preferences https://reviewboard.mozilla.org/r/245540/#review263454 The moving of the code to init() that checks if the maintenance service is installed looks good. Thanks!
Attachment #8979378 -
Flags: review?(jaws) → review+
Comment 92•6 years ago
|
||
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d3399edf0197 Remove app.update.enabled from prefs files r=rstrong https://hg.mozilla.org/integration/autoland/rev/de4a4592dbd9 Remove pref app.update.enabled from the update mechanism r=rstrong https://hg.mozilla.org/integration/autoland/rev/416e398d942f Remove telemetry probes tracking app.update.enabled r=chutten https://hg.mozilla.org/integration/autoland/rev/5791b17ccb82 Remove the UI for app.update.enabled in about:preferences r=jaws https://hg.mozilla.org/integration/autoland/rev/01bf643c77e5 Replace app.update.enabled with app.update.disabledForTesting in the test harness r=ato,chutten,jmaher,lina https://hg.mozilla.org/integration/autoland/rev/48ed6dfe8772 Replace app.update.enabled with app.update.disabledForTesting in enterprise policy tests r=Felipe https://hg.mozilla.org/integration/autoland/rev/5a46a221b6e8 Replace app.update.enabled with app.update.disabledForTesting in update tests r=rstrong
Comment 93•6 years ago
|
||
Backed out for breaking browser chrome on multiple files Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5a46a221b6e84446aacdd4d1ddb79118aa4dd5e5 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187888105&repo=autoland&lineNumber=4583 Backout: https://hg.mozilla.org/integration/autoland/rev/a88624b04c20029f953673b8b2e8c5451af91394
Flags: needinfo?(ksteuber)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 101•6 years ago
|
||
Working on fixing the failures. Last review push was purely a rebase update so that the interdiffs will display properly on the next patch push.
Flags: needinfo?(ksteuber)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 108•6 years ago
|
||
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3214e87404db Remove app.update.enabled from prefs files r=rstrong https://hg.mozilla.org/integration/autoland/rev/7e60005df7f9 Remove pref app.update.enabled from the update mechanism r=rstrong https://hg.mozilla.org/integration/autoland/rev/ed6333e195db Remove telemetry probes tracking app.update.enabled r=chutten https://hg.mozilla.org/integration/autoland/rev/6226afd9d35b Remove the UI for app.update.enabled in about:preferences r=jaws https://hg.mozilla.org/integration/autoland/rev/7ca13add114d Replace app.update.enabled with app.update.disabledForTesting in the test harness r=ato,chutten,jmaher,lina https://hg.mozilla.org/integration/autoland/rev/4dd0d5daf420 Replace app.update.enabled with app.update.disabledForTesting in enterprise policy tests r=Felipe https://hg.mozilla.org/integration/autoland/rev/97321f82dfbd Replace app.update.enabled with app.update.disabledForTesting in update tests r=rstrong
Comment 109•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3214e87404db https://hg.mozilla.org/mozilla-central/rev/7e60005df7f9 https://hg.mozilla.org/mozilla-central/rev/ed6333e195db https://hg.mozilla.org/mozilla-central/rev/6226afd9d35b https://hg.mozilla.org/mozilla-central/rev/7ca13add114d https://hg.mozilla.org/mozilla-central/rev/4dd0d5daf420 https://hg.mozilla.org/mozilla-central/rev/97321f82dfbd
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment 110•6 years ago
|
||
I'm running multiple profiles in parallel on Nightly, and disabled updates in all but one, so things don't get funky. Is abusing app.update.disabledForTesting the best way forward?
Comment 111•6 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #110) > I'm running multiple profiles in parallel on Nightly, and disabled updates > in all but one, so things don't get funky. > > Is abusing app.update.disabledForTesting the best way forward? No, that won't work, see comment 48. It seems like the closest thing to the old behaviour is to disable automatic applying of the updates, by setting app.update.auto to false (comment 44). This is what I'm planning to do in mozregression.
Reporter | ||
Comment 112•6 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: Feature less obvious [Affects Firefox for Android]: No [Suggested wording]: Change: Update options in about:preferences have been removed (available in the advanced preferences) [Links (documentation, blog post, etc)]: A better wording is welcome!
relnote-firefox:
--- → ?
Assignee | ||
Updated•6 years ago
|
relnote-firefox:
? → ---
Assignee | ||
Comment 113•6 years ago
|
||
Release Note Request (optional, but appreciated) [Why is this notable]: Feature less obvious [Affects Firefox for Android]: No [Suggested wording]: Change: "Never check for updates" option removed from about:preferences. If a substitute is needed, we recommend using the enterprise policy: DisableAppUpdate. [Links (documentation, blog post, etc)]: Enterprise policy documentation: https://github.com/mozilla/policy-templates/blob/master/README.md
relnote-firefox:
--- → ?
Comment 114•6 years ago
|
||
So the first two days have been quite annoying with just update notifications, as it's pestering me even more than my profile that's actually doing the update. Added a couple of zeros to the update frequency in my non-updating profile now, let's see how that goes.
Reporter | ||
Comment 116•6 years ago
|
||
Do you know what will happen for people who disabled updates? It will be much harder for them to turn them back on? Thanks
Flags: needinfo?(ksteuber)
Comment 117•6 years ago
|
||
Nightly now insists to remind constantly that there is a, well, nightly update available - is this an incentive to block all connections to Mozila with an app level firewall or the hosts file? The Nightly release notes say I'm to use the DisableAppUpdate policy, but it doesn't seem to work for me and the description says ESR only - is it enabled for Nightly, too? https://github.com/mozilla/policy-templates/blob/master/README.md
Comment 118•6 years ago
|
||
I'm trying not to get pestered by increasing app.update.interval and disabling app.update.doorhanger - if these are the prefered methods it would be helpful mention them prominently if the DisableAppUpdate policy really only works on ESR.
Comment 119•6 years ago
|
||
(In reply to Mozilla Marsu from comment #117) > The Nightly release notes say I'm to use the DisableAppUpdate policy, but it > doesn't seem to work for me and the description says ESR only - is it > enabled for Nightly, too? > https://github.com/mozilla/policy-templates/blob/master/README.md According to bug 1460082, the DisableAppUpdate policy works with non-ESR now. Please make sure that you are using the latest Nightly. And the policy template is probably outdated. Please file an issue (or even send a PR) to <https://github.com/mozilla/policy-templates>.
Assignee | ||
Comment 120•6 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] [PTO until 2018/08/12] from comment #116) > Do you know what will happen for people who disabled updates? It will be > much harder for them to turn them back on? > Thanks I think you misunderstand the change that has been made. I did not just remove an option that controls a pref; I removed the option AND the pref. People who changed this value via about:preferences or about:config will retain the value, but it will do nothing.
Flags: needinfo?(ksteuber)
Comment 121•6 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #119) > (In reply to Mozilla Marsu from comment #117) > > The Nightly release notes say I'm to use the DisableAppUpdate policy, but it > > doesn't seem to work for me and the description says ESR only - is it > > enabled for Nightly, too? > > https://github.com/mozilla/policy-templates/blob/master/README.md > > According to bug 1460082, the DisableAppUpdate policy works with non-ESR > now. Please make sure that you are using the latest Nightly. And the policy > template is probably outdated. Please file an issue (or even send a PR) to > <https://github.com/mozilla/policy-templates>. Thanks, that should cover it and I'll stop complaining :-p ... I'll check why it isn't working over here (I *am* at latest Nightly :-)), probybly b/c I'm running a PortableApps Nightly and/or I screwed up placing the policy. I didn't manage a full PR, but at least a issue: https://github.com/mozilla/policy-templates/issues/209
Comment 122•6 years ago
|
||
The Nightly chanelog says "Never check for updates option removed from about:preferences. If a substitute is needed, we recommend using the DisableAppUpdate enterprise policy" https://www.mozilla.org/en-US/firefox/63.0a1/releasenotes/ But the policy ist *not* a "substitute" because it not only disables automatic update checks, but disabled *manual* checking, too! The About page says "Updates disabled by your system administrator" which is true to the ESR legacy. This does not improve security for peope who opt out of update nagging, as now they are more prone to update even less often. Suggestion: It would be better to allow for manual checking/updating on non-ESR versions in the About windows even if the DisableAppUpdate is active, or introduce a DisableAppUpdateCheck flavor.
Comment 123•6 years ago
|
||
(In reply to Kirk Steuber [:bytesized] from comment #120) > (In reply to Sylvestre Ledru [:sylvestre] [PTO until 2018/08/12] from > comment #116) > > Do you know what will happen for people who disabled updates? It will be > > much harder for them to turn them back on? > > Thanks > > I think you misunderstand the change that has been made. I did not just > remove an option that controls a pref; I removed the option AND the pref. > People who changed this value via about:preferences or about:config will > retain the value, but it will do nothing. What's the answer to comment 43? Is there a build-time option to disable app update as mentioned in comment 47?
Assignee | ||
Comment 124•6 years ago
|
||
Yes. Application update can be disabled at build time with the `--disable-updater` flag.
Updated•6 years ago
|
Comment 125•6 years ago
|
||
Firefox 63.0 just automatically updated to 63.0.1 with 'Check for updates but let you choose to install them' checkbox checked.
Comment 126•6 years ago
|
||
Not to mention getting no warning about Firefox 63.0 changing my 'Never check for updates' preference.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(robert.strong.bugs)
Comment 127•6 years ago
|
||
What would it take to remove this change (Bug 1420514) from ALL future Firefox versions and restore functionality? I just spent hours tracking down the source of the missing settings, and created an account solely to ask this question because it really is that important. As a personal user of Firefox I need control over when updates occur, with no nagging windows and ideally with an easy-to-set setting. As someone who helps administer the machines of many smart people, people using machines that CANNOT be allowed to close down or restart browsers at random times, this change has removed a simple way to accomplish that need. While setting policies is a partial band-aid, it keeps getting harder and harder to simply and reliably control Firefox as these changes and workarounds pile on top of one another. The "best" reason for this change I've seen so far is that 99.9% of users don't change it. Given that 99.9% of people probably don't need the bottom 50 pixels of the browser page that hardly seems a good reason to remove them. In fact, almost every Firefox user I know (especially those who supported Mozilla all the way from the Phoenix and Firebird origins) switched because they appreciated that the designers did not hide settings and trusted users to change things as desired. While the answer may be outside of what I can personally accomplish, I'll ask my original honest question again in case another person tracks down the same problem I did and can do more: What would it take to remove this change (Bug 1420514) from ALL future Firefox versions?
Comment 128•6 years ago
|
||
(In reply to tomh1097 from comment #127) > and created an account solely to ask this question Sadly in vain because bugzilla is not the place for it. Discussions and advocacy for or against features or changes belong in our forums: https://www.mozilla.org/about/forums/ For this topic probably dev-firefox unless I'm missing a more specific place for UI design.
Comment 129•6 years ago
|
||
> The "best" reason for this change I've seen so far is that 99.9% of users don't change it.
The best reason for this is the reason initially given - it makes it much more difficult to strand people, deliberately or accidentally, on insecure and outdated versions of Firefox. Because this can happen via either user action or malicious software, and has been allowed to stand for as long as it has, this has become a significant issue for the Firefox user population, and we need to address it.
The nature of the internet - in particularly, the nature of risk on the internet - has changed dramatically since the Phoenix and Firebird days when early design decisions like this one were first implemented. Today, we need to balance the freedom of our users to configure Firefox as they see fit with an equal right to participate in the world free from harassment, exploitation and the rest of the carnival of malfeasance that makes up the background noise of the modern internet.
If you need this sort of granular control over your update situation - control that Chrome doesn't offer at all, if I recall correctly - please consider Firefox ESR, or some of our enterprise policy tools. But this change isn't going to get reverted.
Comment 130•6 years ago
|
||
This change is _really_ annoying. Now I get frequent popups-errors from Firefox that Firefox is unable to check for updates because our firewall do not permit installing non-approved software. Could you please either remove the popup error or put the "do not check at all" feature back on into Firefox? You are arguing that there is a risk when not updating software, but you are forgetting that some people already update software using other, safer methods than through direct unsecure download.
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Comment hidden (advocacy) |
![]() |
||
Updated•6 years ago
|
Flags: needinfo?(robert.strong.bugs)
Comment hidden (advocacy) |
Reporter | ||
Comment 136•6 years ago
|
||
As reporter of the comment #0 (i know my message was a bit short). Mike, in comment #129, explained why we are doing that from the security perspective. There is also another reason why we did it. It is because the web is moving super fast. On some top websites (youtube, facebook, etc), the pace of the web increased significantly. Because the option was super visible and the impact hard to understand for normal folks (ie people who have no clue about bugzilla), we had a bunch of feedback that "website xxx is slow" or "this website doesn't work with firefox" because these users were using an old version of Firefox because, a few years ago, they disabled update. Also, as a release manager, one of the thing which keep me awake at night is antivirus and security software. Because these software don't test with old version of Firefox, they can break (startup crash, network issue, etc) our web browser. And these classes of issues are super hard to diagnose... Now, I hear your complaints. I will ask if we can have some documentation on that.
Comment 137•6 years ago
|
||
important |
You can turn off updates using our policy support: https://github.com/mozilla/policy-templates/blob/master/README.md Create a directory called "distribution" in the same directory as your Firefox executable (Firefox.app/Content/Resources on Mac). Create a file called policies.json in that directory and put the following in it: { "policies": { "DisableAppUpdate": true } } And you won't get asked for updates. If you're on Windows, you can also use GPO to do it.
Reporter | ||
Updated•6 years ago
|
Whiteboard: [ux feature] → [ux feature][To know how to disable updates, please see comment #c137]
Comment 138•6 years ago
|
||
Comment#137 doesn't address IT departments that control updates. But do check out the link as there are a lot of other things you can do with policy. Unfortunately, you can't control disk cache behavior.
Comment 139•6 years ago
|
||
> Comment#137 doesn't address IT departments that control updates. Can you be more specific with what you are asking for? IT departments can turn off updates and then send their own updates via other mechanisms. IT Departments can setup their own update server via policy. IT Departments can use GPO or Mac OS configuration profiles to enable/disable updates. > Unfortunately, you can't control disk cache behavior. It hasn't been requested. Feel free to open an issue there.
Comment 140•6 years ago
|
||
I thought it was obvious, since I am not in that situation, but have no problem imagining IT departments that can't be bothered to do anything with the users ending up paying the price. As well it is hinted at in Comment#130 and explicitly stated in Comment#135(which does not seem to be advocacy).
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Reporter | ||
Comment 143•6 years ago
|
||
Restricting the comments to editbug permissions.
See comment #137 for the option.
Restrict Comments: true
Assignee | ||
Comment 144•6 years ago
|
||
Just a reminder: Bugzilla is not the correct place to advocate for or against features.
Those discussions belong in our forums: https://www.mozilla.org/about/forums/
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Summary: Remove or hide the update features in the preferences → Remove or hide the update features in the preferences - remove autoupdate
Assignee | ||
Comment 145•4 years ago
|
||
If anyone is still paying attention to this bug and is still disappointed by the removal of the "Never Automatically Update" option, there is a new feature that you might be interested in. Bug 1653430 added the Enterprise Policy: ManualAppUpdateOnly
.
This policy causes the following behavior:
- The user will never be prompted to install updates
- Firefox will not check for updates in the background, though it will check automatically when an update UI is displayed (such as the one in the About dialog). This check will be used to show "Update to version X" in the UI, but will not automatically download the update or prompt the user to update in any other way.
- The update UI will work as expected, unlike when using
DisableAppUpdate
This policy is available starting in Firefox 87, which was released today. If you are using Firefox ESR, this will be available starting in Firefox 91, which is scheduled to be released 2021-07-13.
You need to log in
before you can comment on or make changes to this bug.
Description
•