1113222 - OSX updates appear to be dropping distribution information

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Updates on OSX seem to be dropping the partner repack information.

STR:
- Download a 33.1 partner repack build, such as:
https://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/33.1.1-candidates/build1/partner-repacks/bing/mac/en-US/
- Run it, check that the pref "distribution.id" is present
- Open the About dialog to update it to 34.0
- After update, "distribution.id" is no longer present


Note: with a direct 34.0 build, the information is present. This is only happening when it gets updated.

Comment 1

[Mike Connor [:mconnor]]

This is a really big deal.  We should immediately pull OS X updates for these builds.

Comment 2

[[e-mail] (:bhearsum)]

Updates are shut off. I still have some tweaking to do because the immediate action was to shut off updates for almost everything. Blocking only partner updates is tougher.

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> Updates are shut off. I still have some tweaking to do because the immediate
> action was to shut off updates for almost everything. Blocking only partner
> updates is tougher.

Once we get the OS X specific blocks into place /opt/aus2/incoming/Firefox needs to be chmoded back to 755 to re-enable other updates.

Comment 4

[Robert Strong (they/them - no direct email)]

Felipe, can you attach the distribution directory from the broken app bundle if it is present? Thanks!

Comment 5

[Benjamin Smedberg]

My understanding of bug 1047738 and bug 1048890 was that we were going to have to generate custom MARs for all of the mac partner builds for v2 signing. rstrong, was there a bug specifically tracking that?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Felipe, can you attach the distribution directory from the broken app bundle
> if it is present? Thanks!

Aah I just deleted what I had to start a fully clean new STRs, only to realize that now with updates disabled that won't be possible. But Kamil has the steps on top of his head to perform a manual .mar update and kindly said he will do it

Comment 7

[Kamil Jozwiak [:kjozwiak]]

Attachment: distribution.7z

I went through two 33.1 partner builds (AOL, Bing) and reproduced the same problem. Once 33.1 was updated to 34.0.5 via manual .mar, the distribution.id pref was missing from about:config. I than tried the same thing on a Win machine using the same steps and the distribution.id pref was present after the update. I've also included the update.log just in case (I noticed some errors relating to the distribution folder).

Comment 8

[Robert Strong (they/them - no direct email)]

This isn't about creating partner mars though that would have fixed this. This bug is a "which comes first (the chicken or the egg)" bug that slipped through due to all of the changes needed for v2 signing. Specifically, the updater moves the distribution directory over to the new location but the updater that applies the update doesn't have the code to perform that task.

The good news is that affected builds will get the distribution directory moved on next update.

The way to fix this so there is no client impact is to use a separate binary that is added by the update and runs post update and runs sync which is already built into the updater. This would require creating a new binary for this purpose and uplifting this into release. I think that all partner builds should require updating to this build and then the binary should be removed from future builds since this will need to run sync.

spohl, your mac chops are much better than mine... could you take this on if this is the route we take?

Another option that would affect clients is to update the user twice. Once to get the new updater and a second time to run the updater again to move the distribution directory.

bsmedberg, I'm leaning towards the first approach... which approach do you think should be taken?

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

Updates are now blocked for partner builds, see bug 1113455 for details, and back on for everything else.

Comment 10

[Robert Strong (they/them - no direct email)]

The option we'll go with is using the add-on hotfix to copy existing distribution directories over to the new location. When the app is updated to the version with the changes it will just use the copied distribution directory. On the next update the old distribution directory will be removed by the updater without trying to copy it again.

Now I'll figure out how to create an add-on hotfix.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

There's some good docs on how to create & test hotfixes here: https://developer.mozilla.org/en-US/Add-ons/Hotfix

Comment 12

[Stephen A Pohl [:spohl] (OOO until 7/30, contact :haik if urgent)]

I'm clearing the n-i request based on the assumption that we no longer need a separate binary, but please let me know if we go back to that route. I'd be happy to help!

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Robert I believe the hotfix is live now and this bug is no longer an issue, can you confirm?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I didn't see an "intent to hotfix" thread on release-drivers, or any bug traffic about a hotfix, so that would be surprising!

Comment 15

[Robert Strong (they/them - no direct email)]

It isn't live and I'm working on the hotfix.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Ok, thanks for the update (I must be confusing this hotfix with the other most recent one).  Since this is a hotfix resolution though, it doesn't block gtb for 35 RC and we can go ahead without it.  Leaving the tracking, but nothing actionable for the builds.

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Hi Felipe, I went ahead and kept your last hotfix in this new hotfix. Basically, the distribution directory on Mac OS needs to be copied to the new location under Resources if it exists. The updater has code to clean up the old location so it doesn't remove it in the hotfix.

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 8545119 [details] [diff] [review]
patch rev1

Stephen, I want to run this by you as well. Thanks!

Comment 19

[Kamil Jozwiak [:kjozwiak]]

Robert, let me know if you need this tested once it's ready!

Comment 20

[Stephen A Pohl [:spohl] (OOO until 7/30, contact :haik if urgent)]

Comment on attachment 8545119 [details] [diff] [review]
patch rev1

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

Looks great to me! Minor comments below.

::: v20150106.01/bootstrap.js
@@ +37,5 @@
> +      try {
> +        // Override the special "isUS" logic.
> +        Services.prefs.setBoolPref("browser.search.isUS", false);
> +        // Note that we applied this hotfix in case that is useful later.
> +        Services.prefs.setBoolPref("browser.hotfix.v20141211.applied", true);

I'm assuming that the hotfix version in this pref was intentionally left at v20141211?

@@ +50,5 @@
>      try {
> +      var newDistro = Services.dirsvc.get("XREExeF", Ci.nsIFile).parent;
> +      let oldDistro = newDistro.clone();
> +      oldDistro.append("distribution");
> +      if (oldDistro.exists()) {    

nit: whitespace at end of line

@@ +61,3 @@
>      } catch (ex) {
> +      logError("copying distribution directory to new loaction failed: " + ex);
> +      if (newDistro.exists()) {

Could we limit this catch block (or at least the removal of newDistro) to the copyTo only? I'm a bit concerned that we could end up removing a legitimate newDistro directory if an exception is thrown elsewhere, i.e. not during copyTo.

@@ +93,5 @@
>  }
>  
> +/**
> + * @return boolean whether the hotfix applies to the version of the application
> + * for Bug 1113222.

Could we change this comment (or add a comment in the function body) to say that this only applies to OSX? It wasn't directly obvious to me that the |"nsILocalFileMac" in Components.interfaces| check below was to determine whether or not we were on OSX. Based on the comment, I assumed that we wanted to check whether or not we were running a version that had nsILocalFileMac in Components.interfaces, which I think has been the case since FF4...

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8545119 [details] [diff] [review]
patch rev1

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

There's a README file in the root of the repo where we add a description about each hotfix. I'm guilty of having forgotten to update it last time, but I just pushed an update in the other bug with it. Just a simple description of what the hotfix does and its target versions.

::: v20150106.01/bootstrap.js
@@ +96,5 @@
> + * @return boolean whether the hotfix applies to the version of the application
> + * for Bug 1113222.
> + */
> +function shouldHotfixBug1113222() {
> +  if ("nsILocalFileMac" in Components.interfaces)) {

It's common to add an in-code check for the desired version range in case campitibility checking was overriden and the hotfix gets installed in a version that's not meant to be installed.
Probably not a real problem on the circumstances here, but maybe worth it?

::: v20150106.01/install.rdf
@@ +9,1 @@
>      <em:bootstrap>true</em:bootstrap>

Can you add <em:targetPlatform>Darwin</em:targetPlatform> ?

@@ +16,5 @@
>      <em:targetApplication>
>        <Description>
>          <!-- Firefox -->
>          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> +        <em:minVersion>10.0</em:minVersion>

Is this the right minVersion? Shouldn't this hotfix only run for versions post the v2 signing changes?

I guess you're pre-emptively fixing older users ahead of time so they don't ever face the problem, right? In that case, this is fine, but we can't easily release hotfixes for this version range without signing them 3 times. (because the pinned certificate changed on version 16.0.2 and 24.0). To avoid the hassle, I suggest setting minVersion to 24.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8545119 [details] [diff] [review]
patch rev1

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

::: v20150106.01/bootstrap.js
@@ +32,5 @@
> +      distroID = defaultBranch.getCharPref("distribution.id");
> +    } catch (ex) {}
> +
> +    // If we are a distribution build.
> +    if (distroID) {

Oh another thing, this check won't work properly until the second part of the hotfix runs (and probably after a restart? the pref distribution.id needs to get picked properly).

Does the "distribution" directory only exists for partner builds? If yes, than I think you can replace this check with the oldDistro.exists() from below. Otherwise, it might need more tinkering.

Comment 23

[Robert Strong (they/them - no direct email)]

(In reply to :Felipe Gomes from comment #21)
> Comment on attachment 8545119 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8545119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's a README file in the root of the repo where we add a description
> about each hotfix. I'm guilty of having forgotten to update it last time,
> but I just pushed an update in the other bug with it. Just a simple
> description of what the hotfix does and its target versions.
> 
> ::: v20150106.01/bootstrap.js
> @@ +96,5 @@
> > + * @return boolean whether the hotfix applies to the version of the application
> > + * for Bug 1113222.
> > + */
> > +function shouldHotfixBug1113222() {
> > +  if ("nsILocalFileMac" in Components.interfaces)) {
> 
> It's common to add an in-code check for the desired version range in case
> campitibility checking was overriden and the hotfix gets installed in a
> version that's not meant to be installed.
> Probably not a real problem on the circumstances here, but maybe worth it?
> 
> ::: v20150106.01/install.rdf
> @@ +9,1 @@
> >      <em:bootstrap>true</em:bootstrap>
> 
> Can you add <em:targetPlatform>Darwin</em:targetPlatform> ?
I don't think I can since you want to also apply the other hotfix.

> @@ +16,5 @@
> >      <em:targetApplication>
> >        <Description>
> >          <!-- Firefox -->
> >          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> > +        <em:minVersion>10.0</em:minVersion>
> 
> Is this the right minVersion? Shouldn't this hotfix only run for versions
> post the v2 signing changes?
This should go out to all versions and I used the value from another hotfix that went out to all versions.

> I guess you're pre-emptively fixing older users ahead of time so they don't
> ever face the problem, right? In that case, this is fine, but we can't
> easily release hotfixes for this version range without signing them 3 times.
> (because the pinned certificate changed on version 16.0.2 and 24.0). To
> avoid the hassle, I suggest setting minVersion to 24.
hmmm... I think I'm ok with that.

Comment 24

[Robert Strong (they/them - no direct email)]

(In reply to :Felipe Gomes from comment #22)
> Comment on attachment 8545119 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8545119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: v20150106.01/bootstrap.js
> @@ +32,5 @@
> > +      distroID = defaultBranch.getCharPref("distribution.id");
> > +    } catch (ex) {}
> > +
> > +    // If we are a distribution build.
> > +    if (distroID) {
> 
> Oh another thing, this check won't work properly until the second part of
> the hotfix runs (and probably after a restart? the pref distribution.id
> needs to get picked properly).
> 
> Does the "distribution" directory only exists for partner builds? If yes,
> than I think you can replace this check with the oldDistro.exists() from
> below. Otherwise, it might need more tinkering.
I think I'll do distroID or oldDistro.exists just to be safe since it might be possible for an updated build to have completed the change to the new location and not have the changes from your hotfix.

Thanks!

Comment 25

[Robert Strong (they/them - no direct email)]

(In reply to Stephen Pohl [:spohl] from comment #20)
Will do

Comment 26

[Robert Strong (they/them - no direct email)]

(In reply to :Felipe Gomes from comment #21)
>...
> @@ +16,5 @@
> >      <em:targetApplication>
> >        <Description>
> >          <!-- Firefox -->
> >          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> > +        <em:minVersion>10.0</em:minVersion>
> 
> Is this the right minVersion? Shouldn't this hotfix only run for versions
> post the v2 signing changes?
> 
> I guess you're pre-emptively fixing older users ahead of time so they don't
> ever face the problem, right? In that case, this is fine, but we can't
> easily release hotfixes for this version range without signing them 3 times.
> (because the pinned certificate changed on version 16.0.2 and 24.0). To
> avoid the hassle, I suggest setting minVersion to 24.
After thinking about this I do want this to apply to all versions that support the hotfix in the same way that v20140527.01 supports all versions.

Comment 27

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Notes:
This does need to apply to all versions that the hotfix is deployed to in order to get the files in the right location before update.
This can't be limited to Darwin otherwise the Bug 1110307 hotfix won't be applied to other platforms.
I went with checking if the distribution.ini exists since the distribution directory can exist for non distribution cases.
I don't think it is worth consolidating savePrefFile though I could be convinced otherwise.

Comment 28

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Forget to refresh

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8546462 [details] [diff] [review]
patch rev1

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #26)
> > I guess you're pre-emptively fixing older users ahead of time so they don't
> > ever face the problem, right? In that case, this is fine, but we can't
> > easily release hotfixes for this version range without signing them 3 times.
> > (because the pinned certificate changed on version 16.0.2 and 24.0). To
> > avoid the hassle, I suggest setting minVersion to 24.
> After thinking about this I do want this to apply to all versions that
> support the hotfix in the same way that v20140527.01 supports all versions.

Alright. I don't know exactly how deployment worked for v20140527.01. I think it involved some custom AMO code to serve the right xpi to the right versions. gps or bhearsum should know the details.

::: v20150106.01/bootstrap.js
@@ +53,5 @@
> +          // distributions (e.g. add-ons deployment).
> +          hasDistributionINI = distributionINI.exists();
> +        }
> +    } catch (ex) {
> +      logError("copying distribution directory to new loaction failed: " + ex);

nit: typo "loaction"

Comment 30

[Robert Strong (they/them - no direct email)]

Attachment: patch with comment addressed.

Carrying forward r+

Comment 31

[Robert Strong (they/them - no direct email)]

Pushed to firefox-hotfixes
https://hg.mozilla.org/releases/firefox-hotfixes/rev/e6647840e241

Comment 32

[Robert Strong (they/them - no direct email)]

Filed bug 1119899 for signing, etc. of the hotfix and sent intent to ship email to release-drivers@m.o and moc@m.c per https://wiki.mozilla.org/Firefox/Hotfix

Comment 33

[[e-mail] (:bhearsum)]

(In reply to :Felipe Gomes from comment #21)
> @@ +16,5 @@
> >      <em:targetApplication>
> >        <Description>
> >          <!-- Firefox -->
> >          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> > +        <em:minVersion>10.0</em:minVersion>
> 
> Is this the right minVersion? Shouldn't this hotfix only run for versions
> post the v2 signing changes?
> 
> I guess you're pre-emptively fixing older users ahead of time so they don't
> ever face the problem, right? In that case, this is fine, but we can't
> easily release hotfixes for this version range without signing them 3 times.
> (because the pinned certificate changed on version 16.0.2 and 24.0). To
> avoid the hassle, I suggest setting minVersion to 24.

I think we're going to have to go with a minVersion of 24. While we still have the certs for 16.0.2 -> 24.0, I can't find the passphrase. I can't find any keys for < 16.0.2. I'm also not sure that we can sign a single XPI with multiple signatures (if we can, I certainly don't know how).

Should I sign this one, or wait for a new one?

Comment 34

[Robert Strong (they/them - no direct email)]

mconnor, I don't know the quantity of Mac partner distributions running less than Firefox 24 or whether letting these systems go through two updates to resolve this issue so I am looking for guidance from you on the acceptability of comment #33. Thanks!

Comment 35

[Robert Strong (they/them - no direct email)]

I just talked with bsmedberg and as I understand it there is a hotfix to update the cert info that is applied to systems prior to applying other hotfixes which works around the need to sign with multiple certificates. So, signing with the current cert should suffice.

Comment 36

[Robert Strong (they/them - no direct email)]

I had to land a followup fix (damn mac wouldn't boot so I couldn't test)
https://hg.mozilla.org/releases/firefox-hotfixes/rev/c8e13ae460f7

Comment 37

[Robert Strong (they/them - no direct email)]

Hotfix is deployed