Last Comment Bug 756325 - channel-prefs.js appears in new 'preferences' location on fresh installation but remains in 'pref' for updated existing installation
: channel-prefs.js appears in new 'preferences' location on fresh installation ...
Status: VERIFIED FIXED
[qa+:ashughes]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Robert Strong [:rstrong] (use needinfo to contact me)
:
Mentors:
Depends on: 762588
Blocks: 657789 746156 756298 760792 825870
  Show dependency treegraph
 
Reported: 2012-05-17 17:24 PDT by Dave Hunt (:davehunt)
Modified: 2014-08-08 19:45 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
patch rev1 - move channel-prefs.js back to defaults/pref/ (1.08 KB, patch)
2012-05-29 12:23 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch rev2 - move channel-prefs.js back to defaults/pref/ (3.12 KB, patch)
2012-05-29 14:23 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch rev3 - move channel-prefs.js back to defaults/pref/ (3.17 KB, patch)
2012-05-29 22:28 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
netzen: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Dave Hunt (:davehunt) 2012-05-17 17:24:33 PDT
While investigating bug 756298 for the Mozmill scripts, I noticed that a fresh installation of Nightly had the channel-prefs.js in the new 'preferences' folder, an existing Nightly installation that had been updated to the latest build still had channel-prefs.js in the 'pref' folder.

I suspect this inconsistency could cause problems.
Comment 1 Henrik Skupin (:whimboo) 2012-05-21 03:56:32 PDT
I would think the same. Lets add Anthony and Alex for further input. Also making it a blocking bug 746156.
Comment 2 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-22 09:57:18 PDT
Adding Rob Strong to this bug, whom I hope can answer the concerns.
Comment 3 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-22 12:21:21 PDT
Back to Firefox -> General since this is a Firefox provided preference file which is completely outside of the control of Toolkit -> Application Update.

Looking...
Comment 4 Nick Thomas [:nthomas] 2012-05-22 14:36:17 PDT
Robert, I bet this is from the exclusion of channel-prefs.js from any mar files at generation time. Given that it was quite helpful that the old channel file didn't get added to removed-files.in!
Comment 5 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-22 14:37:33 PDT
Yep... I am just curious why its location was moved.
Comment 6 Nick Thomas [:nthomas] 2012-05-28 14:12:06 PDT
Did Myk have any information on that ? It would be great if we could resolve things before the merge next week.
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-28 22:51:06 PDT
Not sure as to why... just wanted to look over the code that changed to have a better idea as to what had changed.

What I would like to do is remove the requirement for channel-prefs.js while still allowing QA to override the channel... the file has caused problems here and there in the past and I'd like to just get rid of it for the normal use case which is actually fairly simple to do.
Comment 8 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-28 23:02:29 PDT
Nick, do we use the builds from Beta for the Release builds still? If not, I'd like to just preprocess the channel value into nsUpdateService.js and fix up other parts of the code as needed.
Comment 9 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 01:03:08 PDT
I'm proposing the following:
allow specifying a channel via a default pref as before with a new pref name app.update.channel.override
preprocess nsUpdateService.js to include the update channel that was preprocessed into channel-prefs.js
remove the channel-prefs.js files

QA could add a pref file to the app with app.update.channel.override to define the channel to test updates.

The only thing I am unsure of is whether you still need to send updates to beta users with a release channel build? If so, I'd still like to preprocess nsUpdateService.js to include the update channel so if the channel-prefs.js file disappears the user would still have an update channel defined.
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 02:14:44 PDT
For those following along, we can't remove the old channel-prefs.js file because users that have only updated using app update only have the old channel-prefs.js and we can't move back to using only the old channel-prefs.js file and remove the new channel-prefs.js file because users that have installed a version after the move of the channel-prefs.js file will only have the new one. The reason we don't update the channel-prefs.js file has been due to us using the same update mar (Mozilla Archive file used to update Mozilla applications) files on release as we do on beta (at least in the past and I *think* this may still be the case).

