Closed Bug 40534 Opened 25 years ago Closed 25 years ago

Optional UTF8 conversion of prefs can terminate app

Categories

(MailNews Core :: Profile Migration, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED INVALID

People

(Reporter: selmer, Assigned: sspitzer)

Details

In nsProfile.cpp at line 388, there is a call rv = pPrefConverter->ConvertPrefsToUTF8IfNecessary(); return rv; ConvertPrefsToUTF8IfNecessary looks for a pref called "prefs.converted-to-utf8" and returns an error if it can't find this pref. When unable to find this pref, it should make some assumption and continue, or it should skip conversion and return success. Simply returning the error is incorrect. It seems like the absence of the pref should imply the conversion has not been done, therefore it should be assumed the value is false if the pref can't be retrieved. Alternatively, profiles could ignore the error results from this routine. That would be inelegant unless the above suggestion is not feasible for some reason.
Target Milestone: --- → M17
I don't see this as a beta2 stopper yet, but it will get more important if more people see the side-effect.
what you describe will not happen. I think you are reading the code wrong. first, that pref has a default value of false, see all.js so the call to GetBoolPref will not fail, unless prefs are horked. ConvertPrefsToUTF8IfNecessary() will only return a failure code if: 1) we can't get the pref service 2) we can't get the bool pref "prefs.converted-to-utf8" (has a default, so it should never happen) 3) getting the platform charset return a failure (which should not happen.) if any of those things happen, we really have a problem. the code is correct. marking invalid. adding racham and mscott to the cc list, as I think they were in on the discussion concerning this code and bug that pinkerton fixed / caused.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → INVALID
Wait a moment here. We stepped through this code on Sean Su's machine and the attempt to get the bool pref value did, in fact, return an error and cause the routine to skip processing and return an error. Perhaps the problem is that someone removed the pref from all.js. In any case, catching that error couldn't possibly make the code incorrect and would protect against what happened on Sean's machine.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
I thought, because of pinkerton's bug, "really bad things" were happening with ssu's build. you are basing this on something you saw on a horked build. if you want to spend more time on this, we can step through it on a non-horked build. but let's say I change my code to continue on in this "really bad state". my guess is that you'll either exit soon after (because other code will exit due to prefs failure, because clearly prefs are in a bad state) or, you'll crash. this bug is invalid. before you reopen, talk to me in person.
Status: REOPENED → RESOLVED
Closed: 25 years ago25 years ago
Resolution: --- → INVALID
OK, but we stepped past this by changing the return value to zero and then the main window came up with no problem. It may be that this was the horked build or it may not. Sean, can you comment?
I just pulled pinkerton's fix onto the tree that I was testing bug #40324 yesterday (and restored the call to ConvertPrefsToUTF8IfNecessary() - it was commented out), and it still fails at the call to ConvertPrefsToUTF8IfNecessary(). This only happens on a debug build. Optimized builds run fine.
is the default value for that pref in your all.js? selmer made it sound like your all.js was being altered.
all.js was not altered. I did find the following line in all.js (if that's what you're referring to): pref("prefs.converted-to-utf8",false);
According to the code, that's the right name. Why would it return an error? Seth, the error code was x8000ffff if that means anything to you.
here's the problem. (thanks to ssu for letting me debug on his win98 box.) ssu built on winnt, and brought the build over to win98 to debug a win98 problem. somewhere during that process, all the default prefs files got turned into allcaps. all.js -> ALL.JS the code that snarfs all default prefs files looks for files that end with ".js" ".js" != ".JS" so, we failed to load any default prefs, so GetBoolPref() failed, so we failed. this is such a dead snake. if you think we should alter the code in nsPref.cpp to allow for *.js or *.JS, feel free to open another bug. this code is correct. let's not waste anu more time on this bug. I'm going to mark this verified.
Status: RESOLVED → VERIFIED
Thanks for getting to the root of this problem. It's good to know there isn't a fatal flaw somewhere.
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.