Wallet and CookieService should be lazily initialized

VERIFIED FIXED

Status

()

defect
P4
normal
VERIFIED FIXED
20 years ago
13 years ago

People

(Reporter: sfraser_bugs, Assigned: morse)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Currently, the cookie service and wallet components are being initialized in
apprunner main1(). This is not the correct place to do this; these modules should
be lazily initialized the first time they are used. This will help startup
performance.
Reporter

Updated

20 years ago
Blocks: 9147
Assignee

Updated

20 years ago
Status: NEW → ASSIGNED
Target Milestone: M11
Assignee

Comment 1

20 years ago
But we can't do that.  Every time a page is visited, the sigle-signon code needs
to be called to check to see if it is a page that it can prefill with
username/password.  This can happen only if the wallet component is initialized
when the browser starts up.

The same applies to the cookie component since we need to check for cookies
every time we make an http request.

Can you quantify how much time we are adding to the startup performance?  If it
is in the matter of micro or milli seconds, then we shouldn't even consider
making any changes.  If it is in the order of seconds, that's a different story.
Reporter

Comment 2

20 years ago
The first thing that needs to happen is that the code to init these things should
be moved out of main1(), for maintenance and readability issues.

Second, you should init these things the first time you need them. The user might
not be opening a web page first; they might open mailnews, or the address book.
Assignee

Comment 3

20 years ago
OK, you made a good point.  Where is the correct place to put such
initializations?
Reporter

Comment 4

20 years ago
Without knowning more about the pattern of use, I can't say. Talk to necko or
layout people to figure this out.
Shaver talked to me about this. Appshell components startup could be used for
this.

Talk to me about this and I can explain more.
Assignee

Comment 6

20 years ago
I did talk to dp and we exchanged some e-mail on this topic.  I'm putting the
hilites of our discussion in this bug report so that we don't lose the thread.

FIRST DP SAID:

> If you put your hierarchy in the a particular spot in the registry, then
> on startup, your component will be created. These are currently called
> appshell components as these are a function of the app.

> xpinstall and one other appshell component does this. So instead of
> adding code to apprunner to just trigger a creation on startup, you can
> add this little code that writes your cid into the registry. And you
> would need to implement nsIAppshellComponent I think.

> You can look at xpinstall/src/nsSoftwareUpdate.cpp for the code that
> adds itself to the registry.

> This is post beta stuff clearly.

MY REPLY TO DP:

> Is this really post-beta?  I thought that this bug was being filed as a
> performance issue so they could improve startup time.

DP REPLIED BACK:

> I dont see how this would improve startup. Eitherway your dll will
> get loaded at startup. Maybe I misread the bug...

AND I ANSWERED:

> I was against this at first and my initial comments in the bug report so
> indicate.  But then sfraser added the following comment which made me
> change my mind.
>
>> Second, you should init these things the first time you need them. The
>> user might not be opening a web page first; they might open mailnews,
>> or the address book.
>
> So it is a performance issue -- the dll should not get loaded if all you
> are doing is going to mailnews.  Will the scheme you described avoid
> loading the dll if you never open the browser?  If so, then your scheme
> is the wrong solution.

AND DP ACKNOWLEDGED WITH:

> Since appshell are needed to display mail also, my guess is it will get
> loaded. So yes, wrong solution.
AND dp finally said:

Maybe you can invent the right solution. Other components using the old solution
may also be doing the wrong thing. They can be switched over too to your
solution.

Updated

20 years ago
Whiteboard: [Perf]
Assignee

Updated

20 years ago
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee

Comment 8

20 years ago
Checked in new version of nsAppRunner.cpp (and its associated makefile) to fix
this problem).
Reporter

Comment 9

20 years ago
could you explain how they get initialized now?
Assignee

Comment 10

20 years ago
Beats me!  But they are getting initialized because everything is working.  So
apparently that code in nsAppRunner was doing nothing more than loading the
dll's too early.
Assignee

Updated

20 years ago
Status: RESOLVED → REOPENED
Assignee

