Improper display of preference Nightly updates.

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Preferences
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Iceberg, Assigned: MattN)

Tracking

({regression})

16 Branch
Firefox 17
All
Linux
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15+ verified, firefox16+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Firefox Nightly preferences -> Advanced -> Updates: Nightly updates.

Steps to reproduce:

Select the second (Check for updates) or third (Never check for updates) option.
Close the preferences window.
Reopen the preferences window.

Actual results:
"Automatically install updates" always it appears as selected.

Expected results:
"Check for updates" it must appears selected.
or
"Never check for updates" it must appears selected.

Reproducible: always.

Firefox Nightly of May 17 2012 work correctly.
The later versions have this problem.

Important: if I selected "Never check for updates" (or Check for updates) appears selected "Automatically install updates" but Firefox Nightly does not automatically install updates.
The preference chosen by the user is displayed incorrectly but respected.
(Reporter)

Comment 1

5 years ago
I add that this bug affects Firefox 16, but also Firefox 15.
Thanks for the report. This seems pretty bad, actually.

Only occurs on Linux, does not occur on Mac or Windows.

Can't change the default update settings in Preferences. The default option always remains selected after updating

Last good nightly: 2012-05-17
First bad nightly: 2012-05-18

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=762e95608da3&tochange=e794cef56df6

