Status

P2
trivial
ASSIGNED
5 years ago
5 years ago

People

(Reporter: wtc, Assigned: wtc)

Tracking

trunk
3.15.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 812207 [details] [diff] [review]
Fix the "called from ssl3_xxx" comments in ssl3con.c

I am using this bug report to fix function comments that don't warrant their
own bug reports.

The first patch updates the "called from ssl3_xxx" comments in ssl3con.c.
ssl3_HandleServerHelloDone now does the bulk of its work in the new
ssl3_SendClientSecondRound function, so the functions that were called by
ssl3_HandleServerHelloDone directly are now called by ssl3_SendClientSecondRound.
Brian, feel free to reject this patch. (The change to add ssl3_HandleFinished
to the list for ssl3_SendNextProto is worth keeping though.)
Attachment #812207 - Flags: review?(brian)
(Assignee)

Comment 1

5 years ago
Created attachment 812214 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList
Attachment #812214 - Flags: review?(ryan.sleevi)

Comment 2

5 years ago
Comment on attachment 812214 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList

Review of attachment 812214 [details] [diff] [review]:
-----------------------------------------------------------------

r- on styleistic nits, but r+ for adding documentation.

::: lib/certdb/cert.h
@@ +1226,5 @@
>  void
>  CERT_RemoveCertListNode(CERTCertListNode *node);
>  
> +/*
> + * The new cert list node takes ownership of |cert|. |cert| is freed

The use of |cert| is a Chromium-ism.

In NSS, "cert" is used (see lines 106-209, 116-117, 123-124, etc)

@@ +1233,5 @@
>  SECStatus
>  CERT_AddCertToListTail(CERTCertList *certs, CERTCertificate *cert);
>  
> +/*
> + * The new cert list node takes ownership of |cert|. |cert| is freed

Same here

@@ +1240,5 @@
>  SECStatus
>  CERT_AddCertToListHead(CERTCertList *certs, CERTCertificate *cert);
>  
>  SECStatus
>  CERT_AddCertToListTailWithData(CERTCertList *certs, CERTCertificate *cert,

Seems like you should document this
Attachment #812214 - Flags: review?(ryan.sleevi) → review-
Attachment #812207 - Flags: review?(brian) → review+
(Assignee)

Comment 3

5 years ago
Comment on attachment 812207 [details] [diff] [review]
Fix the "called from ssl3_xxx" comments in ssl3con.c

https://hg.mozilla.org/projects/nss/rev/741d64d4656c
Attachment #812207 - Flags: checked-in+
(Assignee)

Comment 4

5 years ago
Created attachment 815120 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList, v2

Ryan: please review the new patch. Thanks.

I remember seeing the use of |cert| in Mozilla code and Bugzilla comments
before the Chromium project existed. It has crept into the NSS source tree
because many NSS developers are familiar with this notation from either
Mozilla or Chromium. I think it is a good notation.
Attachment #812214 - Attachment is obsolete: true
Attachment #815120 - Flags: review?(ryan.sleevi)

Updated

5 years ago
Attachment #815120 - Flags: review?(ryan.sleevi) → review+
(Assignee)

Comment 5

5 years ago
Comment on attachment 815120 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList, v2

https://hg.mozilla.org/projects/nss/rev/41112f489386
Attachment #815120 - Flags: checked-in+
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
You need to log in before you can comment on or make changes to this bug.