Closed Bug 96525 Opened 23 years ago Closed 23 years ago

delay loading of strres dll in nsProfile.cpp

Categories

(Core Graveyard :: Profile: BackEnd, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: ftang, Assigned: ccarlen)

References

Details

Attachments

(1 file)

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.
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"
Blocks: 75041
No longer blocks: 75041
Accepting.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5
Blocks: 75041
-> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
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 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.
Attachment #56688 - Flags: review+
Comment on attachment 56688 [details] [diff] [review] remove ref to strres from profile nice! sr=alecf
Attachment #56688 - Flags: superreview+
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?
>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.
Mass move to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
-> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Mass move to 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
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.
thank you :-)
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Verified code fix
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: