Last Comment Bug 763232 - Improper display of preference Nightly updates.
: Improper display of preference Nightly updates.
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: 16 Branch
: All Linux
: -- normal (vote)
: Firefox 17
Assigned To: Matthew N. [:MattN] (PTO Jun. 29-30)
:
Mentors:
Depends on:
Blocks: 317190
  Show dependency treegraph
 
Reported: 2012-06-09 14:41 PDT by Iceberg
Modified: 2012-08-09 07:42 PDT (History)
12 users (show)
MattN+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed


Attachments
v.1 Catch exception getting shell service and disable/hide usage of it when unavailable (6.60 KB, patch)
2012-07-24 15:53 PDT, Matthew N. [:MattN] (PTO Jun. 29-30)
jaws: feedback+
Details | Diff | Review
v.2 Address review comments (12.30 KB, patch)
2012-07-26 21:55 PDT, Matthew N. [:MattN] (PTO Jun. 29-30)
jaws: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Iceberg 2012-06-09 14:41:12 PDT
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.
Comment 1 Iceberg 2012-06-10 13:29:44 PDT
I add that this bug affects Firefox 16, but also Firefox 15.
Comment 2 Virgil Dicu [:virgil] [QA] 2012-06-11 07:39:04 PDT
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?
Comment 3 Virgil Dicu [:virgil] [QA] 2012-06-11 07:40:08 PDT
(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
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-11 09:43:09 PDT
Robert or Dave, do you have any thoughts on this bug?
Comment 5 Robert Strong [:rstrong] (use needinfo to contact me) 2012-06-11 10:54:16 PDT
(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
Comment 6 Matthew N. [:MattN] (PTO Jun. 29-30) 2012-06-11 11:13:54 PDT
(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)?
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2012-06-11 11:16:59 PDT
(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.
Comment 8 Iceberg 2012-06-11 12:28:15 PDT
(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
Comment 9 Virgil Dicu [:virgil] [QA] 2012-07-20 08:28:08 PDT
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?
Comment 10 Matthew N. [:MattN] (PTO Jun. 29-30) 2012-07-20 16:01:25 PDT
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.
Comment 11 Christian Ascheberg 2012-07-23 10:44:06 PDT
(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.
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-23 14:11:42 PDT
Tracking for 15, but also 16 since we'll want to have clear tracking that this gets resolved across all branches.
Comment 13 Matthew N. [:MattN] (PTO Jun. 29-30) 2012-07-24 15:53:58 PDT
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
Comment 14 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-24 17:47:05 PDT
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.
Comment 15 Matthew N. [:MattN] (PTO Jun. 29-30) 2012-07-26 21:55:46 PDT
Created attachment 646469 [details] [diff] [review]
v.2 Address review comments
Comment 16 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-26 22:26:04 PDT
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 :)
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-31 23:40:40 PDT
Matt, are you going to land this and request aurora/beta approval?
Comment 18 Matthew N. [:MattN] (PTO Jun. 29-30) 2012-08-01 01:48:28 PDT
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
Comment 19 Matthew N. [:MattN] (PTO Jun. 29-30) 2012-08-01 01:52:43 PDT
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
Comment 20 Ed Morley [:emorley] 2012-08-01 10:52:55 PDT
https://hg.mozilla.org/mozilla-central/rev/59d1993da7e1
Comment 22 Ioana (away) 2012-08-09 07:42:24 PDT
Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0 (20120808131812)

Note You need to log in before you can comment on or make changes to this bug.