85770 - browser should not autoregister components at startup

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Chris Waterson]

So, it looks like we _always_ run autoreg at startup, whether we're in an
optimized build or not. The root of the problem is that we call
NS_SetupRegistry(PR_TRUE) here:

  http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/nsAppRunner.cpp#997

changes made by dbragg (in r1.278 of the file), apparently to fix bug 65678. The
PR_TRUE tells |NS_SetupRegistry_1()| to run autoregistration.

dbragg: could you comment on why this is necessary?

Comment 1

[dbragg]

Yes.  This had to be done because of the new install cleanup routine.  dveditz
was going to change this when he landed some other fixes.  I don't know what
those were but it was through discussion with him that I made the change to
always do an autoreg at startup.  Unfortunately Dan is on sabbatical right now
so he can't comment.

Comment 2

[John Bandhauer]

That is a *significant* change. We were seeing this as a measureable and 
unnecessary startup cost. So now some versions of the software will do this. 
Older (and I hope) future versions will not. Confusion ensues :(

Comment 3

[dbragg]

That is correct.  Future versions, including rtm should not.  Dan should be
fixing this when he returns.  Perhaps you should reassign the bug to him.

Comment 4

[Chris Waterson]

``Significant'', for sure. It slows the statically linked build down by about
64%, and actually makes it _slower_ than the dynamic build. Which sucks bigly,
because with autoreg _off_, the static build starts about 30% faster than the
dynamic build:

Startup time, 200MHz PII running Win98
Dynamic build, autoreg always 24s 1.00
Static build, autoreg always 28s 1.17
Static build, no static autoreg 17s 0.71

Since dveditz is on sabbatical, I significantly doubt that he'll return in time
to deal with this for RTM. So it looks like we're going to have to figure this
out ourselves.

dbragg wrote:

> This had to be done because of the new install cleanup routine.

Could you elaborate? What is the ``install cleanup routine''?

> dveditz was going to change this when he landed some other fixes.
> I don't know what those were but it was through discussion with him
> that I made the change to always do an autoreg at startup.

Again, please elaborate. You put the code in to always autoreg; what was the
reason for that, exactly?

Comment 5

[dbragg]

I'll try to elaborate as much as I can but there are some holes in my knowledge
that relate to the registry stuff that Dan was going to address.

First, the new cleanup process:
The cleanup of files with an update installation is a tricky problem.  If the
user is running a version of mozilla and they update to a new version using
mozilla rather than the native install "wizards" there are obviously going to be
files that are in use and can't be replaced right away.  They must be relased
first.  The only way to do that is to kick off a native code cleanup routine
after the program exits.  Before that can be done though a list of files to
delete/replace has to be generated during the update installation phase.  This
list is in the form of a mozilla registry file called xpicleanup.dat.  The
cleanup utility is kicked off at shutdown time.  It reads the xpicleanup.dat
file and tries to delete or replace any files that are in the list.  When it
completes all the entries in the list it deletes xpicleanup.dat and exits.

The old process:
In the old world we did cleanup at startup time which had several drawbacks. 
One of which was that the xpinstall module itself could never be updated but
there were more.  Anyway, the important part about the old method was that it
always ran at startup (a performance hit itself) and determined if an autoreg
was needed.

When I was working on my part of the new cleanup routine I noticed that the
NeedsAutoReg parameter was no longer being set because xpinstall was no longer
being called at start up to determine if an autoreg was needed.  Dan said to
just pass TRUE for now until he got his fixes done and to note that in the code.
 That's why there's a comment above the NS_SetupRegistry_1 call that says "This
call will be replaced by a registry initialization."

Does that help?  I'll see if I can contact Dan too.

Comment 6

[Chris Waterson]

-> cathleen, who Has A Plan!

Comment 7

[Cathleen]

Attachment: proposed fix

Comment 8

[Cathleen]

ssu, syd or dbragg, can you guys help r= my changes in xpinstall?
waterson, can you sr=?

There is an issue where the registry code will generate assertion... I am 
finding out whether if it's a good idea to also change registry to be thread 
safe (sent an email to scc and waterson).  I also got hold of dveditz today, and 
discussed the assertion/threadsafe issue with him.  He said if we can't get 
registry to be threadsafe, the risk of us running an install while someone else 
is calling registry is very slim.  He'd like to get registry fixed to be 
threadsafe, but if not, we'll have to live with it.

Comment 9

[Roger Chickering]

Cathleen,

I know I wasn't asked to review, but I was curious so I looked
and I have some comments:  :-)

I think the nsAppRunner.cpp code would be more robust if you only
had one call to NS_SetupRegistry_1().  You would have a PRBool
variable "needAutoReg":

  PRBool needAutoReg = PR_FALSE

which you would set to PR_TRUE if you find "Autoreg=yes" in the
registry.  Then you call NS_SetupRegistry_1(needAutoReg) after all
the if logic.

Better yet, get rid of NS_SetupRegistry_1() altogether and call
NS_AutoregisterComponents() from inside the if statement and then
call NS_SetupRegistry() after the if statement.

It seems like you should be using XPI_AUTOREG_VAL instead of
"Autoreg" in the nsAppRunner.cpp code.

Thanks!
-Roger

Comment 10

[Cathleen]

actually, I just look at the stuff in nsAppRunner, it's all screwed up.  I was 
doing

rv=some_reg_function()
if (NS_FAILED(rv)) return rv;

rv=some_reg_function()
if (NS_FAILED(rv)) return rv;

then I just changed everything in the last minute, to do

rv=some_reg_funtion()
if (NS_SUCCEEDED(rv)) {

  rv=some_reg_function()
  if (NS_SUCCEEDED(rv)) {
     ...
  }
}

I didn't want to prevent the app from running, and now things just don't look 
right at all.  I'm a LOOSER!!!

working on a new patch...

Comment 11

[Cathleen]

Attachment: much better fix :-)

Comment 12

[Cathleen]

Attachment: newer and better patch

Comment 13

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

How about using an integer to express the boolean-valued flag, rather than
string duping and comparison?

Comment 14

[Cathleen]

so, this autoreg flag as a string set to either "yes" or "no" is a legacy flag 
from xpinstall...  there are actually some other code in xpinstall that will 
check for this flag in the registry.  but, I talked to dbragg on Friday, and he 
said that section of code is no longer in use now (as of a month or so ago, 
initialize xpinstall on startup is been removed) 

I think I'm going to keep the value as is... just in case in future, xpinstall, 
for some reason needs it again.

Comment 15

[Cathleen]

filed a bug on libreg thread-safe issue... bug 87680

Comment 16

[Cathleen]

Attachment: newer patch, nsRegistry nsISupports member function threadsafe, xpinstall to not return if err occurred

Comment 17

[Cathleen]

I need some r= love from xpinstall experts and sr= from waterson

so, one other thing I did in my latest patch, besides what's mentioned above, is 
to also change AddRef and Release for nsInstallProgressDialog from NS_IMPL_ to 
NS_IMPL_THREADSAFE_  to get rid of assertion for not threadsafe.  If anyone from 
xpinstall objects, I'll back that change out!

Comment 18

[Chris Waterson]

Looks pretty good! Maybe instead of hard-coding this logic into nsAppRunner.cpp, 
we could make this check be a helper function (probably a completely inline 
function to avoid linkage headaches, like the stuff in nsNetUtil.h) that's part 
of an XPInstall header?

Here are my nits...

1. In nsRegistry.cpp, it looks like somebody was trying to make the interface 
names line up. Don't bust their spaces!

-NS_IMPL_ISUPPORTS1( nsRegistry,             nsIRegistry      )
+NS_IMPL_THREADSAFE_ISUPPORTS1( nsRegistry,             nsIRegistry      )
 NS_IMPL_ISUPPORTS2( nsRegSubtreeEnumerator, nsIEnumerator,
                     nsIRegistryEnumerator)
 NS_IMPL_ISUPPORTS1( nsRegistryNode,         nsIRegistryNode  )

2. Don't use Hungarian, and declare variables as close as possible to where 
they're used. |sAutoReg| should just be |autoReg|, and should be declared right 
before it's used by |GetStringUTF8|. |bNeedAutoReg| should just be 
|needAutoReg|.

+  char* sAutoReg;
+  PRBool bNeedAutoReg = PR_FALSE;

3. In xpinstall/src/Makefile.in, how come you left this in, but commented out?

 # XXX shouldn't need to export this
-EXPORTS                = nsXPITriggerInfo.h
+# EXPORTS      = nsXPITriggerInfo.h
+EXPORTS = nsSoftwareUpdate.h \
+          nsTopProgressNotifier.h \
+          $(NULL)
+

Comment 19

[Cathleen]

1. okay. I'll remove some spaces so that nsIRegistry lines up with nsIEnumerator

2. okay. I'll change variable names too.

3. okay. I'll remove those lines too.  since someone had already comented that
   we shouldn't export nsXPITriggerInfo.h, and I remember checking with lxr, I
   wasn't able to find anyone else besides xpinstall using it.

Thanks waterson!  :-)

Comment 20

[Cathleen]

Attachment: newer patch, fixed waterson's nits :-)

Comment 21

[Chris Waterson]

Attachment: move ``needs autoreg'' logic into helper function in nsISoftwareUpdate.h

Comment 22

[Chris Waterson]

Hey cathleen, I took what you did and moved the ``do I need to autoreg?'' logic
into an inline helper function in nsISoftwareUpdate.h. That keeps all the
software update logic in one module so that we can re-use it in other
embeddings, if need be. What do you think?

Comment 23

[Chris Waterson]

Attachment: as above, but cleaner, and actually works

Comment 24

[Chris Waterson]

Okay, so the last patch is a bit cleaner (all the registry twiddling is done in
|inline| functions). It's a bit more conservative, and autoregs _unless_ it
finds the XPI key (this handles the case where somebody decides to delete their
component registry). I've also broken up the ``reset'' part, so that (in theory)
we'll only clear the ``needs autoreg'' flag if autoreg completed successfully.

Comment 25

[Cathleen]

thanks waterson for putting the code into inline utilities.  :-)

I tested it and it works great.  I'm building a static optimized build and see 
how it works out.

ssu, dbragg, sgehani, can any one of you review latest patch?  -thanks!!

Comment 26

[Cathleen]

nominating for nsBranch.  
we need this fix to start testing static build, which is going to improve 30% on 
startup and 15-20% on page load speeds.

Comment 27

[Cathleen]

alec or seth, can either of you do a sr=?

Comment 28

[Alec Flett]

seems quite reasonable. well done old chap.
sr=alecf

Comment 29

[Sean Su]

you probably don't need a r= since you already got the sr=, but I'll throw it in 
for good measure: r=ssu

Comment 30

[Alec Flett]

nope, always need both r= and sr= :)

Comment 31

[Cathleen]

great! thanks guys!
fix checked into trunk.  :-)

Comment 32

[Cathleen]

fix landed to 0.92 branch :-)

Comment 33

[lchiang]

not sure how to verify this.  I don't think Scott Collins is the right QA
contact.  Will change it to me for now.  (Unless you want to verify this fix,
Scott :-) )

Comment 34

[Daniel Veditz [:dveditz]]

I'll verify this, looks good. Tree no longer autoregisters every time.