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)
MailNews Core
Profile Migration
Tracking
(Not tracked)
VERIFIED
INVALID
M17
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.
Reporter | ||
Updated•25 years ago
|
Target Milestone: --- → M17
Reporter | ||
Comment 1•25 years ago
|
||
I don't see this as a beta2 stopper yet, but it will get more important if more
people see the side-effect.
Assignee | ||
Comment 2•25 years ago
|
||
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
Reporter | ||
Comment 3•25 years ago
|
||
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 → ---
Assignee | ||
Comment 4•25 years ago
|
||
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 ago → 25 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 5•25 years ago
|
||
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.
Assignee | ||
Comment 7•25 years ago
|
||
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);
Reporter | ||
Comment 9•25 years ago
|
||
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.
Assignee | ||
Comment 10•25 years ago
|
||
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
Reporter | ||
Comment 11•25 years ago
|
||
Thanks for getting to the root of this problem. It's good to know there isn't a
fatal flaw somewhere.
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•