browser should not autoregister components at startup

VERIFIED FIXED in mozilla0.9.3

Status

()

defect
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: waterson, Assigned: cathleennscp)

Tracking

Trunk
mozilla0.9.3
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PDT+)

Attachments

(7 attachments)

Reporter

Description

18 years ago
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

18 years ago
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

18 years ago
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

18 years ago
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.
Assignee

Updated

18 years ago
Blocks: 7251, 46775
Reporter

Updated

18 years ago
Blocks: 81371
Status: NEW → ASSIGNED
Reporter

Comment 4

18 years ago
``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

18 years ago
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.
Reporter

Comment 6

18 years ago
-> cathleen, who Has A Plan!
Assignee: waterson → cathleen
Status: ASSIGNED → NEW
Assignee

Comment 7

18 years ago
Posted patch proposed fixSplinter Review
Assignee

Comment 8

18 years ago
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.
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.3

Comment 9

18 years ago
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
Assignee

Comment 10

18 years ago
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...
Assignee

Comment 11

18 years ago
Assignee

Comment 12

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

Comment 14

18 years ago
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.
Assignee

Comment 15

18 years ago
filed a bug on libreg thread-safe issue... bug 87680
Depends on: 87680
Assignee

Comment 17

18 years ago
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!
Reporter

Comment 18

18 years ago
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)
+

Assignee

Comment 19

18 years ago
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!  :-)
Reporter

Comment 22

18 years ago
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?
Reporter

Comment 24

18 years ago
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.
Assignee

Comment 25

18 years ago
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!!
Assignee

Comment 26

18 years ago
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.
Keywords: nsBranch
Assignee

Comment 27

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

Comment 28

18 years ago
seems quite reasonable. well done old chap.
sr=alecf

Comment 29

18 years ago
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

18 years ago
nope, always need both r= and sr= :)
Assignee

Comment 31

18 years ago
great! thanks guys!
fix checked into trunk.  :-)
Assignee

Updated

18 years ago
Whiteboard: PDT+
Assignee

Comment 32

18 years ago
fix landed to 0.92 branch :-)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

Comment 33

18 years ago
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 :-) )
QA Contact: scc → lchiang
I'll verify this, looks good. Tree no longer autoregisters every time.
Status: RESOLVED → VERIFIED
No longer blocks: 7251

Updated

18 years ago
Blocks: 7251
You need to log in before you can comment on or make changes to this bug.