265492 - Packages sometimes accidentally contain compreg.dat, xpti.dat, or chrome.rdf

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P2)

Description

[Benjamin Smedberg]

Sometimes our shipped packages accidentally contain compreg.dat, xpti.dat, or
chrome.rdf, all of which are runtime-generated files that should not be shipped.
This patch simply adds them to the "no-package" list.

Comment 1

[Benjamin Smedberg]

Attachment: Don't package generated files

Comment 2

[Darin Fisher]

but there's a reason for including these files: having them makes the app
startup faster.  what problems are caused by shipping these?

Comment 3

[Adam Hauner]

Darin: AFAIK, if compreg.dat is bundled, TalkBack doesn't start after crash.

Comment 4

[Benjamin Smedberg]

Darin, the build machine may have different runtime parameters than the
installation machine, which means that negotiateauth may be improperly
registered and other similar circumstances. The reason we have "firefox
-register" is so that we can generate these files properly at install-time (at
least the RPMs do this correctly, I believe we got the linux installer to do
this also).

Comment 5

[Darin Fisher]

ah, ok... sorry for the spam.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 162883 [details] [diff] [review]
Don't package generated files

I think it's really just compreg that we have to worry about, but if we
regenerate that we'll be regenerating the others as well, so no need to ship
them.

Comment 7

[Chase Phillips]

Comment on attachment 162883 [details] [diff] [review]
Don't package generated files

This would be good to get checked in for 1.0.1.  Requesting
approval-aviary1.0.1.

Comment 8

[Chase Phillips]

Bug 279993 tracks a problem where compreg.dat in Suite builds causes Talkback
not to start properly.  We should generate a patch for Suite on the trunk and
the 1.7 branch and get that r+sr+a, too.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 162883 [details] [diff] [review]
Don't package generated files

a=dveditz for the 1.0.1 branch

Comment 10

[Chase Phillips]

Landed on Aviary 1.0.1 branch:

Checking in Makefile.in;
/cvsroot/mozilla/browser/installer/Makefile.in,v  <--  Makefile.in
new revision: 1.6.4.5.2.1; previous revision: 1.6.4.5
done

Comment 11

[Jay Patel [:jay]]

Bryner mentioned that there is also an "exclude from package" list for browser
and mail builds in /mozilla/toolkit/mozapps/installer/packager.mk which covers
both Thunderbird and Firefox.  Would it be better to patch that file instead?

Comment 12

[Chase Phillips]

Comment on attachment 162883 [details] [diff] [review]
Don't package generated files

Backed this attachment ouf of aviary 1.0.1.  It should be removed from the list
of approved patches for 1.0.1.

Checking in browser/installer/Makefile.in;
/cvsroot/mozilla/browser/installer/Makefile.in,v  <--  Makefile.in
new revision: 1.6.4.5.2.2; previous revision: 1.6.4.5.2.1
done

Further investigation and discussion suggests this should be handled by the
Talkback portion of the build so that a valid compreg.dat is generated and
distributed with our zip and gzip packaged builds.

Comment 13

[Chase Phillips]

Reopening to undo my previous resolution.

Comment 14

[sairuh (rarely reading bugmail)]

removing fixed-aviary1.0.1 keyword since this was backed out (per comment 12).

Comment 15

[Benjamin Smedberg]

We should not be shipping compreg.dat! The contents of compreg.dat depend on the
configuration of the particular machine in question (whether it has gnome-vfs,
or gdi+, or any number of things that we use dynamic loading of components to
detect).

Comment 16

[Robert Kaiser]

We should check in such a patch at least on trunk for all products, I think.

Comment 17

[Boris Zbarsky [:bzbarsky]]

This is making talkback not work in some of our trunk builds _again_.  Could we
please get this resolved?  I've had a number of trunk crashes in the last few
days, and I have absolutely no data on them... (other than they all seem to be
session history related, based on what I was doing when I crashed).

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Just to confirm: bz: you're saying that this is causing us to not have working
talkback on Firefox?  Recent suspicions have involved the SeaMonkey rebranding,
so that would be a new data point.

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> Just to confirm: bz: you're saying that this is causing us to not have working
> talkback on Firefox?  Recent suspicions have involved the SeaMonkey rebranding,
> so that would be a new data point.

I currently know two things about that recent talkback stuff (which actually was
started off in bug 301000):
1) There's definately some rebranding problem involved (noted in the other bug),
but it might not even be the major problem
2) We have reports that after deleting compreg.dat, it started working for
people, which strongly points to this bug here.

As bsmedberg said, we definately should not ship those files in any of our
products - the talkback thing might be only one one of the problems caused by this.

Comment 20

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I ask again, are you seeing _this_ bug, which is filed against Firefox/Build
Config, and which has largely been about a patch to browser/installer/Makefile.in?

Comment 21

[Benjamin Smedberg]

*this* bug has morphed anyway... it should be a patch to
toolkit/mozapps/installer/packager.mk instead of browser/installer/Makefile.in

Comment 22

[Chase Phillips]

Background: At some point around 1/20 compreg.dat became tainted and needed to
be removed from our builds before Talkback would be invoked at crash.  In an
attempt to fix the problem on 2/15, this patch was landed.  The following day I
put a lot of debugging time in, the compreg.dat generation problem was
discovered and a fix went in soon afterwards.  Once the separate fix went in,
compreg.dat could once again be properly regenerated and included in builds. 
When we/I recognized that we could once again ship a proper compreg.dat, this
patch got the heave-ho to get us back to the steady state we'd been in prior to
~1/15.

I recall at the time that people felt uncomfortable that we were making
compressed builds that required first being run by a user on the system that had
write access to the installation before it could be run by other users.  Did
bryner, darin, ben, or asa feel strongly about this?  I remember consulting with
them on this issue but I can't recall their stated opinions.

I don't dispute that compreg.dat's generation may be broken in other ways based
on the build system's configuration.  My only concern is that we appear to have
been shipping a pre-generated compreg.dat for 6-12 months or longer.

If we've changed our minds on that and don't mind requiring, eg, root to run the
app after unpacking it before any user can run the app, then let's land this
fix.  I'd like to see sign-off on such a decision by somebody such as brendan or
bryner first, though.

Comment 23

[Benjamin Smedberg]

Toolkit applications should work correctly even when compreg.dat/xpti.dat are
not packaged and the user does not have write access to the install dir (on
linux installations IMO there should never be a compreg.dat in the installdir).
The only reason we even have a compreg.dat in the installdir any more is to run
profile migration and the profile manager, but neither of these cases *require*
that compreg.dat be present, it can and should be generated on the fly and does
not need to be saved.

compreg.dat pregeneration is very wrong because the configuration of the
end-user's system may be different than our build machine (GSSAPI not present,
for example), and this affects compreg.dat in non-trivial ways.

I cannot understand what we think is *good* about pre-shipping compreg.dat.

Comment 24

[Chase Phillips]

(In reply to comment #23)
> I cannot understand what we think is *good* about pre-shipping compreg.dat.

You're being devious, Benjamin, as it's obvious you understand at least one good
thing that is caused by our shipping it -- we do not require root on a system to
first run the app after unpacking it so other users may run it.

We have been shipping compreg.dat in our builds for a long time now and the
world has not caught on fire.  It has worked.  To my knowledge we don't find
faulty compreg.dats causing users an inability to use the app.  The only times
I've seen our pre-shipping compreg.dat not work are when the /wrong/ compreg.dat
ends up in the build -- the one that doesn't know about the Talkback component.
 Even that only leads to Talkback not being invoked when the app crashes.

I'll restate: I'm not against us taking this patch as it is obvious to the
component aficionados that the build system configuration affects how
compreg.dat is created.  But, Benjamin, your calling this fire four alarms has
stoked my own curiosity: what bugs are on file due to a mismatch of
configuration between build system and user system and an improperly generated
compreg.dat?

If the build and user systems can differ so dramatically, then changes to the
user system over time can be just as damaging.  This bug is starting to sound
like a cheat to get compreg.dat right just once at first startup and then
abandon the problem from then on.  Shouldn't we be sanity checking the file over
time if it's so important?

Comment 25

[timeless]

chase: if compreg.dat isn't there, we'll register components and keep them in
memory <full stop>.

with firefox, that registration can end up being serialized to a per user
location (or maybe per profile per user, whichever).

with seamonkey, worst case is that you have to pay each time for registration,
and it's not a big deal in contrast to a browser that doesn't function properly.

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

compreg.dat, xpti.dat etc are actually generated and stored in the user's
profile directory, and have been for toolkit applications since Firefox 0.9.
There should be no reason why a user needs root to generate these files. With
1.1, they won't need root to generate extensions.rdf either, since this is now
stored exclusively in the profile dir too. 

Now, it may be that we're shipping bogus compreg.dat files in bin/, but
shouldn't toolkit apps be ignoring those ones and exclusively using the one
generated in the profile?

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

shipping compreg.dat should improve startup performance, right? that does seem
like a good thing about it to me.

That compreg.dat might contain components like GSSAPI does not seem like much of
a problem to me. GetService will still fail.

Comment 28

[Chase Phillips]

(In reply to comment #23)
> Toolkit applications should work correctly even when compreg.dat/xpti.dat are
> not packaged and the user does not have write access to the install dir (on
> linux installations IMO there should never be a compreg.dat in the installdir).
> The only reason we even have a compreg.dat in the installdir any more is to run
> profile migration and the profile manager, but neither of these cases *require*
> that compreg.dat be present, it can and should be generated on the fly and does
> not need to be saved.

bsmedberg, I'll eat crow here as when I reread this later I understand that you
and I are saying pretty much the same thing.  (Mea culpa - I didn't get it on my
first readings.)  I note in your first sentence you say "should" instead of "do"
-- any reason for that?  Also, Ben says this stuff should be in the profile
directory.  It seems the compreg.dat we generate is overriding that.  Who knows?
 If we're not already sure can we doublecheck what we're doing at present?

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #27)
> shipping compreg.dat should improve startup performance, right? that does seem
> like a good thing about it to me.

Just first startup.  After that, if it's regenerated in the profile (right?  Or
is there a suite bug here that's confusing things?), then we have both it and
the XUL FastLoad file cached.  So second through Nth startups are fast.  Who
cares about first startup perf?

/be

Comment 30

[Darin Fisher]

Initial startup performance should be minimally impacted by not having
compreg.dat since most of the components are statically linked into firefox.  My
DPA build has 15 modules listed in compreg.dat (only 5 of those are DLLs).

Has anyone noticed a significant delay before the profile migrator (or profile
selection) dialog is shown?

Comment 31

[Benjamin Smedberg]

Attachment: Don't package compreg, xpti, or seamonkey overlayinfo

Comment 32

[Benjamin Smedberg]

Comment on attachment 190139 [details] [diff] [review]
Don't package compreg, xpti, or seamonkey overlayinfo

Oh well, no second-review flag here... kairo, please pretend that sr means r2

Comment 33

[Robert Kaiser]

Comment on attachment 190139 [details] [diff] [review]
Don't package compreg, xpti, or seamonkey overlayinfo

Looks good to me. second-review=kairo.

Just as notes for others that follow this bug:
1) This extends the NO_PKG_FILES block beginning at
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/installer/packager.mk#135
2) This also fixes SeaMonkey packages as along with rebranding, we changed to
use packager.mk from there as well. We might still have to look into
installers, though.

Comment 34

[Benjamin Smedberg]

Fixed on trunk for 1.8b4