96525 - delay loading of strres dll in nsProfile.cpp

Status: VERIFIED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[Frank Tang]

I find this out when I try to address 75041. Somehow the first time we load the
strres.dll is in nsProfile while we try to flush out the stringbundle cache.
This is funny, we load it to ask it to clean up. I think we should not load the
dll while we don't need to.

reproduce procedure in window.
1. download the http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31201
save it as c:/temp/minimal.xul

2. in VC, open file mozilla/intl/strres/src/nsStringBundle.cpp
set a breakpoint at the last line , the line of NS_IMPL_NSGETMODULE
3. in VC, open "Project:Setting, switch from "General" tab to "Debug" tab, while
Category combobox is set to "General" put 
"-chrome file:/c:/temp/minimal.xul" in Program Argument
4. change the Category Combobox to "Addiontional DLLs", dobule click under the
"Local Name" it will show a new row with a checkbox check in the left hand side, 
in the right hand side of the row, you will see a "..." button, click that
button and locate dist/win*/bin/components/strres.dll
5. run the mozilla
it will break at the previous break point, and you will find out the reason it
load the strres.dll into the memory is because of the following line:

1067 if (isSwitch)
1068 {
1069 // Phase 1: See if anybody objects to the profile being changed.
1070 mProfileChangeVetoed = PR_FALSE;        
1071 observerService->Notify(subject,
NS_LITERAL_STRING("profile-approve-change").get(), context.get());
1072 if (mProfileChangeVetoed)
1073 return NS_OK;
1074 1075 // Phase 2: Send the "teardown" notification
1076 observerService->Notify(subject,
NS_LITERAL_STRING("profile-change-teardown").get(), context.get());
1077 1078 // Phase 3: Notify observers of a profile change
1079 observerService->Notify(subject,
NS_LITERAL_STRING("profile-before-change").get(), context.get());        
1080 }
1081 1082 // Flush the stringbundle cache
1083 nsCOMPtr<nsIStringBundleService> bundleService = 
1084 do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
1085 if (NS_SUCCEEDED(rv)) {
1086 rv = bundleService->FlushBundles();
1087 NS_ASSERTION(NS_SUCCEEDED(rv), "failed to flush bundle cache");
1088 }

I talk to dp, and he make a good point, should line 1081-1088 inside the if
(isSwitch) block?
We should not load the nsIStringBundleService and call FlushBundles unless it
have ever been created.

Comment 1

[Conrad Carlen (not reading bugmail)]

Yes. There should be no ref to string bundle as there is here in profile mgr.
The reason this is done (it has been for a long time) is that, on choosing a new
profile, possibly the locale has changed. This has been around for longer then
the profile changeobserver mechanism. See:
http://lxr.mozilla.org/mozilla/source/profile/public/nsIProfileChangeStatus.idl

The string bundle service should be a profile change observer (which it can be
by implementing nsIObserver andregistering for a notification listed in
nsIProfileChangeStatus). Probably either "profile-before-change" or
"profile-after-change"

Comment 2

[Conrad Carlen (not reading bugmail)]

Accepting.

Comment 3

[Conrad Carlen (not reading bugmail)]

-> 0.9.6

Comment 4

[Conrad Carlen (not reading bugmail)]

Attachment: remove ref to strres from profile

Comment 5

[Conrad Carlen (not reading bugmail)]

This will fix it. Since the string bundle service already implemented
nsIObserver and nsISupportsWeakReference, it was pretty simple :-)

CC'ing Alec for sr. Frank - can you r=?

Comment 6

[tao]

Comment on attachment 56688 [details] [diff] [review]
remove ref to strres from profile

looks good. r=tao providing
you verified this patch
does not cause the bundle
flushing needed in switching
from global locale to user
profile locale.

TO verify this, you can
1. download a region pack,
regfr.xpi from http://home.netscape.com/computing/download/languagepacks_62.html
and 
2. select the France region pack and restart.
3. you default home page
should point to our French home page.

Comment 7

[Alec Flett]

Comment on attachment 56688 [details] [diff] [review]
remove ref to strres from profile

nice! sr=alecf

Comment 8

[Conrad Carlen (not reading bugmail)]

tao - I tried your test with NS 6.2 so I could observe the right behavior first
before trying it with my patch. With 6.2, I did this:

1. installed the french langpack and restarted (I couldn't set language/locale
to french until I did)
2. set language and content to french and homepage pref to
http://home.netscape.com/bookmark/6_2/homebutton.html (the default)
3. restarted.

The homepage was the same as U.S. homepage. What could be wrong? Did this work
right in 6.2? Is there another way to test this?

Comment 9

[tao]

>2. set language and content to french and 
Do this.
> homepage pref to http://home.netscape.com/bookmark/6_2/homebutton.html (the
default)
Don't change this. If you manually re-define this, it will be saved into your
profile.

Try this:
1. launch the browser with profile manager
2. create a new profile and set the region to France (French Region Pack).
3. start N6 with this new profile. 
4. the start up page and subsequential home page (clicking on "N" logo) should
   all point to French sites.

Ping me if this still does not work for you.

Comment 10

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.7

Comment 11

[Conrad Carlen (not reading bugmail)]

-> 0.9.8

Comment 12

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.9

Comment 13

[Conrad Carlen (not reading bugmail)]

I was finally able to run Tao's test. I had to make an alternate content region
by hand in order to do so. I changed "browser.startup.homepage" in my new region
to be cnn.com.
1) I made a new profile in the new region and it came up with the right home page.
2) To make sure it was OK with a string which had actually been used before the
region changed, I stuck in some code in the top and bottom of
nsProfile::SetCurrentProfile to get the value of the "browser.startup.homepage"
pref and dump it to the console. It gave me:

before switch: http://www.mozilla.org/start
### nsCacheProfilePrefObserver::Observe [topic=profile-after-change data=startup]
after switch: http://www.cnn.com

With that, I can say it works. I'll check it in.

Comment 14

[tao]

thank you :-)

Comment 15

[Conrad Carlen (not reading bugmail)]

Checked in.

Comment 16

[K'Trina Medina]

Verified code fix