Suspected: bug 706292?
(In reply to Virgil Dicu [:virgil] [QA] from comment #2)
> Can't change the default update settings in Preferences. The default option
> always remains selected after updating

*after closing the Preferences dialog
Status: UNCONFIRMED → NEW
Ever confirmed: true
Robert or Dave, do you have any thoughts on this bug?
Keywords: regression
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #4)
> Robert or Dave, do you have any thoughts on this bug?
Adding MattN and Unfocused since they were the ones that worked on bug 706292
(In reply to Robert Strong [:rstrong] (do not email) from comment #5)
> (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #4)
> > Robert or Dave, do you have any thoughts on this bug?
> Adding MattN and Unfocused since they were the ones that worked on bug 706292

Bug 706292 was only for the add-ons manager which shouldn't affect application update.

Iceberg, are you using the new in-content preferences (browser.preferences.inContent = true)?
(In reply to Matthew N. [:MattN] from comment #6)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #5)
> > (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #4)
> > > Robert or Dave, do you have any thoughts on this bug?
> > Adding MattN and Unfocused since they were the ones that worked on bug 706292
> 
> Bug 706292 was only for the add-ons manager which shouldn't affect
> application update.
Just looked and didn't notice any changes to the Firefox preferences code but it is important to note that this is a bug in the Firefox preferences pane and that both the add-ons manager and application update preferences are on the same preferences pane.
(Reporter)

Comment 8

5 years ago
(In reply to Matthew N. [:MattN] from comment #6)

> Iceberg, are you using the new in-content preferences
> (browser.preferences.inContent = true)?

This bug occurs in both cases.

browser.preferences.inContent = false
and
browser.preferences.inContent = true
This is still an issue on Linux - Firefox 15 is affected. Testing shows that the selected option from Updates is applied, but not remembered in Preferences.

Using hg bisect:

The first bad revision is:
changeset:   94267:e794cef56df6
parent:      94222:e34babb30393
parent:      94266:c70b34f9477d
user:        Ryan VanderMeulen <ryanvm@gmail.com>
date:        Thu May 17 23:25:38 2012 -0400
summary:     Merge last PGO-green inbound changeset to m-c.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e34babb30393&tochange=e794cef56df6

This rules out bug 706292. I can bisect more on inbound next week, but bug 317190 looks the most likely to trigger this. Christian and jaws, do you think this is possible?
Requesting tracking-firefox-15 since this seems to be caused by bug 317190 and users who disabled automatic updates won't have a way to re-enable them (and vice versa).  The problem is that getService(Components.interfaces.nsIShellService) throws on Linux and bug 317190 now calls that during init of the advanced pane.  Workaround patch coming soon.
Assignee: nobody → mnoorenberghe+bmo
Blocks: 317190
Status: NEW → ASSIGNED
tracking-firefox15: --- → ?
Hardware: x86 → All

Comment 11

5 years ago
(In reply to Matthew N. [:MattN] from comment #10)
> The problem is that getService(Components.interfaces.nsIShellService) throws on Linux

Looks like bug 297841 was once filed for that.

I can reproduce this bug on Ubuntu using Nightly. The version of Firefox that comes with Ubuntu does not show any errors when clicking the "Check now" button though.
Tracking for 15, but also 16 since we'll want to have clear tracking that this gets resolved across all branches.
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox15: ? → +
tracking-firefox16: --- → +
Created attachment 645559 [details] [diff] [review]
v.1 Catch exception getting shell service and disable/hide usage of it when unavailable

The helper catches the exception and when no shell service is returned I do the following since they don't work anyways: 
* hide the button to set the browser as the default
* disable the checkbox to check if the browser is the default on every startup
Attachment #645559 - Flags: review?(jaws)
Comment on attachment 645559 [details] [diff] [review]
v.1 Catch exception getting shell service and disable/hide usage of it when unavailable

Review of attachment 645559 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for tests! Can you also duplicate this test for preferences/in-content?

::: browser/components/preferences/advanced.js
@@ +700,5 @@
>     */
>    updateSetDefaultBrowser: function()
>    {
> +    let shellSvc = getShellService();
> +    let pane = document.getElementById("setDefaultPane");

s/pane/setDefaultPane

@@ +704,5 @@
> +    let pane = document.getElementById("setDefaultPane");
> +    if (!shellSvc) {
> +      // Hide the set default browser button and disable the checkbox to check
> +      // the default at startup.
> +      pane.hidden = true;

With the rename of the variable, I think we can remove this comment.

::: browser/components/preferences/tests/browser_advanced_update.js
@@ +3,5 @@
> +  resetPreferences();
> +
> +  function observer(win, topic, data) {
> +    if (topic != "advanced-pane-loaded")
> +      return;

This observer isn't being used for any other topics so we shouldn't need this check here.

@@ +20,5 @@
> +  let enableSearchUpdate = doc.getElementById("enableSearchUpdate");
> +
> +  // Ensure that the update pref dialog reflects the actual pref value.
> +  Services.prefs.setBoolPref("browser.search.update", false);
> +  ok(!enableSearchUpdate.checked, "Ensure search updates are disabled");

Is there enough time between the setting of the pref and the calling of updateSetDefaultBrowser() for this to work? I think you should also add a test case for the other state as well.

@@ +31,5 @@
> +  finish();
> +}
> +
> +function resetPreferences() {
> +  Services.prefs.clearUserPref("browser.search.update");

Please use registerCleanupFunction(function() { ... }) to guarantee that the pref is cleared.
Attachment #645559 - Flags: review?(jaws) → feedback+
Created attachment 646469 [details] [diff] [review]
v.2 Address review comments
Attachment #645559 - Attachment is obsolete: true
Attachment #646469 - Flags: review?(jaws)
Comment on attachment 646469 [details] [diff] [review]
v.2 Address review comments

Review of attachment 646469 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following changes. see the privacy tests for an example of how the in-content tests change panes.

::: browser/components/preferences/in-content/tests/browser_advanced_update.js
@@ +13,5 @@
> +    Services.obs.removeObserver(observer, "advanced-pane-loaded");
> +    runTest(win);
> +  }
> +  Services.obs.addObserver(observer, "advanced-pane-loaded", false);
> +*/

as talked about on IRC, don't forget to remove this commented out code.

@@ +16,5 @@
> +  Services.obs.addObserver(observer, "advanced-pane-loaded", false);
> +*/
> +  Services.prefs.setBoolPref("browser.search.update", false);
> +
> +  open_preferences(runTest);

this test (the in-content one) should probably switch to the advanced pane before starting the test though.

::: browser/components/preferences/in-content/tests/head.js
@@ +24,5 @@
>    isnot(aElement, null, "Element should not be null, when checking visibility");
>    ok(is_hidden(aElement), aMsg);
> +}
> +
> +function open_preferences(aCallback) {

thanks for removing this duplication :)
Attachment #646469 - Flags: review?(jaws) → review+
Matt, are you going to land this and request aurora/beta approval?
It was near the top of my to-do list but other high priority stuff has been keeping me busy.

https://hg.mozilla.org/integration/mozilla-inbound/rev/59d1993da7e1

Try results: https://tbpl.mozilla.org/?tree=Try&rev=2e8b66dfc3da
Flags: in-testsuite+
Comment on attachment 646469 [details] [diff] [review]
v.2 Address review comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 317190
User impact if declined: The user's app update pref UI won't reflect their current value.
Testing completed (on m-c, etc.): Tested on my Fedora build. Just landed on m-i.
Risk to taking this patch (and alternatives if risky): Low risk switch to using a helper function to get the app shell service like other code already uses. Possible risk that this patch also throws an exception and we're back to where we started.
String or UUID changes made by this patch:  None
Attachment #646469 - Flags: approval-mozilla-beta?
Attachment #646469 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/59d1993da7e1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Attachment #646469 - Flags: approval-mozilla-beta?
Attachment #646469 - Flags: approval-mozilla-beta+
Attachment #646469 - Flags: approval-mozilla-aurora?
Attachment #646469 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/573d539ae6d5
https://hg.mozilla.org/releases/mozilla-beta/rev/82fa443157c9
status-firefox15: affected → fixed
status-firefox16: affected → fixed

Comment 22

5 years ago
Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0 (20120808131812)
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.