Closed Bug 426830 Opened 13 years ago Closed 12 years ago

remove unused nick_template_with_num

Categories

(Core Graveyard :: Security: UI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wladow, Assigned: KaiE)

References

()

Details

Attachments

(2 files, 2 obsolete files)

We should remove unused nick_template_with_num added by Bug 77983

Daniel Veditz in the https://bugzilla.mozilla.org/show_bug.cgi?id=426822#c1:

nick_template_with_num doesn't appear to be used. It's loaded from the bundle
into a variable but after that seems forgotten.

mismatched allocators are used here (PR_smprintf vs PR_Free).

Both nickFmt and nickFmtWithNum are leaked.
Boy, function default_nickname is really a hack.
I see several other leaks.
Attached patch Patch v1 (obsolete) — Splinter Review
This change should get careful testing before it gets checked in.
Attachment #313458 - Flags: review?(rrelyea)
The patch used in this bug has some code that casts const away, because NSS non-const parameters.

I'm working on bug 426886 to make that unnecessary.
Depends on: 426686
Depends on: 426886
No longer depends on: 426686
Comment on attachment 313458 [details] [diff] [review]
Patch v1

r-

The new get default nickname has some bugs:

1) The addition of the slotname for non-internal certs does not happen. you set nickname to plainName instead of tmp.

2) even if this were correct, theres a bug in the loop where plainName is used as the base of the numberically added nickname rather than nickname (which may have a slot name added to it).
Attachment #313458 - Flags: review?(rrelyea) → review-
> 1) The addition of the slotname for non-internal certs does not happen. you set
> nickname to plainName instead of tmp.
> 
> 2) even if this were correct, theres a bug in the loop where plainName is used
> as the base of the numberically added nickname rather than nickname (which may
> have a slot name added to it).


Thanks, good catch, I think had missed that the token name must be kept when trying more names in the loop.

To make the code clearer, I renamed "plainName" to be "baseName".

(plainName implied name-of-cert-only, but we really need the basename-for-looping which includes the potential token prefix)

The new patch stores token:name in baseName (if !internal), and updates output nickname candidate to that value.

Inside the loop, baseName gets used for potentially appending numbers.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #313458 - Attachment is obsolete: true
Attachment #323831 - Flags: review?(rrelyea)
The comment after CERT_CompareName is misleading.
It misses the word "not".
It says: let's use this nickname
but the code apparently does "do not use this nickname"

I'll attach a new patch with the comment fixed.

Attached patch Patch v3Splinter Review
Attachment #323831 - Attachment is obsolete: true
Attachment #323832 - Flags: review?(rrelyea)
Attachment #323831 - Flags: review?(rrelyea)
Comment on attachment 323832 [details] [diff] [review]
Patch v3

r+ with some comments and reservations..

comments: The not you added was correct in the original.... If you find a cert with the same nickname, but also has the same subject, go ahead and use it. By freeing the dummy cert it will break out of the loop and use that nickanme (so the code is correct).

reservations:

I think the original code is wrong. The current code preserves this problem: We should issue this check independent of whether or not it's the internal token. Currently the internal token will ignore your attempt to change the label on a cert that already has one associated with it's subject, so there currently is no harm in the code, but it may cause some problems when the new DB starts up.

bob
Attachment #323832 - Flags: review?(rrelyea) → review+
(In reply to comment #10)
> comments: The not you added was correct in the original.... If you find a cert
> with the same nickname, but also has the same subject, go ahead and use it. By
> freeing the dummy cert it will break out of the loop and use that nickanme (so
> the code is correct).

I'm reading your words as:
  it is wrong to add the word "not" to that comment.

After reading the code again, I concur.
I'll undo my change and keep the comment as is.


> I think the original code is wrong. The current code preserves this problem

I'm reading your words as:
  The existing code has a problem.
  The patch doesn't change that.
  This problem should be worked on eventually.
(In reply to comment #9)
> Don't forget to remove the string and its note too:
> http://lxr.mozilla.org/mozilla/source/security/manager/locales/en-US/chrome/pipnss/pipnss.properties#94

Thanks for the reminder!
I've verified that indeed the string is no longer used.
Attached patch Patch v4Splinter Review
This is the patch with the comment reverted and the string (and note) removed.

I'll check this in to mozilla-central.
Checked in to mozilla-central

http://hg.mozilla.org/mozilla-central/index.cgi/rev/c53be3413e41d458039e24b0138220293e115f77

Marking fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.