Closed Bug 841094 Opened 7 years ago Closed 7 years ago

"extensions" directory disappeared after updating to Feb12th Nightly

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox21 + verified

People

(Reporter: vladan, Assigned: glandium)

References

Details

Attachments

(1 file)

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.
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?
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?
Flags: needinfo?(robert.bugzilla)
Flags: needinfo?(nthomas)
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(benjamin)
Flags: needinfo?(rhelmer)
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.
Flags: needinfo?(gavin.sharp)
(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.
Flags: needinfo?(robert.bugzilla)
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 ?
Flags: needinfo?(nthomas)
(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.
(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.
Duplicate of this bug: 841384
Flags: needinfo?(rhelmer)
Flags: needinfo?(benjamin)
Blocks: 841382
Duplicate of this bug: 842072
Now the question is how to deal with partial upgrades from a version that doesn't have the extensions directory.
Attachment #715041 - Flags: review?(robert.bugzilla)
Assignee: nobody → mh+mozilla
(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.
No longer blocks: 841839
Status: NEW → ASSIGNED
Flags: in-testsuite?
Another idea that might work to unscrew nightly users would be to skip generating partial updates for one nightly.
(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.
(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?
(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 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.
Attachment #715041 - Flags: review?(robert.bugzilla) → review+
Flags: needinfo?(kev)
Flags: needinfo?(gavin.sharp)
(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).
Flags: needinfo?(gavin.sharp)
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.
Adding QA to help with verification here once this lands, as a part of 21.0a2 sign-off.
Keywords: qawanted, verifyme
QA Contact: anthony.s.hughes
(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.
Keywords: qawanted
(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 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.
(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/
(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 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.
Attachment #715041 - Flags: checkin+
Blocks: 843073
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/3ac36d8b2489
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kev)
Keywords: verifyme
Depends on: 878419
You need to log in before you can comment on or make changes to this bug.