Updated

20 years ago
Status: REOPENED → ASSIGNED
Assignee

Updated

20 years ago
Resolution: FIXED → ---
Assignee

Comment 11

20 years ago
What was I ever thinking.  With this change the dll's were not getting loaded.
This resulted in bug report 14721.  So I had to back out this change to fix that
bug.  Reopening this report.

Updated

20 years ago
QA Contact: tever → paulmac

Comment 12

20 years ago
actually steve meant bug 15721
Assignee

Comment 13

20 years ago
See some discussion currently going on in bug 17120 which is relevant to this
bug.
Assignee

Updated

20 years ago
Target Milestone: M11 → M12
Assignee

Comment 14

20 years ago
Not going to make it for M11.  Pushing to M12

Updated

20 years ago
Blocks: 17907
Assignee

Comment 15

20 years ago
*** Bug 18352 has been marked as a duplicate of this bug. ***

Updated

20 years ago
Blocks: 18352

Updated

20 years ago
Blocks: 18471

Updated

20 years ago
Blocks: 18951
Assignee

Comment 16

20 years ago
Reassigning to dp at his request
Assignee

Updated

20 years ago
Assignee: morse → dp
Status: ASSIGNED → NEW

Updated

20 years ago
Status: NEW → ASSIGNED

Updated

20 years ago
Blocks: 20203
I have gotten the nsCookieHTTPNotify object delinked from the internals of
cookieservice.

Moving that object out into apprunner and creating that instead of the service
will delay the load of cookie dll.

Updated

20 years ago
Target Milestone: M12 → M13

Updated

20 years ago
Keywords: perf

Comment 18

20 years ago
Bulk add of "perf" to new keyword field.  This will replace the [PERF] we were
using in the Status Summary field.

Updated

20 years ago
Target Milestone: M13 → M14
Moving to M14 since this requires a change to jsloader for getting a principal.

The way we are fixing this is to move the cookie code into necko.
This needs jband to fix nscategory enumeration. Pushing out to M15.

Comment 21

20 years ago
On behalf of PorkJockeys: putting on beta1 radar, per beta criteria priority #2 
- startup performance is not within beta metrics. removing extraneous tags, cc 
waterson
Keywords: beta1
Summary: [Perf] Wallet and CookieService should be lazily initialized → Wallet and CookieService should be lazily initialized
Whiteboard: [Perf]
I would love to fix this. But this aint beta. User benefit of performance is 
not loading 1 dll and that too only if we start navigator in mail mode say. For 
navigator this aint a big major win any more than delaying load of a dll.

Comment 23

20 years ago
Putting on PDT- radar for beta1.  Will not hold beta for this bug.
Whiteboard: [PDT-]
Unfortunately I already fixed this. Code fix. Maybe simon or morse can verify
this.

Cookie get created on first http request using category manager.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED

Updated

20 years ago
QA Contact: paulmac → tever
Reporter

Comment 25

20 years ago
Reopening. We are still initializing the wallet service in main1():

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

This should happen "sometime later", like when wallet is first used.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Comment 26

20 years ago
Reassign to wallet owner
Assignee: dp → morse
Status: REOPENED → NEW
Assignee

Updated

20 years ago
Status: NEW → ASSIGNED
Target Milestone: M14 → M16
Assignee

Updated

19 years ago
Target Milestone: M16 → M17
Assignee

Comment 27

19 years ago
*** Bug 38611 has been marked as a duplicate of this bug. ***

Updated

19 years ago
Target Milestone: M17 → M18

Comment 28

19 years ago
Putting on [nsbeta2-] radar.  Adding relnote keyword. Putting on nsbeta3 radar 
so we can get done for PR3.
Keywords: nsbeta3, relnote
Whiteboard: [PDT-] → [nsbeta2-][PDT-]
Assignee

Comment 29

19 years ago
Pushing this post beta2
Target Milestone: M18 → M20

Updated

19 years ago
No longer blocks: 17907

Updated

19 years ago
No longer blocks: 18471

Updated

19 years ago
No longer blocks: 18951

Updated

19 years ago
No longer blocks: 20203
Adding nsbeta2 keyword to bugs with nsbeta2 triage value in status field so the 
queries don't get screwed up
Keywords: nsbeta2
Doesn't seem worth the effort just to save one library's worth of loading in 
the rare case of someone doing mail-only. Open to ideas, but giving [nsbeta3-]
Whiteboard: [nsbeta2-][PDT-] → [nsbeta2-][PDT-][nsbeta3-]
Assignee

Comment 32

19 years ago
*** Bug 50254 has been marked as a duplicate of this bug. ***
Assignee

Updated

19 years ago
Target Milestone: M20 → M25
M25 is meaningless -- how about a mozilla0.9 or mozilla1.0 setting, per the
http://www.mozilla.org/roadmap.html?

/be
Assignee

Updated

19 years ago
Summary: Wallet and CookieService should be lazily initialized → [y]Wallet and CookieService should be lazily initialized
Target Milestone: M25 → ---
Assignee

Updated

19 years ago
Summary: [y]Wallet and CookieService should be lazily initialized → Wallet and CookieService should be lazily initialized
Whiteboard: [nsbeta2-][PDT-][nsbeta3-] → [y][nsbeta2-][PDT-][nsbeta3-]
Netscape nav triage team: based on Steve Morse's pre-triage recommendation, this 
is not a beta stopper.
Keywords: beta1nsbeta1-
Assignee

Updated

19 years ago
Whiteboard: [y][nsbeta2-][PDT-][nsbeta3-] → [nsbeta2-][PDT-][nsbeta3-]
Assignee

Updated

19 years ago
Target Milestone: --- → Future
Keywords: nsbeta2, nsbeta3helpwanted
Whiteboard: [nsbeta2-][PDT-][nsbeta3-]
Nominating for mozilla1.0 -- should helpwanted be set too?

/be
Keywords: mozilla1.0

Updated

19 years ago
Blocks: 7251

Updated

19 years ago
Blocks: 71373

Updated

19 years ago
Blocks: 71781

Updated

19 years ago
No longer blocks: 7251

Updated

19 years ago
No longer blocks: 71373

Comment 36

18 years ago
So, if I read the comments correctly, the CookieService part of this is fixed
and now only Wallet remains?  If that's true, please update the summary.

We are now trying to cut every little piece of time we can out of startup. 
Loading DLLs is one of the biggest prices we pay at startup and fixing this bug
could help.  Can we get this before beta?
Assignee

Comment 37

18 years ago
This is currently an nsbeta1- future bug.  That doesn't sound like a beta bug to 
me.

Comment 38

18 years ago
Renominating.
OS: Mac System 8.5 → All

Comment 39

18 years ago
nav triage team:

We don't believe that the instantiation for wallet is that much time. Also, you 
need to initialize it before you load a page anyway, so we don't think it will 
affect startup performance much.

However, initialization of wallet shouldn't be in main1().
Keywords: nsbeta1nsbeta1-
renominating, we need to talk about this as per today's perf meeting 
discussion.
Keywords: nsbeta1-nsbeta1
moving to mozilla0.9.4 as per discussion in perf meeting. I'll talk to Steve 
about this so that we can scope this out or bury it. 
Keywords: nsbeta1nsbeta1+
Target Milestone: Future → mozilla0.9.4

Comment 42

18 years ago
nav triage team:

Don't think we'll have time to get around to this for 0.9.4. Pushing out to
mozilla1.0.1 to give us time to nail it once and for all (we hope).
Target Milestone: mozilla0.9.4 → mozilla1.0.1
I don't think we should ship mozilla 1.0 without better decoupling of the
extensions/ stuff.  It would be a disservice to many of our downstream code
consumers, especially in the embedding space.

(mozilla1.0 is already in the keywords, but then so is nsbeta1+...)
I will take this bug. My plan is to load the wallet dll on first form. That
should delay loading of the dll up until the first page is parsed (assuming the
first page usually has a form)
Assignee: morse → dp
Status: ASSIGNED → NEW
Ccing morse - module owner of wallet.

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0.1 → mozilla0.9.5
r=gagan

Steve, can you look at the patch too. Better yet try it out. I need to test it
throughly. I tried the password manager senarios. But didn't know enough to try
the form manager.

NOTES: When the category enumeration fails, should we retry. I am leaning
towards no as we could end up trying multiple times for no avail. Let me know if
people feel otherwise.
Assignee

Comment 48

18 years ago
r=morse for the changes in nsWalletService
Assignee

Comment 49

18 years ago
Of course my r=morse applies to nsWalletFactory as well.

I just tried to apply the patch and it did not compile.  Probably because my 
tree is about a week old.  I'll pull a new tree and try again.
Assignee

Comment 50

18 years ago
Patch cannot be automatically applied to latest tree because of recent changes 
to nsHttpHandler.cpp.  But I was able to manually apply the patch for that file.

With the patch, all functions of form manager and password manager still appear 
to be working.
Alright! thanks Steve. I will get super review and start the checkin process.
cc'ing terri, who's the qa person for the passwd and form mgrs.

Comment 53

18 years ago
sr=jband.

nits...

NS_CreateServicesFromCategory - sigh, yet another linkage point to xpcom.

+        nsCOMPtr<nsISupports> instance = do_GetService(contractID, &rv);
+        if (NS_FAILED(rv))
+            nFailed++;

Why not a continue like the other similar blocks above this? I realize the the
do_QueryInterface that follows will just fail nicely, but it is unnecessary.
Plus, a good optimizer will just share code with the identical blocks above.

Just curious... Any chance of checking a pref and skipping all this for people
who don't want to use the feature at all?

Comment 54

18 years ago
cc'ing chak. this createservicesfromcategory things smells like some application
startup stuff he banged out. chak, can/should the stuff you did be applied here,
or am I off in the weeds?

Comment 55

18 years ago
adding valeski to see if there is any embedding issues with this.
Replied to jband on the concerns via email. For the record,
a) Accepted on the "continue". I will add it in while checking it in.
b) Pref is not the right way. Plus the whole idea is to delay loading of this
dll when the pref is set to true.
c) Yeah another symbol exported from xpcom. Atleast this is not changing
nsIServiceManager
a=asa on behalf of drivers
Fix checked into 0.9.4 for delaying wallet dll when navigator is started up.

Reassigning bug to Steve (owner of wallet) for using similar mechanism on mail,
ftp, http passwords.
Assignee: dp → morse
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.5 → mozilla1.0
No longer a perf issue. This is purely a code cleanup issue.
No longer blocks: 71781
Keywords: perf
Assignee

Updated

18 years ago
Target Milestone: mozilla1.0 → mozilla1.0.1
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED
Assignee

Updated

17 years ago
Target Milestone: mozilla1.0.1 → mozilla1.1alpha
Assignee

Updated

17 years ago
Priority: P3 → P4
Target Milestone: mozilla1.1alpha → mozilla1.2beta
Assignee

Updated

17 years ago
Target Milestone: mozilla1.2beta → mozilla1.3beta
Target Milestone: mozilla1.3beta → ---
after reading through this, I don't think its clear what the remaining parts of
this bug are that need to be fixed.  If its just code cleanup, a clean report
specifying what needs to be fixed still should be filed as a followup in the
appropriate component (which I'm pretty sure isn't Cookies if you read the
entire bug).

Marking fixed based on the comments throughout as well as the fact that the bug
as filed is now fixed.  If there is followup work to be done, please file a
followup and note the new bug here.  Thanks! 
Status: ASSIGNED → RESOLVED
Closed: 20 years ago15 years ago
QA Contact: tever → cookieqa
Resolution: --- → FIXED

Comment 61

13 years ago
V/fixed.
Status: RESOLVED → VERIFIED
Keywords: helpwanted
QA Contact: networking.cookies → benc
You need to log in before you can comment on or make changes to this bug.