Closed Bug 756325 Opened 12 years ago Closed 12 years ago

channel-prefs.js appears in new 'preferences' location on fresh installation but remains in 'pref' for updated existing installation

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 14
Tracking Flags:
Tracking Status
firefox14 + verified
firefox15 + verified

People

(Reporter: davehunt, Assigned: robert.strong.bugs)

References

Details

(Whiteboard: [qa+:ashughes])

Attachments

(1 file, 2 obsolete files)

patch rev3 - move channel-prefs.js back to defaults/pref/
3.17 KB, patch
bbondy
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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.
I would think the same. Lets add Anthony and Alex for further input. Also making it a blocking bug 746156.
Blocks: 746156
Component: Preferences → General
OS: Mac OS X → All
QA Contact: preferences → general
Hardware: x86 → All
Adding Rob Strong to this bug, whom I hope can answer the concerns.
Component: General → Application Update
Product: Firefox → Toolkit
QA Contact: general → application.update
Back to Firefox -> General since this is a Firefox provided preference file which is completely outside of the control of Toolkit -> Application Update.

Looking...
Component: Application Update → General
Product: Toolkit → Firefox
QA Contact: application.update → general
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!
Yep... I am just curious why its location was moved.
Did Myk have any information on that ? It would be great if we could resolve things before the merge next week.
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.
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.
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.
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.
(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/.
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.
status-firefox15: --- → affected
tracking-firefox15: --- → ?
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!
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #628055 - Flags: review?(netzen)
Filed bug 759460 for preprocessing the channel name into nsUpdateService.js to mitigate issues where the channel-prefs.js file disappears for whatever reason.
Filed bug 759469 for the new updater instruction mentioned in comment #13 item 2.
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. :(
Attachment #628055 - Flags: review?(netzen)
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?
Attachment #628055 - Flags: review+
(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).
> 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)
Also if releng does a channel change special case then a user might end up with 2 pref files.
Comment on attachment 628055 [details] [diff] [review]
patch rev1 - move channel-prefs.js back to defaults/pref/

Clearing for now, please re r? me
Attachment #628055 - Flags: review+
(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.
Blocks: 657789
> 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.
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.
> 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.
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.
No longer blocks: 657789
> 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.
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.
Attachment #628055 - Attachment is obsolete: true
Attachment #628109 - Flags: review?(netzen)
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.
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.
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 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.
Attachment #628109 - Flags: review?(netzen)
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.
Attachment #628109 - Attachment is obsolete: true
Attachment #628228 - Flags: review?(netzen)
Once this bug has been fixed we can revert the change we made over on bug 756298 for our Mozmill update tests.
Blocks: 756298
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.
Attachment #628228 - Flags: review?(netzen) → review+
https://hg.mozilla.org/mozilla-central/rev/4f8a0cf03399
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
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
Attachment #628228 - Flags: approval-mozilla-aurora?
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 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.
Attachment #628228 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed in Firefox 15.0a1 2012-06-01.
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
Whiteboard: [qa+:ashughes]
[Triage Comment]
Reminder that merge day is tomorrow afternoon PT so please land this to Aurora (14) before then.
Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b1e65dae1d5
status-firefox14: affected → fixed
Target Milestone: Firefox 15 → Firefox 14
Verified fixed in Firefox 14.0a2 2012-06-04.
status-firefox14: fixed → verified
Depends on: 762588
Blocks: 825870
See Also: → 1541601
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: