841094 - "extensions" directory disappeared after updating to Feb12th Nightly

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Vladan Djeric (:vladan)]

After updating my Nightly from the Feb 11th to Feb 12th version, the "extensions" directory disappeared from C:\Program Files (x86)\Nightly\. This directory contained the default theme, and I now no longer see the default theme in the Addon Manager's "Appearance" tab.

This might be a regression from bug 755724.

Comment 1

[Mike Hommey [:glandium]]

The update.manifest in the complete.mar does this:

add-if "browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}" "browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf"
add-if "browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}" "browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/icon.png"

I guess the partial mars have the same thing.

Since the directory didn't exist, it doesn't create the file. Looking at the update-packaging code, it looks like it's intentional that we're putting an add-if for the extensions directory. Why are we doing that?

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> I guess the partial mars have the same thing.

Confirmed, the same thing is in
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/02/2013-02-12-03-11-20-mozilla-central/firefox-21.0a1.en-US.linux-x86_64.partial.20130211031055-20130212031120.mar

Comment 3

[Mike Hommey [:glandium]]

It would be interesting to know why there are special cases for searchplugins and extensions in the update-packaging stuff. Is it safe to remove all of them, now?

Comment 4

[Mike Hommey [:glandium]]

The special cases I'm talking about:
http://hg.mozilla.org/mozilla-central/file/081cf5b0121e/tools/update-packaging/make_incremental_updates.py#l44
http://hg.mozilla.org/mozilla-central/file/081cf5b0121e/tools/update-packaging/make_incremental_updates.py#l69

http://hg.mozilla.org/mozilla-central/file/081cf5b0121e/tools/update-packaging/common.sh#l50
http://hg.mozilla.org/mozilla-central/file/081cf5b0121e/tools/update-packaging/common.sh#l65

Note how the shell and python implementations don't do the same check (and i think we're using the shell scripts, the python script should not have done that)

Comment 5

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

Search plugins special casing seems to have been a leftover from pre-Firefox 2, when we didn't have a built-in way to disable default search plugins aside from deleting them from the appdir, and we wanted to not break updates for users who did that. See bug 325561.

For extensions, the context is in bug 299021, but I haven't read through that bug to see whether the concerns are still relevant.

Comment 6

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

(In reply to Mike Hommey [:glandium] from comment #3)
> It would be interesting to know why there are special cases for
> searchplugins and extensions in the update-packaging stuff. Is it safe to
> remove all of them, now?
What gavin said in comment #5. There is also an exception for extensions when generating the removals prior to applying a complete update.
http://mxr.mozilla.org/mozilla-central/source/config/createprecomplete.py#12

How would the final result between the shell and python scripts be any different in practice? I don't see it but my brain is flu addled right now.

I recall that releng used to use both the python and shell scripts and I haven't heard of that changing. One of the cc'd releng folk should be able to answer that.

The default theme always has an extension id of {972ce4c6-7e08-4474-a285-3208198ce6fd} and I think that could be checked during packaging to use an add or patch instruction instead of the add-if or patch-if instruction.

Comment 7

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

We no longer have optional extensions in release builds, since DOMi and Talkback are long gone. Testpilot may be present in distribution/extensions/, but that gets copied into the profile, right ?

Comment 8

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

(In reply to Nick Thomas [:nthomas] (gone Feb 14-Mar 3 PST) from comment #7)
> We no longer have optional extensions in release builds, since DOMi and
> Talkback are long gone. Testpilot may be present in
> distribution/extensions/, but that gets copied into the profile, right ?
That appears to be the case. The rule would likely need to be updated to only apply to distributions/extensions/ in update packaging and in createprecomplete.py if that path were taken. Since there are people that are in this state the next update they get would need add instructions for partial updates or they would need to get a complete update.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #6)
> How would the final result between the shell and python scripts be any
> different in practice? I don't see it but my brain is flu addled right now.

Because one is testing whether the path starts with extensions/ while the other checks if the path contains extensions/. The latter matches for browser/extensions/, the new location for the default theme, but the former doesn't.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Don't special-case extensions/ and searchplugins/ directories when creating complete and partial mar, but special-case distribution/extensions

Now the question is how to deal with partial upgrades from a version that doesn't have the extensions directory.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #12)
> Now the question is how to deal with partial upgrades from a version that
> doesn't have the extensions directory.

Considering this will only affect one set of partial upgrades, we only really need to handle this once. However there are several options:
- Temporarily change the common.sh script to forcefully add browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}
- Temporarily change the tinderbox scripts to remove browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd} from the previous application directory (after running unwrap_full_update.pl).
- Temporarily change the partial-patch rule in tools/update-packaging to remove browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd} from the previous application directory.
- etc.

I'd go for the second or third option, whichever works best.

Now, considering the timing, we also need to ensure it won't be a problem for aurora upgrades.

Comment 14

[Mike Hommey [:glandium]]

Another idea that might work to unscrew nightly users would be to skip generating partial updates for one nightly.

Comment 15

[[e-mail] (:bhearsum)]

(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > Now the question is how to deal with partial upgrades from a version that
> > doesn't have the extensions directory.
> 
> Considering this will only affect one set of partial upgrades, we only
> really need to handle this once.

Is this still going to hold true for users that don't get the update with this fix? Eg, we ship it tomorrow by someone doesn't start their Nightly until Thursday.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Ben Hearsum [:bhearsum] from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > (In reply to Mike Hommey [:glandium] from comment #12)
> > > Now the question is how to deal with partial upgrades from a version that
> > > doesn't have the extensions directory.
> > 
> > Considering this will only affect one set of partial upgrades, we only
> > really need to handle this once.
> 
> Is this still going to hold true for users that don't get the update with
> this fix? Eg, we ship it tomorrow by someone doesn't start their Nightly
> until Thursday.

These people are going to get the complete upgrade, aren't they?

Comment 17

[[e-mail] (:bhearsum)]

(In reply to Mike Hommey [:glandium] from comment #16)
> (In reply to Ben Hearsum [:bhearsum] from comment #15)
> > (In reply to Mike Hommey [:glandium] from comment #13)
> > > (In reply to Mike Hommey [:glandium] from comment #12)
> > > > Now the question is how to deal with partial upgrades from a version that
> > > > doesn't have the extensions directory.
> > > 
> > > Considering this will only affect one set of partial upgrades, we only
> > > really need to handle this once.
> > 
> > Is this still going to hold true for users that don't get the update with
> > this fix? Eg, we ship it tomorrow by someone doesn't start their Nightly
> > until Thursday.
> 
> These people are going to get the complete upgrade, aren't they?

Yeah.

Comment 18

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

Comment on attachment 715041 [details] [diff] [review]
Don't special-case extensions/ and searchplugins/ directories when creating complete and partial mar, but special-case distribution/extensions

This should be fine as long as gavin / kev are fine with people not being able to remove distributed searchplugins (for example, if someone distributes Firefox the original plugins must be present when applying a partial update). I am technically not a peer of these scripts so it would be a good thing to get one of the releng people to r+ it as well.

Comment 19

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

(In reply to Robert Strong [:rstrong] (do not email) from comment #18)
> This should be fine as long as gavin / kev are fine with people not being
> able to remove distributed searchplugins (for example, if someone
> distributes Firefox the original plugins must be present when applying a
> partial update).

This should be fine. There's no reason for users to do that anymore since we support "removing" default plugins through the UI, and customized builds should do it with distribution configs/bundled add-ons. We have no evidence that this was ever common to begin with, AFAIK (beyond a few reports of it in the 1.5 days).

Comment 20

[Mike Hommey [:glandium]]

It's also possible to customize the searchplugins order and default selection, I'm not sure there's much incentive for e.g. enterprises to go as far as remove some of the shipped searchplugins.

Comment 21

[bhavana bajaj [:bajaj]]

Adding QA to help with verification here once this lands, as a part of 21.0a2 sign-off.

Comment 22

[u279076]

(In reply to bhavana bajaj [:bajaj] from comment #21)
> Adding QA to help with verification here once this lands, as a part of
> 21.0a2 sign-off.

Added to my sign-off checklist.

Comment 23

[bhavana bajaj [:bajaj]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #18)
I am technically not a peer of these scripts so it would be
> a good thing to get one of the releng people to r+ it as well.

:bhearsum, can you please help here ? Mike may be waiting on one more review before landing.

Comment 24

[[e-mail] (:bhearsum)]

Comment on attachment 715041 [details] [diff] [review]
Don't special-case extensions/ and searchplugins/ directories when creating complete and partial mar, but special-case distribution/extensions

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

::: tools/update-packaging/make_incremental_updates.py
@@ +44,2 @@
>          """
> +        m = re.match("((?:|.*/)distribution/extensions)/", filename):

Although it's unlikely to be a problem, I think we should keep the restrictiveness of the old code. That is, only match "distribution/extensions" or "Contents/MacOS/distribution/extensions/". This matches anything containing "/distribution/extensions/" AFAICT.

@@ +65,2 @@
>          """
> +        m = re.match("((?:|.*/)distribution/extensions)/", filename):

Same here.

r=me with those changed.

Comment 25

[Mike Hommey [:glandium]]

(In reply to Ben Hearsum [:bhearsum] from comment #24)
> Comment on attachment 715041 [details] [diff] [review]
> Don't special-case extensions/ and searchplugins/ directories when creating
> complete and partial mar, but special-case distribution/extensions
> 
> Review of attachment 715041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/update-packaging/make_incremental_updates.py
> @@ +44,2 @@
> >          """
> > +        m = re.match("((?:|.*/)distribution/extensions)/", filename):
> 
> Although it's unlikely to be a problem, I think we should keep the
> restrictiveness of the old code. That is, only match
> "distribution/extensions" or "Contents/MacOS/distribution/extensions/". This
> matches anything containing "/distribution/extensions/" AFAICT.
> 
> @@ +65,2 @@
> >          """
> > +        m = re.match("((?:|.*/)distribution/extensions)/", filename):
> 
> Same here.
> 
> r=me with those changed.

Until something is decided about bug 842334, and if that something is that we move the distribution/ directory back to where it was, the restrictiveness of the old code is not going to do much good, since the distribution directory is now under browser/

Comment 26

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #25)
> since the distribution directory is now under browser/

And that is only true for Firefox desktop, not any other product.

Comment 27

[[e-mail] (:bhearsum)]

Comment on attachment 715041 [details] [diff] [review]
Don't special-case extensions/ and searchplugins/ directories when creating complete and partial mar, but special-case distribution/extensions

(In reply to Mike Hommey [:glandium] from comment #25)
> Until something is decided about bug 842334, and if that something is that
> we move the distribution/ directory back to where it was, the
> restrictiveness of the old code is not going to do much good, since the
> distribution directory is now under browser/

Ah, ok. r=me then.

Comment 28

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac36d8b2489

Comment 29

[bhavana bajaj [:bajaj]]

(In reply to Mike Hommey [:glandium] from comment #28)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac36d8b2489

Considering aurora updates are disabled until Friday, per Bug 842638 , we could already go ahead and land this on aurora which may be very helpful for QA testing/verification needed here.

Comment 30

[Mike Hommey [:glandium]]

(In reply to bhavana bajaj [:bajaj] from comment #29)
> (In reply to Mike Hommey [:glandium] from comment #28)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac36d8b2489
> 
> Considering aurora updates are disabled until Friday, per Bug 842638 , we
> could already go ahead and land this on aurora which may be very helpful for
> QA testing/verification needed here.

I took that as an a+.
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f571fa8ec14

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ac36d8b2489

Comment 32

[Scoobidiver (away)]

Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/8f571fa8ec14

Comment 33

[u279076]

I can at least confirm that this isn't reproducible for me updating the following:
> Firefox Aurora 21.0a2 2013-02-20 to Firefox Aurora 21.0a2 2013-02-22
> Firefox Nightly 21.0a1 2012-02-11 to Firefox Nightly 22.0a1 2012-02-22

Unfortunately I can't confirm that I was able to reproduce the original bug because updates are not available to prove it (ie. updates to a known broken build). As such I'm marking this verified fixed but please reopen if this is still reproducible for you using the latest builds.