Nick, another thing I could do is add a new version of the update manifest that supports a if file not present add instruction and change the mar generation scripts to specify the channel-prefs.js file for this instruction. Regretfully, that would only help installs that have an updater that already has this support so we wouldn't be able to remove the old channel-prefs.js file without likely having to hardcode a remove instruction for the old channel-prefs.js file in the new version of the manifest (meh).

cc'ing Myk

Myk, is there a reason the defaults/pref/ dir can't be used for the channel-prefs.js file? I see in bug 746156 #comment 3 that this needed to be changed for firefox-l10n.js but it was also changed for channel-prefs.js.

In summary, if we need to keep the channel after an update using a release mar file on the beta channel this is going to be very difficult to fix completely. If we can move the channel-prefs.js back to defaults/prefs/ we can limit the bug to just nightly users and clean that up over time in part by adding an "if file not present" instruction to the updater.
Comment 11 Myk Melez [:myk] [@mykmelez] 2012-05-29 10:36:27 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #10)
> Myk, is there a reason the defaults/pref/ dir can't be used for the
> channel-prefs.js file? I see in bug 746156 #comment 3 that this needed to be
> changed for firefox-l10n.js but it was also changed for channel-prefs.js.

I changed the location of all the Firefox default pref files, from the GRE location to the app-specific location, so the webapp runtime wouldn't inherit Firefox's preferences.  But channel-prefs.js is harmless, containing only a single pref that the runtime won't use anyway.  So we can move it back to defaults/pref/.
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 11:10:48 PDT
Drivers, this bug puts us in an inconsistent state as to where the channel-prefs.js file is located depending on whether the install was updated from a previous build or it was installed after the change to the file's location. See comment #10 for more details.

I *think* we need to have this fixed or at the very least mitigated so it only affects the nightly users that manually installed before the next merge.
Comment 13 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 12:23:00 PDT
Created attachment 628055 [details] [diff] [review]
patch rev1 - move channel-prefs.js back to defaults/pref/

The path I think we should take to resolve this:

1. move the channel-prefs.js file back to defaults/pref/. This will lessen the number of users affected by this bug and keep it from affecting Aurora, Beta, and Release.

2. File a new update bug for the ability to add files that don't exist. This will add the channel-prefs.js file to the defaults/pref/ directory for installations that happened after channel-prefs.js was moved.

3. Evaluate the need to remove the extra channel-prefs.js file from the installations that happened after channel-prefs.js was moved... my current thinking is it isn't important to remove it for these installations.

Chime in if you have concerns!
Comment 14 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 12:28:47 PDT
Filed bug 759460 for preprocessing the channel name into nsUpdateService.js to mitigate issues where the channel-prefs.js file disappears for whatever reason.
Comment 15 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 12:54:28 PDT
Filed bug 759469 for the new updater instruction mentioned in comment #13 item 2.
Comment 16 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 12:56:20 PDT
Comment on attachment 628055 [details] [diff] [review]
patch rev1 - move channel-prefs.js back to defaults/pref/

