Closed Bug 842334 Opened 11 years ago Closed 11 years ago

Ensure distribution directory is still at the top level after the move of the browser into the browser directory

Categories

(Core :: Preferences: Backend, defect)

Product:
Core
Component:
Preferences: Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24
Tracking Flags:
Tracking Status
firefox21 + fixed
firefox22 + fixed
firefox23 + fixed
firefox24 + fixed

People

(Reporter: mkaply, Assigned: glandium)

References

Details

Attachments

(3 files, 2 obsolete files)

Move distribution/ back to the installation directory
2.32 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Original patch + test failure fix
2.90 KB, patch
mkaply
: review+
bajaj
: approval-mozilla-release+
Details | Diff | Splinter Review
Fixup test_browserGlue_distribution.js and use XREExeF in nsXREDirProvider
3.05 KB, patch
Gavin
: review+
philor
: checkin+
Details | Diff | Splinter Review 
Bug 755724 split app and platform resources.

We need to ensure that the distribution directory still works properly at the top level (it should not be in the browser directory).
Blocks: metro-build
Assignee: nobody → mh+mozilla
Comment on attachment 715505 [details] [diff] [review]
Move distribution/ back to the installation directory

Mike, can you check this does the right thing for you?

Now the question is whether this makes sense or not. Do we want the distribution directory to be shared between browser/ and metro/ or do we want separate ones? The same question actually holds for bug 841011.
Attachment #715505 - Flags: feedback?(mozilla)
Attachment #715505 - Flags: feedback?(jmathies)
Kev Needs to be in this discussion since his team owns creating distributions for third parties.
Any chance you could create a try build?
Let me do some reading. I don't know what the distributions thing actually does or whether the metro browser would use a resource from it.
(In reply to Jim Mathies [:jimm] from comment #5)
> Let me do some reading. I don't know what the distributions thing actually
> does or whether the metro browser would use a resource from it.

Does this - 

https://wiki.mozilla.org/Distribution_INI_File

cover everything that can be customized?
(In reply to Jim Mathies [:jimm] from comment #6)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > Let me do some reading. I don't know what the distributions thing actually
> > does or whether the metro browser would use a resource from it.
> 
> Does this - 
> 
> https://wiki.mozilla.org/Distribution_INI_File
> 
> cover everything that can be customized?

Ah, this includes packaging extensions. So no, I do not think we want to share a distribution folder. If firefox desktop needs to rely on a distribution folder in the gre for backward compat we'll have to figure out a way to turn that off in the metro app. (which I can help do if need be.)
Sorry for spamming the bug but I just wanted to ask, why not move distributions to browser/distributions? Seems like that would be the right way to do this going forward, even if it causes some consumers of this feature migration pain.
Attachment #715505 - Flags: feedback?(jmathies) → feedback-
(In reply to Jim Mathies [:jimm] from comment #8)
> Sorry for spamming the bug but I just wanted to ask, why not move
> distributions to browser/distributions? Seems like that would be the right
> way to do this going forward, even if it causes some consumers of this
> feature migration pain.

That's actually where it is now, but the question is whether it makes sense to move it back or not. I don't know what the addons manager does for addons under distribution/extensions, but if it looks at the em:targetApplication entries in install.rdf (and i guess it does), there should be no problem having the extensions in a shared location. It would actually make sense to have them in a shared location.

Note that "parent of XREExeF" is slightly different from "gre directory" (not on mozilla.org builds, but on FF-on-XR builds, it is).
Ok, so sounds like loading non-metro compliant extensions will be avoided. I suppose we won't be filtering things prefs and bookmarks? That probably isn't a major deal breaker although could cause some problems for consumers down the road. I suppose we could try to address that when it comes up.
Is the metro separation going to be there on all platforms?

Or would the distribution directory be in different places on different platforms?

Are we really going through all this pain just for Win Metro?
(In reply to Jim Mathies [:jimm] from comment #10)
> Ok, so sounds like loading non-metro compliant extensions will be avoided.

Note it's only a supposition, assuming addons from there are installed through the same process as downloaded addons.

(In reply to Michael Kaply (mkaply) from comment #11)
> Is the metro separation going to be there on all platforms?

The move of browser-specific files into the browser/ subdirectory is there on all desktop platforms.
(In reply to Michael Kaply (mkaply) from comment #11)

> Are we really going through all this pain just for Win Metro?

metro was the genesis for getting app and gre separated. Overall this is a good thing for platform long term. But yes, we are going through all this today just for Win Metro.
(In reply to Mike Hommey [:glandium] from comment #12)
> (In reply to Jim Mathies [:jimm] from comment #10)
> > Ok, so sounds like loading non-metro compliant extensions will be avoided.
> 
> Note it's only a supposition, assuming addons from there are installed
> through the same process as downloaded addons.

Addons can't load in metro currently (pref'd off), so when we get to implementing that we can test to be sure it's working correctly. I'll do some quick testing today of a distribution to confirm it's not a problem.
(In reply to Michael Kaply (mkaply) from comment #4)
> Any chance you could create a try build?

https://tbpl.mozilla.org/?tree=Try&rev=9654280eee4d

should be up in a couple hours.
I think we're definitely going to need Kev's input on this.

If we do decide to put the distribution directory in browser, has any thought been given as to how upgrades will work?

If an installed Firefox distribution (Bing, Yahoo, etc.) has files in distribution, will they get uninstalled and then moved to the new distribution directory?
Attachment #715505 - Attachment is obsolete: true
Attachment #715505 - Flags: feedback?(mozilla)
Comment on attachment 715585 [details] [diff] [review]
Move distribution/ back to the installation directory

Mike, can you test this build?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mh@glandium.org-aedbd5e38e2b/try-win32/
Attachment #715585 - Flags: feedback?(mozilla)
Comment on attachment 715585 [details] [diff] [review]
Move distribution/ back to the installation directory

I think this is the right move. As Mike mentions in comment 16, leaving it under browser means it moves compared to where it was before and we don't handle that.

Mike, I'm still interested in knowing if the try build works for you.
Attachment #715585 - Flags: review?(gavin.sharp)
Attachment #715585 - Flags: review?(gavin.sharp) → review?(benjamin)
Comment on attachment 715585 [details] [diff] [review]
Move distribution/ back to the installation directory

This appears to affect the following aspects of distribution/:

* search plugins
* bookmarks
* prefs

Prefs is iffy, because prefs may be specific to the metro or desktop half, but for now I think it's ok.

It does not affect:

* extensions
* bundles

This is also correct, because these will be different in the metro and desktop halves, or we may only support restartless or jetpack style extensions in the metro side.

It may be worth commenting on this difference at http://hg.mozilla.org/mozilla-central/annotate/e7632ab657e5/toolkit/xre/nsXREDirProvider.cpp#l375 though to avoid future confusion.
Attachment #715585 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> It does not affect:
> 
> * extensions
> * bundles
> 
> This is also correct, because these will be different in the metro and
> desktop halves, or we may only support restartless or jetpack style
> extensions in the metro side.

Mmmm I did mean to change this, but failed to. At least for extensions. I'm not sure how bundles are handled. For extensions, the appid for metro and browser are different, so i don't actually expect problems from the directory being shared, but if the addons manager doesn't handle the situation nicely, then we can fix that in a followup. AIUI metro doesn't enable the addons manager at the moment, so it shouldn't be an immediate problem.
Flags: needinfo?(benjamin)
bundles are not checked against app IDs and must not live in the toplevel distribution directory.
Flags: needinfo?(benjamin)
Do metro and browser share preferences?

Are there going to be issues with sharing bookmarks and preferences in the distribution.ini files?

What about searchplugins?

The distribution directory has a lot of moving parts...
(In reply to Michael Kaply (mkaply) from comment #23)
> Do metro and browser share preferences?

They don't share prefs. They are separate browsers.

here's metro base:

http://mxr.mozilla.org/mozilla-central/source/browser/metro/

See 'profile' for prefs.

> Are there going to be issues with sharing bookmarks and preferences in the
> distribution.ini files?

Depends on what prefs they set. But yes I think there's a chance someone might turn something on or off in desktop and potentially break something in metrofx. Maybe metrofx should just ignore distribution.ini prefs? 

For bookmarks, metro doesn't display conventional desktop bookmarks. So while these bookmarks may get included in awesome screen searches, they won't show up in the ui. (Based on what I see in https://wiki.mozilla.org/Distribution_INI_File) 

> What about searchplugins?

The two browser share the same searchplugins, so this shouldn't be an issue.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/import/Makefile.in#27
Based on everything you're saying here, I'm starting to believe that the distribution directory does belong in the browser directory.

It sounds like most of the things that are done via distribution won't have an affect on metro anyway, so having it at the top level will be confusing.

I'm still waiting for someone from Kev Needham's team to get involved in this discussion, since the distribution directory is there baby. I've emailed him.
I took a little survey on the enterprise list, and I can't find a single person that's going to do anything around metro.

Based on that, and based on the fact that the distribution directory is really only about the main browser, I think it should be in the browser directory.

But what it is probably going to boil down to is if partner browser (like ebay/yahoo/etc.) are going to want to customize the metro version of Firefox (or if they even can).

I emailed Kev to get input.
Let's call this a wontfix then.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Attachment #715585 - Flags: feedback?(mozilla)
(In reply to Michael Kaply (mkaply) from comment #26) 
> I emailed Kev to get input.

How can release engineering find out about these changes earlier in the future?

Tomcat is rolling a patch in bug 865616 now to update our partner repack scripts to deal with the change, but if I hadn't seen Mike's post on Planet (http://mike.kaply.com/2013/04/24/major-changes-coming-in-firefox-21/), it's quite possible we would have found this change the hard way during the release process for Firefox 21. 

This would have affected the EU ballot builds which use the same script.
It was announced prominently on dev-planning and dev-platform when it landed on nightly (Feb 7 "Upcoming separation between resource://app and resource://gre". What kind of "find out" are you looking for?
(In reply to Chris Cooper [:coop] from comment #28)
> (In reply to Michael Kaply (mkaply) from comment #26) 
> > I emailed Kev to get input.
> 
> How can release engineering find out about these changes earlier in the
> future?
> 

Who owns partner builds now? Clearly it's not Kev Needham. I tried to get the right people in the loop on this early on, but got no replies.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> It was announced prominently on dev-planning and dev-platform when it landed
> on nightly (Feb 7 "Upcoming separation between resource://app and
> resource://gre". What kind of "find out" are you looking for?

I don't think all the repercussions of this were clear to people when that was posted.

Things like autoconfig and distribution are second class citizens.
(In reply to Michael Kaply (mkaply) from comment #30)
> Who owns partner builds now?

Also, why aren't partner builds attempted on m-c or m-a? Beta is a bit late to find out such things.
(In reply to Mike Hommey [:glandium] from comment #31)
> (In reply to Michael Kaply (mkaply) from comment #30)
> > Who owns partner builds now?

I believe this is Joanne Nagel now, along with Gavin if needed.
(In reply to Michael Kaply (mkaply) from comment #16)
> If an installed Firefox distribution (Bing, Yahoo, etc.) has files in
> distribution, will they get uninstalled and then moved to the new
> distribution directory?

Hi, has this problem been solved? I tried put an distribution.ini into a 20.0b7 installation and it stopped working after Fx updated to latest 21.0 beta.
> Hi, has this problem been solved? I tried put an distribution.ini into a 20.0b7 installation and it stopped working after Fx updated to latest 21.0 beta.

With FF 21, the distribution directory goes in the browser subdirectory now, not the root.
(In reply to Michael Kaply (mkaply) from comment #34)
> 
> With FF 21, the distribution directory goes in the browser subdirectory now,
> not the root.
So any existing partner build installation will lose their partner channel information?
(In reply to Hector Zhao [:hectorz] from comment #35)
> (In reply to Michael Kaply (mkaply) from comment #34)
> > 
> > With FF 21, the distribution directory goes in the browser subdirectory now,
> > not the root.
> So any existing partner build installation will lose their partner channel
> information?

No, the upgrade would have the distribution directory in the new location.

Partner builds get updated with partner builds, don't they? I sure hope so...
(In reply to Michael Kaply (mkaply) from comment #36)
> No, the upgrade would have the distribution directory in the new location.
> 
> Partner builds get updated with partner builds, don't they? I sure hope so...

https://aus3.mozilla.org/update/3/Firefox/19.0/20130215130331/WINNT_x86-msvc/en-US/release/Windows_NT%206.2/default/default/update.xml

https://aus3.mozilla.org/update/3/Firefox/19.0/20130215130331/WINNT_x86-msvc/en-US/release-cck-google/Windows_NT%206.2/google-cjk/1.1/update.xml

Partner builds seems to get the same update as release channel
(In reply to Hector Zhao [:hectorz] from comment #37)
> 
> Partner builds seems to get the same update as release channel

Then we have a very serious problem...
(In reply to Michael Kaply (mkaply) from comment #38)
> (In reply to Hector Zhao [:hectorz] from comment #37)
> > 
> > Partner builds seems to get the same update as release channel
> 
> Then we have a very serious problem...

It IS really a *serious* problem for China Edition users.  Anything we can do at this point ? Just a few days before the final release.
Basically in the current situation, after China Edition users get their firefox upgraded to Firefox 21, which is going to happen next Tueday, they will lost all the information stored in distribution diretory.

Still is it possible we keep the distribution diretory in the root for Firefox 21 ?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
will do some tests for the Partner Repacks. Also informed r-d about this.
We either need to keep the distribution directory where it is, or ensure that on updates existing directories get moved. We cannot ship 21 with this change in place if partner distributions break.

Partner distributions have always updated form the release channel by design. This allows us to provide a single update to all users, and the use of the distribution directory is key, because it is untouched in an update. Because it is being touched in this particular update, there needs to be a migration function in place.

Adding RS so he's aware, and given the timeframe and potential impact it may make sense to consider backing this out.
(In reply to Carsten Book [:Tomcat] from comment #40)
> will do some tests for the Partner Repacks. Also informed r-d about this.

ok did some testing and the impact of this change to update is (as far as i see)

-> Build get reset so a Vanilla Firefox, so Update Channel, Build identification etc are like a normal Build, so more able to say this is a partner build like yahoode

-> The distribution folder is still there, however of course not copied/applied since there is now the browser directory

-> All Customizations are reset, this apply's to bookmarks, search engine (moved in a test from customized yahoo back to google default, homepage reset to Firefox default) .

The only thing that stayed was the toolbar (maybe get copied into the profile?)
Toolbars get copied into the user profile on first-run of a partner build, so that's expected behaviour.
I think we can add a one-off for the 20 -> 21 update that copies the distribution folder from gre to app. Unfortunately I'm not familiar enough with toolkit update code to know where that might happen.
I filed a RelEng bug that will get us testing partner repacks in our update tests that we do as part of each release, to help catch this sort of thing earlier. bug 870766.
(In reply to Kev Needham [:kev] from comment #41)
> Adding RS so he's aware, and given the timeframe and potential impact it may
> make sense to consider backing this out.

There is nothing reasonable to back out. Backing out would mean removing metro. However, the patch in this bug can be landed, although it only fixes half of the problem, as mentioned in comment 20 and comment 21.
> There is nothing reasonable to back out. Backing out would mean removing
> metro. However, the patch in this bug can be landed, although it only fixes
> half of the problem, as mentioned in comment 20 and comment 21.

I should mention that I won't be able to look at this today, so if someone wants to add extensions and bundles, please go ahead.
Assignee: mh+mozilla → nobody
Then I'll be asking for a halt on the release, because we can't ship Fx the way it is, unless we can find a fix that works per comment #44

(In reply to Mike Hommey [:glandium] from comment #46)
> (In reply to Kev Needham [:kev] from comment #41)
> > Adding RS so he's aware, and given the timeframe and potential impact it may
> > make sense to consider backing this out.
> 
> There is nothing reasonable to back out. Backing out would mean removing
> metro. However, the patch in this bug can be landed, although it only fixes
> half of the problem, as mentioned in comment 20 and comment 21.
I'm trying to reproduce this as described using Firefox 19.0.2 en-US and the steps from our Moztrap test[1].

Steps:
1. Download and install Firefox 19.0.2
2. Create a "distribution" folder in the installation directory
3. Create a "distribution.ini" file with in the distribution folder with the following text:
> [Global]
> id=MozillaOnline
> version=2013.2
> about=北京谋智网络技术有限公司
> about.zh-CN=北京谋智网络技术有限公司
> about.en-US=Mozilla Online 
4. Change the update channel to "betatest" in channel-prefs.js
5. Start Firefox and install a Persona and toolbar add-on
6. Go to a few websites and bookmark them
7. Change the search engine to something other than Google
8. Open the About dialog to check for updates
> About dialog shows the distribution branding
> Update is found and downloaded
9. Restart Firefox
> Update to Firefox 21.0 is applied
> Distribution branding is lost in the about dialog
> Toolbar, persona, bookmarks, and search provider are intact

Unfortunately I don't think I'm able to reproduce this issue. Can someone please clarify the steps to reproduce? 

[1] https://moztrap.mozilla.org/manage/case/6785/
> Unfortunately I don't think I'm able to reproduce this issue. Can someone please clarify the steps to reproduce? 

You've reproduced it

> Distribution branding is lost in the about dialog

This is a big deal.

Distribution.ini doesn't just contain branding, it contains preferences and more.

Also, if a new profile is created, it won't get any of the distribution stuff like bookmarks and the toolbar.
So as near as I can tell the only thing I'm reproducing is the branding in the about dialog. Is there something I should be adding to our test distribution.ini to test toolbars, bookmarks, search etc? It seems like our test is flawed and does not catch this behaviour.
The missing branding in the dialog is enough of a clue that something is wrong, so you shouldn't need anything additional.

If the branding is missing, distribution.ini is not being loaded.
Okay, thanks Michael. I'm not sure how this was missed given the steps are in our test but that's a post-mortem action for me to take; we don't need to discuss it further in this bug. Let's focus on getting this fixed and verified.
For clarity, the updater updates files (adds, mods, and deletes) and has no concept of "migration" beyond what the mar contains so it is possible to add / modify (in place without moving) / remove distribution files during an update so mars per distribution could be created to do this though it would be a HUGE PITA. The release mar updates ignore the distributions directory since they aren't included in our builds which allows our regular release update mar files to be used for distributions as well instead of requiring each distribution to have their own update mar files.

If the distributions code can't be made to use the original distributions directory then the best I can likely do is add post update code to move the directory to the new location. This would likely require running this new post update code synchronously so the move is performed before Firefox launches which will impact startup time. Also, it would have to run synchronously for all future updates until we decided that either the number of remaining users are small enough that we are OK that they don't have the customizations on the first launch after an update or we can require all users to update to a specific version that runs the post update code synchronously so they already have the change when updating to newer versions.

The above is all off the top of my head and rather risky so I'd very much prefer if the original distributions directory were used and though I know next to nothing about the distributions code the attached patch seems a lot safer to me than the changes I outlined above.
I agree putting distribution back, but there were problems with that as well. Extensions and bundles didn't work.

Plus from bsmedberg:

> bundles are not checked against app IDs and must not live in the toplevel distribution directory.

If there is a distribution directory in metro, does metro Firefox load anything from it?

Does metro Firefox look at the distribution directory at all?
(In reply to Michael Kaply (mkaply) from comment #55)
> I agree putting distribution back, but there were problems with that as
> well. Extensions and bundles didn't work.
> 
> Plus from bsmedberg:
> 
> > bundles are not checked against app IDs and must not live in the toplevel distribution directory.
Seems like they could be made to work especially since aiui metro is not built for the release this bug is about.

> 
> If there is a distribution directory in metro, does metro Firefox load
> anything from it?
> 
> Does metro Firefox look at the distribution directory at all?
That can be handled later since for the purpose of the release this bug is about metro is not enabled.
(In reply to Michael Kaply (mkaply) from comment #55)
> Does metro Firefox look at the distribution directory at all?

metro is a normal app, just like firefox, using the same gre dir. If distribution code to leverage those resources is in platform and loads from gre, metro will use those resources too with the exception of any components that are prefed off, like addons.

Is there a pref we can flip that turns distribution resource use off that we can add to app prefs? Then we could move distribution back to gre and think about this over a longer period of time.

If we do move it back and do nothing, these resources will get imported into metrofx. Hard to say what the side effects of that might be. You said yourself nobody who uses this feature expects to leverage windows 8, is that really the case? Or do we anticipate that these users will migrate to win8 in time and ultimately start running into problems getting the default metro browser running?
We should fix this to work as it did previously, and file a blocker for the release of Metro to ensure that we handle these cases correctly for those users, and make sure we have a migration/compatibility strategy in place before that time.

I do not believe that concerns over future Metro issues should justify any bustage to distribution.ini in Firefox 21.  Metro is not a shipping product yet.

Assigning this to glandium provisionally, since he appears to have blame.  If there's someone else who can fix it sooner, that's fine.
Assignee: nobody → mh+mozilla
(In reply to Mike Connor [:mconnor] from comment #58)
> We should fix this to work as it did previously, and file a blocker for the
> release of Metro to ensure that we handle these cases correctly for those
> users, and make sure we have a migration/compatibility strategy in place
> before that time.
> 
> I do not believe that concerns over future Metro issues should justify any
> bustage to distribution.ini in Firefox 21.  Metro is not a shipping product
> yet.
> 
> Assigning this to glandium provisionally, since he appears to have blame. 
> If there's someone else who can fix it sooner, that's fine.

Based on comment 47 & given we need an urgent fix here to re-spin our RC build asap we had an email thread to find a potential assignee here and :bsmedberg said he may be able to help here.

So needinfo'ing him to check the state here .
Flags: needinfo?(benjamin)
I've got what I believe is a patch together. My patch is taking forever.
build is taking forever, so I haven't verified yet.
OK, I've tested my patch with:

distribution.ini (glandium's patch) - works
distribution/searchplugins (glandium's patch) - works
distribution/bundles (my patch) - works
distribution/extensions (my patch) - works

So I think this covers the code. There are some tests that will probably need changing as well.
Comment on attachment 748230 [details] [diff] [review]
Patch for bundles and extensions as well

There's some weirdness here in the difference between the gredir (used in nsXREDirProvider::LoadAppBundleDirs) and XREExeF (used in browser/components/distribution and browser/components/dirprovider).

I don't think this actually matters in practice: it would only affect users with a separate GRE (Linux distros) who also had distribution data (not Linux distros).
Attachment #748230 - Flags: review+
Flags: needinfo?(benjamin)
Maybe we need a new directory concept in the provider? The app directory versus the EXE directory?

Are there ever cases where we need that directory?
Bhavana, how do you want to handle this? These fixes are very localized to distribution, so there is little risk to anything except the distribution system. But I don't know whether we have any automated tests for the distribution system or how thorough they are (or even whether they need to be updated against these patches).

mkaply, did you do a tryserver run with these?
mkaply, the provider could use XREExeF also.
All of these tests:

/toolkit/mozapps/extensions/test/xpcshell/test_bug430120.js
/toolkit/mozapps/extensions/test/xpcshell/test_distribution.js
/toolkit/mozapps/extensions/test/xpcshell/test_langpack.js

These are the only ones I found with references to "distribution"
No, I haven't done a tryserver. Honestly, I've never done that before. I'll take a look at that later tonight.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #65)
> Bhavana, how do you want to handle this? These fixes are very localized to
> distribution, so there is little risk to anything except the distribution
> system. But I don't know whether we have any automated tests for the
> distribution system or how thorough they are (or even whether they need to
> be updated against these patches).
> 
> mkaply, did you do a tryserver run with these?

Can QA confirm about the current testing situation for the distribution system ? Do we have anything in Mozmill at all or could we do manual testing to be confident here ?
Our Mozmill testing isn't set up for this type of situation which is why we set up the manual regression test in Moztrap. That said, the only thing our Moztrap test covers is installing a software update with a custom distribution.ini file and verifying the branding remains intact post-update.

My understanding is that this is sufficient to test this bug. Unfortunately we have no other tests, nor the expertise, for doing anything more focused than that.
(In reply to bhavana bajaj [:bajaj] from comment #69)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #65)
> > Bhavana, how do you want to handle this? These fixes are very localized to
> > distribution, so there is little risk to anything except the distribution
> > system. But I don't know whether we have any automated tests for the
> > distribution system or how thorough they are (or even whether they need to
> > be updated against these patches).
> > 
> > mkaply, did you do a tryserver run with these?
> 
> Can QA confirm about the current testing situation for the distribution
> system ? Do we have anything in Mozmill at all or could we do manual testing
> to be confident here ?

Benjamin,Given comment #70 and keeping in mind I will request additional testing help from Tomcat,Hong I think we should be in a good state to make sure required testing is done.

Do you think this is good to land now on release-branch or do you recommend we kick-off a try server build with this patch before our final RC ? In addition to the patch,do we still need some changes to our test code so our try servers are happy ?
Yes, we are stay tuned, will help on testing as soon as it is available.
Looks like try servers are not happy:

For 5a9b84cd238e, I see xpcshell test failures in /builds/slave/talos-slave/test/build/tests/xpcshell/tests/browser/components/places/tests/unit/test_browserGlue_distribution.js 

For 638928940740 : The above failure , plus a lot more :(

Kaply,glandium : Any ideas why these are failing ?
I have a fix for browser glue. I have no idea on the other failure.
This has the fix for the test failure.
Attachment #748230 - Attachment is obsolete: true
Comment on attachment 748344 [details] [diff] [review]
Original patch + test failure fix

Carrying over r=bsmedberg

[Approval Request Comment]
Regression caused by (bug #):755724 

User impact if declined: 
Partner builds will lose their customization

Testing completed (on m-c, etc.): 
Mozilla-central and beta try server

Risk to taking this patch (and alternatives if risky):
Low risk. Only affects code that uses the distribution directory

String or IDL/UUID changes made by this patch:
None
Attachment #748344 - Flags: review+
Attachment #748344 - Flags: approval-mozilla-release?
Comment on attachment 748344 [details] [diff] [review]
Original patch + test failure fix

had an irc conversation with :mkaply/:mbrureck and we discovered that test_browserGlue_distribution.js is the only real failure on those try build's which Kaply has a fix for.Also confirmed with :mbrureck that the overall patch looks good to land on mozilla-release.hence the a+ .
Attachment #748344 - Flags: approval-mozilla-release? → approval-mozilla-release+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #63)
> Comment on attachment 748230 [details] [diff] [review]
> Patch for bundles and extensions as well
> 
> There's some weirdness here in the difference between the gredir (used in
> nsXREDirProvider::LoadAppBundleDirs) and XREExeF (used in
> browser/components/distribution and browser/components/dirprovider).
> 
> I don't think this actually matters in practice: it would only affect users
> with a separate GRE (Linux distros) who also had distribution data (not
> Linux distros).

I think some distros are using the distribution stuff. But few are actually doing FF-on-XR (which is the only place where this matters), and I don't think Fedora, who does, is using it. Debian, which is the other I know of that does FF-on-XR, doesn't.

However, to avoid any unexpected problems, I'd advise to fix this up by using XREExeF everywhere. Probably too late for 21, but at least for 22.
(In reply to Michael Kaply (mkaply) from comment #79)
> https://hg.mozilla.org/releases/mozilla-release/rev/79fb387132d5

This did not land cleanly as expected.May have to backed out .

https://tbpl.mozilla.org/?tree=Mozilla-Release
(In reply to bhavana bajaj [:bajaj] from comment #81)
> 
> This did not land cleanly as expected.May have to backed out .
> 
> https://tbpl.mozilla.org/?tree=Mozilla-Release

It looks like similar changes should be done in do_register_cleanup at https://hg.mozilla.org/releases/mozilla-release/file/79fb387132d5/browser/components/places/tests/unit/test_browserGlue_distribution.js#l94 , I'll need some time to set up the environment on my home laptop before I can prepare a patch and start a try build.
(In reply to Hector Zhao [:hectorz] from comment #82)
> (In reply to bhavana bajaj [:bajaj] from comment #81)
> > 
> > This did not land cleanly as expected.May have to backed out .
> > 
> > https://tbpl.mozilla.org/?tree=Mozilla-Release
> 
> It looks like similar changes should be done in do_register_cleanup at
> https://hg.mozilla.org/releases/mozilla-release/file/79fb387132d5/browser/
> components/places/tests/unit/test_browserGlue_distribution.js#l94 , I'll
> need some time to set up the environment on my home laptop before I can
> prepare a patch and start a try build.

I could buy that, but that shouldn't influence the other tests, which are failing (since xpcshell tests are run independently). Except if tests are relying on previous tests having run before, which would be pretty bad in itself.
Anyways, testing that on try, with additional changes to use XREExeF.
https://tbpl.mozilla.org/?tree=Try&rev=ba9b0f018c77
(In reply to Mike Hommey [:glandium] from comment #83)
> 
> I could buy that, but that shouldn't influence the other tests, which are
> failing (since xpcshell tests are run independently). Except if tests are
> relying on previous tests having run before, which would be pretty bad in
> itself.

Comments on line 95-96: "Remove the distribution file, even if the test failed, otherwise all next tests will import it."

"Menu Link Before", "Toolbar Link Before" which is provided by the test distribution.ini can be seen in the tbpl logs.
(In reply to Mike Hommey [:glandium] from comment #84)
> Anyways, testing that on try, with additional changes to use XREExeF.
> https://tbpl.mozilla.org/?tree=Try&rev=ba9b0f018c77

Thanks!
Local testing suggests this works. Try build from comment 84 will tell more.
Attachment #748389 - Flags: review?(benjamin)
Note I'm likely to be away by the time this can be landed, so please land this for me when it's green on try and reviewed if I'm not around by then.
(In reply to bhavana bajaj [:bajaj] from comment #72)
 confident here ?
> 
> Benjamin,Given comment #70 and keeping in mind I will request additional
> testing help from Tomcat,Hong I think we should be in a good state to make
> sure required testing is done.
> 

yep confirmed , also standing by for testing
btw backed out the change to the partner repack scripts in bug 871137 to revert for the partner builds generation the directory change
Looking at the try server, this new patch built clean.
:benjamin or anyone else who is comfortable with this code & available atm, can you please help with review/landing of glandium's patch ?
Flags: needinfo?(benjamin)
Attachment #748389 - Flags: review?(benjamin) → review+
Comment on attachment 748389 [details] [diff] [review]
Fixup test_browserGlue_distribution.js and use XREExeF in nsXREDirProvider

https://hg.mozilla.org/releases/mozilla-release/rev/f770da61f391
Attachment #748389 - Flags: checkin+
Thanks Gavin and Phil.

What do we do for other branches? Do we want to land this (in which case aurora and m-c should be enough if landing before the merge day), or do we want to handle a "proper" migration?
(In reply to Mike Hommey [:glandium] from comment #94)
> Thanks Gavin and Phil.
> 
> What do we do for other branches? Do we want to land this (in which case
> aurora and m-c should be enough if landing before the merge day),

I would prefer this to be consistent and want to go that route .
> or do we
> want to handle a "proper" migration?

Can you explain what you mean by this ?
I think we should land this patch and file a followup for any other potential changes (trunk-only).
We have updates on betatest channel now for Firefox 21.0build3. I've checked the "custom distribution" case as previously outlined in comment 49 and a few Partner Repack builds. In either case this appears to be fixed now.

Tomcat and Hong, can you please do more exploratory testing around the distribution system? Thanks.
QA from Beijing office also tested the build with China Edition, it appears that the problem got fixed !
Thanks !
status-firefox22: --- → fixed
Flags: needinfo?(benjamin)
Hong and Carsten have reported via email that this is fixed in 21build4, overnight testing confirms this as well. Can we mark this bug fixed now?
No, it hasn't been merged to mozilla-central yet.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #101)
> No, it hasn't been merged to mozilla-central yet.

Ah yes, you are right. I think we can mark verified fixed for Firefox 21 though. Thanks everyone who's helped out with this bug.
status-firefox21: fixed → verified
https://hg.mozilla.org/mozilla-central/rev/c4f761a0ff76
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
What about aurora?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #103)
> https://hg.mozilla.org/mozilla-central/rev/c4f761a0ff76

(In reply to Michael Kaply (mkaply) from comment #104)
> What about aurora?

:RyanVM, I know this fix landed on m-c the same morning around the time the train migrations happened, so can you clarify if this fix also made it to aurora?
Status: RESOLVED → REOPENED
status-firefox21: verified → fixed
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
Target Milestone: mozilla24 → ---
(In reply to John O'Duinn [:joduinn] from comment #105)
> :RyanVM, I know this fix landed on m-c the same morning around the time the
> train migrations happened, so can you clarify if this fix also made it to
> aurora?

It didn't:
https://hg.mozilla.org/mozilla-aurora/rev/c4f761a0ff76
Which is what the target milestone used to say. Also, why was this reopened?
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Resetting this to firefox21:verified since someone inadvertently changed it.

For the record I think this landed for:
 * Firefox 21 in Release
 * Firefox 22 in Aurora before the merge
 * Firefox 24 in Central after the merge

It did not land for 23 yet (currently Aurora).
status-firefox21: fixed → verified
Note, this leaves testpilot in browser/distribution/extensions. As a consequence, testpilot is disabled on new installs of the 22.0b1 candidate. Not sure if that matters too much in light of bug 867445 though
(In reply to Chris Coulson from comment #110)
> Note, this leaves testpilot in browser/distribution/extensions. As a
> consequence, testpilot is disabled on new installs of the 22.0b1 candidate.
> Not sure if that matters too much in light of bug 867445 though

Thanks for the note. When we re-add testpilot, we'll need to undo http://hg.mozilla.org/mozilla-central/diff/8279af77dc28/browser/app/profile/extensions/Makefile.in. I filed bug 872680.
Not sure this is entirely fixed. 

Yahoo is reporting than their German partner repack (yahoo-de) is losing it's customizations when they update from older versions to 21.0. I've reproduced this myself by doing the following:

* download the yahoo-de build: http://stage.mozilla.org/pub/mozilla.org/firefox/candidates/18.0-candidates/build1/partner-repacks/yahoo-de/win32/de/Firefox%20Setup%2018.0.exe
* set app-update.log to true
* AUS:SVC Checker:getUpdateURL - update URL: https://aus3.mozilla.org/update/3/Firefox/18.0/20130104151925/WINNT_x86-msvc/de/release-cck-yahoode/Windows_NT%206.1.1.0%20(x64)/yahoode/1.16/update.xml?force=1
* after update, application.ini shows "SourceStamp=30ec6828d10e" which corresponds to build3, but all customizations (notably the Yahoo/Bing search engine are lost)

Yahoo is only reporting problems with the de locale.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
rstrong, this is assigned to Mike but may be best to have somebody on your team look at comment 113 given update/customization implications
Flags: needinfo?(robert.bugzilla)
The only thing I can think of that would cause this is the mar generation code which has versions written in both shell and python or the precomplete generation code which is written in python though it could also be removed-files.in. Basically, the mar and precomplete generation scripts need to exclude the distributions directory when generating mar files. I should be able to investigate next week but the rest of my team that knows update code are already over allocated.

coop, can you check if other locales since it seems very strange that only de would be affected?
Flags: needinfo?(robert.bugzilla) → needinfo?(coop)
I just checked the precomplete script and it looks fine and it also hasn't been changed significantly in a very long time
http://mxr.mozilla.org/mozilla-release/source/config/createprecomplete.py

I also checked the precomplete file in the partner repack and it looks fine (e.g. no reference to distribution)
http://stage.mozilla.org/pub/mozilla.org/firefox/candidates/18.0-candidates/build1/partner-repacks/yahoo-de/win32/de/Firefox%20Setup%2018.0.exe

So, that leaves the mar file generation scripts or removed-files.in
Rob, is there anything I can have Softvision QA do overnight to assist in this investigation?
Anthony, nothing I can think of except possibly the question at the end of comment #115
(In reply to Robert Strong [:rstrong] (do not email) from comment #118)
> Anthony, nothing I can think of except possibly the question at the end of
> comment #115

Sure, I'll ask them to spotcheck some different Yahoo locales and different partner builds.
I verified that it isn't removed-files.in so I updated using the partner build in comment #113 and all of the distribution files are still intact! This appears to be a bug in the distributions code which is really not well owned by anyone. :(
Specifically, the error is
Zeitstempel: 6/13/2013 5:50:05 PM
Fehler: [Exception... "Could not convert Native argument arg 2 [nsIINIParser.getString]"  nsresult: "0x8057000a (NS_ERROR_XPC_BAD_CONVERT_NATIVE)"  location: "JS frame :: resource://app/modules/distribution.js :: DIST_applyPrefDefaults :: line 269"  data: no]
Quelldatei: resource://app/modules/distribution.js
Zeile: 269

Filed bug 882989
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: needinfo?(coop)
Resolution: --- → FIXED
Hmm, so is there something about the yahoo-DE build that's broken in some special way?  Was there any errors in console or anything that would give us a clue about what's up here?
(In reply to Mike Connor [:mconnor] from comment #123)
> Hmm, so is there something about the yahoo-DE build that's broken in some
> special way?  Was there any errors in console or anything that would give us
> a clue about what's up here?

The update process gets hung up on non-utf-8 chars in the distribution.ini file. See https://bugzilla.mozilla.org/show_bug.cgi?id=882989#c8 and later.
(In reply to Chris Cooper [:coop] from comment #124)
> (In reply to Mike Connor [:mconnor] from comment #123)
> > Hmm, so is there something about the yahoo-DE build that's broken in some
> > special way?  Was there any errors in console or anything that would give us
> > a clue about what's up here?
> 
> The update process gets hung up on non-utf-8 chars in the distribution.ini
> file. See https://bugzilla.mozilla.org/show_bug.cgi?id=882989#c8 and later.
Actually the update process works fine and does not get hung up. We do our partner repacks by adding files that the update process usually never even sees (the exception is when we need to do one-off mar files to update them which is how bug 882989 will solve this issue) which allows us to use the same mar file for partner repacks as we do for our own builds. What happened is the distribution.ini file was incorrectly encoded as ANSI and something changed to make nsIINIParser to throw in some of the cases where the ini file was encoded as ANSI when there were certain characters contained in the file. I did some investigation on mxr but didn't see any changes to nsIINIParser that I thought would have caused it to now throw.

See bug 882989 comment #0 for the error being thrown by distribution.js.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: