Status

()

Core
Preferences: Backend
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Ulrich Drepper, Assigned: Brian Nesse (gone))

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

16 years ago
Some code in modules/libpref/src still uses strtok.  strtok really must not be
used since it's not thread-safe and not even save to call inbetween sucessive
calls in the same thread.  I'll shortly attach a patch.  The patch also changes
configure.in.  I'll file a few more strtok bugs and some include the same
configure.in change.  So there might be a conflict.
(Reporter)

Comment 1

16 years ago
Created attachment 48995 [details] [diff] [review]
Remove uses of strtok
(Reporter)

Updated

16 years ago
Keywords: patch, review
to prefs backend.
Assignee: sgehani → bnesse
Component: Preferences → Preferences: Backend
(Assignee)

Comment 3

16 years ago
Not that there appears to be anything wrong with the patch but... The fact that
it only addresses one of the five references to strtok in prefapi.c implies to
me that you are trying to fix a specific bug as opposed to general cleanup. Is
that true?

nsIPref, and therefore NextChild(), is depricated. Changing the calling code to
use nsIPrefService and nsIPrefBranch and using GetChildList() would be
preferable to me. This is not to say, however, that I will stand in the way
landing this patch if there is a good reason for it...
(Reporter)

Comment 4

16 years ago
Hum, don't know why the other strtok changes in perfapi are not included. 
Probably I attached the wrong file.  I'll attach an updated, complete patch
(without the configure change which is already in the CVS archive).

And the nsPref.cpp change you cannot leave out until the last user of the code
is gone.
(Reporter)

Comment 5

16 years ago
Created attachment 49354 [details] [diff] [review]
updated, complete patch
(Assignee)

Comment 6

16 years ago
Comment on attachment 49354 [details] [diff] [review]
updated, complete patch

Seems ok to me. r=bnesse.
Attachment #49354 - Flags: review+

Comment 7

16 years ago
sr=alecf
Fix checked into the trunk.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Reopening, since the change to prefapi.c should say |&nextStr| instead of
|nextStr|, and there's also some funky indentation.
really
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 51259 [details] [diff] [review]
proposed additional patch
Is attachment 51259 [details] [diff] [review] right?

Comment 13

16 years ago
Comment on attachment 51259 [details] [diff] [review]
proposed additional patch

oops! bad super-reviewer, no cookie... :)
sr=alecf
Attachment #51259 - Flags: superreview+
(Assignee)

Comment 14

16 years ago
Comment on attachment 51259 [details] [diff] [review]
proposed additional patch

Hmm. oops. r=bnesse.
Attachment #51259 - Flags: review+
Revision to fix checked in 2001-09-28 22:55:53 PDT.  Marking FIXED again.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
rs vrfy
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.