1487228 - Consider adding memory reporting for nsNSSCertList

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I am looking at a DMD report for a content process, and I see 212 allocations with this stack:

    #01: replace_malloc(unsigned long) (DMD.cpp:1267, in libmozglue.dylib)
    #02: PL_ArenaAllocate (plarena.c:127, in libnss3.dylib)
    #03: PORT_ArenaAlloc_Util (secport.c:321, in libnss3.dylib)
    #04: PORT_ArenaZAlloc_Util (secport.c:342, in libnss3.dylib)
    #05: CERT_NewCertList (certdb.c:2484, in libnss3.dylib)
    #06: nsresult mozilla::psm::Constructor<nsNSSCertList, (nsresult (nsNSSCertList::*)())0, (mozilla::psm::ProcessRestriction)1, (mozilla::psm::ThreadRestriction)1>(nsISupports*, nsID const&, void**) (memory:2601, in XUL)

each allocation is 2048 bytes, for a total of almost half a megabyte of cert lists.  I also have 85 allocations of 192 bytes each under CERT_NewCertList via PR_NewLock....

It might be good to memory-report these things.

Comment 1

[Nicholas Nethercote [inactive]]

In general I've avoided memory reporting NSS stuff because I wasn't sure how best to do it. But you are right that it would be worth figuring that out.

Comment 2

[Dana Keeler (she/her) [:keeler]]

These are probably fairly memory-inefficient for our purposes. Each CERTCertList creates a PLArena with a chunksize of 2048 bytes, but we only need space for 3 pointers per certificate in the list. In Gecko, these lists are rarely longer than 3 certificates, although in some instances (e.g. tests or the certificate manager) they can have a few hundred certificates. In many cases there may not be a compelling reason to use CERTCertList, and we could save some memory by avoiding these altogether.

Comment 3

[Dana Keeler (she/her) [:keeler]]

Attachment: Bug 1487228 - (1/2) remove nsIX509CertList.getRawCertList r?jcj

nsIX509CertList.getRawCertList is only used once and doesn't provide
particularly unique functionality (its one use can easily be re-worked in terms
of other APIs). Removing this API will ease refactoring work to avoid holding
long-lived references to CERTCertList instances in nsNSSCertList.

Comment 4

[Dana Keeler (she/her) [:keeler]]

Attachment: Bug 1487228 - (2/2) avoid holding CERTCertList instances long-term in nsNSSCertList r?jcj

Each instance of CERTCertList creates a PLArena with a chunk size of 2048 bytes,
but only needs space for 3 pointers per certificate in the list. The majority of
the time Gecko uses CERTCertList, we'll store ~3 certificates (although in some
cases we do store a few hundred, such as in tests or the certificate manager).
This is fairly inefficient. This patch starts the process of avoiding using
CERTCertList in Gecko by converting nsNSSCertList (i.e. nsIX509CertList) (as
well as nsNSSCertListEnumerator) to use a more efficient data structure to hold
references to certificates long-term. Future follow-up patches could (and
should) update certificate verification APIs in PSM to avoid CERTCertList as
well.

Depends on D5096

Comment 5

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9006733 [details]
Bug 1487228 - (1/2) remove nsIX509CertList.getRawCertList r?jcj

J.C. Jones [:jcj] (he/him) has approved the revision.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9006734 [details]
Bug 1487228 - (2/2) avoid holding CERTCertList instances long-term in nsNSSCertList r?jcj

J.C. Jones [:jcj] (he/him) has approved the revision.

Comment 7

[Dana Keeler (she/her) [:keeler]]

Thanks for the reviews!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57de2331660f141f3c8c1bc00a8ef658ed45d5aa

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b0cc329120c
(1/2) remove nsIX509CertList.getRawCertList r=jcj
https://hg.mozilla.org/integration/autoland/rev/0936ac1e0de3
(2/2) avoid holding CERTCertList instances long-term in nsNSSCertList r=jcj

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9b0cc329120c
https://hg.mozilla.org/mozilla-central/rev/0936ac1e0de3