I need to double check a couple of things with packager.mk before this is reviewed. :(
Comment 17 Brian R. Bondy [:bbondy] 2012-05-29 13:09:51 PDT
Comment on attachment 628055 [details] [diff] [review]
patch rev1 - move channel-prefs.js back to defaults/pref/

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

Do we need to add @BINPATH@/@PREF_DIR@/channel-prefs.js to removed-files.in?
Comment 18 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 13:12:01 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #17)
> Comment on attachment 628055 [details] [diff] [review]
> patch rev1 - move channel-prefs.js back to defaults/pref/
> 
> Review of attachment 628055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we need to add @BINPATH@/@PREF_DIR@/channel-prefs.js to removed-files.in?
We can't without first guaranteeing that the one in defaults/pref/ is present and we have no clean way of adding the one to defaults/pref/ at present (bug 759469 will make that better though).
Comment 19 Brian R. Bondy [:bbondy] 2012-05-29 13:18:04 PDT
> We can't without first guaranteeing that the one in defaults/pref/ is present and 
> we have no clean way of adding the one to defaults/pref/ at present

I thought it wouldn't be critical since we'd fall back to the current channel name as per Bug 759460. 

Wouldn't it be possible for a user to have both pref files? Let's say they install on top of an existing installation?  (Although I'm sure this is not common)
Comment 20 Brian R. Bondy [:bbondy] 2012-05-29 13:18:28 PDT
Also if releng does a channel change special case then a user might end up with 2 pref files.
Comment 21 Brian R. Bondy [:bbondy] 2012-05-29 13:29:14 PDT
Comment on attachment 628055 [details] [diff] [review]
patch rev1 - move channel-prefs.js back to defaults/pref/

Clearing for now, please re r? me
Comment 22 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 13:29:57 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #19)
> > We can't without first guaranteeing that the one in defaults/pref/ is present and 
> > we have no clean way of adding the one to defaults/pref/ at present
> 
> I thought it wouldn't be critical since we'd fall back to the current
> channel name as per Bug 759460. 
That will make it better as well but I want to have bug 759469 as well which will make it even better for this instance.

> Wouldn't it be possible for a user to have both pref files? Let's say they
> install on top of an existing installation?  (Although I'm sure this is not
> common)
It isn't common and it isn't supported except on Windows where we do remove the files using the uninstall.log before installing.

(In reply to Brian R. Bondy [:bbondy] from comment #20)
> Also if releng does a channel change special case then a user might end up
> with 2 pref files.
Yes and because of this bug we won't be doing that until both of the above bugs are fixed and we remove the extra chanel-prefs.js file.
Comment 23 Brian R. Bondy [:bbondy] 2012-05-29 13:43:07 PDT
> Yes and because of this bug we won't be doing that until both of the above 
> bugs are fixed and we remove the extra chanel-prefs.js file.

OK, I added a dependency for the planned channel change that I know about.
Comment 24 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 13:46:33 PDT
It depends on whether those locales have been getting updates and would be affected by this bug and I do think that releng can just create a custom one-time mar (which they will have to do anyways) to handle this for those locales. As long as *this* bug where the location of the channel-prefs.js file has changed doesn't make it onto aurora, etc. and those locales haven't been updating then this has nothing to do with them.
Comment 25 Brian R. Bondy [:bbondy] 2012-05-29 14:04:02 PDT
> It depends on whether those locales have been getting updates and 
> would be affected by this bug

I'm not sure how often Nightly builds happen for those locale builds, but I think it's safest to mark it so they are aware they need to take this into consideration when generating the MAR file if those locales are affected.

> As long as *this* bug where the location of the channel-prefs.js file 
> has changed doesn't make it onto aurora, etc. and those locales haven't 
> been updating then this has nothing to do with them.

That bug is for migrating from m-c to aurora, so I think if those builds have been getting updates, then they are already affected by this.
Comment 26 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 14:10:44 PDT
Already understood on all counts with the exception that we don't know whether these locales that are going to have their users moved from m-c are updating on m-c and I tend not to adding dependencies without first knowing whether it is actually a dependency.
Comment 27 Brian R. Bondy [:bbondy] 2012-05-29 14:21:53 PDT
> I tend not to adding dependencies without first knowing whether it 
> is actually a dependency.

Removed the dependencies and I asked in the bug if they have been getting updates. Although the answer may change before these land:

> ...we won't be doing that [channel change] until both of the above 
> bugs are fixed and we remove the extra chanel-prefs.js file.

I had marked it that way because it would be easier for that bug, when it was ready to land, to ensure that the blocking bugs are OK to be ignored and land anyway.
Comment 28 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 14:23:09 PDT
Created attachment 628109 [details] [diff] [review]
patch rev2 - move channel-prefs.js back to defaults/pref/

That was fun... so, I had to one-off the handling of Firefox's channel-prefs.js in both the browser/app Makefile and packager.mk.
Comment 29 Nick Thomas [:nthomas] 2012-05-29 14:48:27 PDT
Responding to the earlier queries about beta & release and preprocessing, we are currently not pushing release mars to the beta channel (eg just prior to a release coming out). I know there was effort to maintain the possibility of that with the signed mar changes, so dropping it now would be a wider discussion. There have also been proposals to take nightly users to and give them new-feature builds for short periods, and it would be useful to preserve the original update channel (via channel-prefs.js) to return them afterwards.

Having said that, the current plan to revert the defaults/preferences/ change sounds good to me.
Comment 30 Brian R. Bondy [:bbondy] 2012-05-29 15:32:37 PDT
It was confirmed that most of those locales did get updates by the way, and so they are affected by this problem.  I'll leave the dependencies to your discretion now that we have more certainty that the channel change will be affected and can cause 2 pref files with different channels.
Comment 31 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 15:37:13 PDT
Will do... I *think* I have a decent way to fix this for that case as well and will likely fix it in bug 759469.
Comment 32 Brian R. Bondy [:bbondy] 2012-05-29 19:07:51 PDT
Comment on attachment 628109 [details] [diff] [review]
patch rev2 - move channel-prefs.js back to defaults/pref/

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

I installed Aurora and noticed it is also using the new defaults/preferences directory and not defaults/pref

::: browser/app/Makefile.in
@@ +138,5 @@
>  	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin/defaults/profile
>  
> +# channel-prefs.js is handled separate from other prefs due to bug 756325
> +libs:: $(srcdir)/profile/channel-prefs.js
> +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(PREF_PPFLAGS) $(ACDEFINES) $^ > $(DIST)/bin/defaults/pref/channel-prefs.js

$(DIST)/bin/defaults/pref must exist before running this script, tried it and build fails for that reason.
Comment 33 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-29 22:28:03 PDT
Created attachment 628228 [details] [diff] [review]
patch rev3 - move channel-prefs.js back to defaults/pref/

Thanks for catching that... I've had that happen to me before and should've known better. :)

This will precreate the dir if it doesn't exist

Also, thanks for checking on Aurora... I'll try to get this over there as well.
Comment 34 Henrik Skupin (:whimboo) 2012-05-30 01:29:01 PDT
Once this bug has been fixed we can revert the change we made over on bug 756298 for our Mozmill update tests.
Comment 35 Brian R. Bondy [:bbondy] 2012-05-30 06:47:35 PDT
Comment on attachment 628228 [details] [diff] [review]
patch rev3 - move channel-prefs.js back to defaults/pref/

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

Looks good, also tried it and it installs correctly.
Comment 36 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-30 15:18:59 PDT
Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8a0cf03399
Comment 37 Ed Morley [:emorley] 2012-05-31 05:57:54 PDT
https://hg.mozilla.org/mozilla-central/rev/4f8a0cf03399
Comment 38 Robert Strong [:rstrong] (use needinfo to contact me) 2012-05-31 09:43:20 PDT
Comment on attachment 628228 [details] [diff] [review]
patch rev3 - move channel-prefs.js back to defaults/pref/

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 746156
User impact if declined: Inconsistent file locations on Beta and Release users. Releng will need to do additional one-off work to create updates that move users to other channels.
Testing completed (on m-c, etc.): Visual inspection of files in the build.
Risk to taking this patch (and alternatives if risky): Minimal and it is mitigated by Bug 759460 which I will request approval for shortly.
String or UUID changes made by this patch: None
Comment 39 Robert Strong [:rstrong] (use needinfo to contact me) 2012-06-01 12:24:11 PDT
To verify this, download a nightly build with the fix and verify that defaults/preferences/channel-prefs.js does not exist and that defaults/pref/channel-prefs.js exists.
Comment 40 Alex Keybl [:akeybl] 2012-06-01 15:06:39 PDT
Comment on attachment 628228 [details] [diff] [review]
patch rev3 - move channel-prefs.js back to defaults/pref/

[Triage Comment]
Approved for Aurora 14, given the alternatives and low risk evaluation.
Comment 41 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-01 17:02:02 PDT
Verified fixed in Firefox 15.0a1 2012-06-01.
Comment 42 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-03 12:00:40 PDT
[Triage Comment]
Reminder that merge day is tomorrow afternoon PT so please land this to Aurora (14) before then.
Comment 43 Robert Strong [:rstrong] (use needinfo to contact me) 2012-06-03 22:24:34 PDT
Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b1e65dae1d5
Comment 44 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-04 16:10:38 PDT
Verified fixed in Firefox 14.0a2 2012-06-04.

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