1211568 - Upgrade Firefox 44 to NSS 3.21

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert [:KaiE:]]

Upgrade Firefox 44 to NSS 3.21

Comment 1

[Martin Thomson [:mt:]]

r=me

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c288fb0952fb

Comment 3

[Kai Engert [:KaiE:]]

Landed Beta2.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=15206192&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf908d9dcb6

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc991cc398a

Comment 6

[Kai Engert [:KaiE:]]

clearing needinfo, Martin worked on the fix.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5dc991cc398a

Comment 8

[Kartikaya Gupta (email:[e-mail])]

This change seems to introduce some spew when building a desktop linux build, from the line at [1]. The spew isn't bad but it looks like a bug because the condition is check for non-Android platforms ("ifneq" a few lines up) and the message output says that it is an Android platform.

[1] http://hg.mozilla.org/mozilla-central/diff/5dc991cc398a/security/nss/coreconf/Linux.mk#l1.85

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> This change seems to introduce some spew when building a desktop linux
> build, from the line at [1]. The spew isn't bad but it looks like a bug
> because the condition is check for non-Android platforms ("ifneq" a few
> lines up) and the message output says that it is an Android platform.
> 
> [1]
> http://hg.mozilla.org/mozilla-central/diff/5dc991cc398a/security/nss/
> coreconf/Linux.mk#l1.85

:mt, it looks like you checked that line in as part of bug 1216318. Was this the intended behavior? Is the error message just wrong?

Comment 10

[Daniel Holbert [:dholbert]]

Specifically, the build-spew looks like:
 0:15.48 ../../coreconf/Linux.mk:172: !!! OS_TARGET is Android, disabling -Werror

I get 6 copies of this line on a no-op "./mach build binaries" build, when building Firefox for 64-bit desktop linux.

Comment 11

[Nicholas Nethercote [inactive]]

This change appears to have significantly slowed down |mach build binaries| for me. A no-op build with |mach build binaries| used to take ~2.5 seconds on my Linux box. It now takes ~7.8 seconds, and it's all while outputting these lines:

>  0:03.12 ../coreconf/Linux.mk:172: !!! OS_TARGET is Android, disabling -Werror
>  0:03.20 cd util; /usr/bin/make libs
>  0:03.30 cd freebl; /usr/bin/make libs
>  0:03.52 cd dbm; /usr/bin/make libs
>  0:03.62 cd include; /usr/bin/make libs
>  0:03.72 cd src; /usr/bin/make libs
>  0:03.82 cd softoken; /usr/bin/make libs
>  0:03.92 cd legacydb; /usr/bin/make libs
>  0:04.01 cd base; /usr/bin/make libs
>  0:04.12 cd dev; /usr/bin/make libs
>  0:04.22 cd pki; /usr/bin/make libs
>  0:04.32 cd libpkix; /usr/bin/make libs
>  0:04.41 cd include; /usr/bin/make libs
>  0:04.51 cd pkix; /usr/bin/make libs
>  0:04.61 cd certsel; /usr/bin/make libs
>  0:04.71 cd crlsel; /usr/bin/make libs
>  0:04.81 cd checker; /usr/bin/make libs
>  0:04.91 cd params; /usr/bin/make libs
>  0:05.00 cd results; /usr/bin/make libs
>  0:05.10 cd store; /usr/bin/make libs
>  0:05.20 cd top; /usr/bin/make libs
>  0:05.30 cd util; /usr/bin/make libs
>  0:05.40 cd pkix_pl_nss; /usr/bin/make libs
>  0:05.49 cd pki; /usr/bin/make libs
>  0:05.59 cd system; /usr/bin/make libs
>  0:05.69 cd module; /usr/bin/make libs
>  0:05.80 cd certdb; /usr/bin/make libs
>  0:05.90 cd certhigh; /usr/bin/make libs
>  0:05.99 cd pk11wrap; /usr/bin/make libs
>  0:06.09 cd cryptohi; /usr/bin/make libs
>  0:06.19 cd nss; /usr/bin/make libs
>  0:08.00 cd ssl; /usr/bin/make libs
>  0:08.11 cd pkcs7; /usr/bin/make libs
>  0:08.21 cd pkcs12; /usr/bin/make libs
>  0:08.31 cd smime; /usr/bin/make libs
>  0:08.59 cd crmf; /usr/bin/make libs
>  0:08.69 cd jar; /usr/bin/make libs
>  0:08.79 cd ckfw; /usr/bin/make libs
>  0:08.89 cd builtins; /usr/bin/make libs
>  0:09.01 ../../coreconf/Linux.mk:172: !!! OS_TARGET is Android, disabling -Werror
>  0:09.10 ../../coreconf/Linux.mk:172: !!! OS_TARGET is Android, disabling -Werror
>  0:09.18 cd mangle; /usr/bin/make libs
>  0:09.30 ../../coreconf/Linux.mk:172: !!! OS_TARGET is Android, disabling -Werror
>  0:09.39 ../../coreconf/Linux.mk:172: !!! OS_TARGET is Android, disabling -Werror
>  0:09.49 ../../coreconf/Linux.mk:172: !!! OS_TARGET is Android, disabling -Werror

which glandium tells me are NSS. This part used to take ~0.5 seconds, it's now 5 or 6 seconds.

Comment 12

[Daniel Holbert [:dholbert]]

(I'm guessing njn's hardware is pretty fast.  On my few-years-old desktop machine, a no-op "./mach build binaries" spends 10 seconds churning through the section that njn highlighted, with ~3.5 seconds elapsing just between the adjacent "cd nss" and "cd ssl" lines.)

Comment 13

[Martin Thomson [:mt:]]

Ugh, this isn't right.  I clearly inverted the logic.  I'll fix that.

I assume that everyone here is using --enable-warnings-as-errors.  What concerns me most is the fact that this is taking so much longer.  That might be due to folks using clang, for which I had to add some nasty hacks.  I can reduce the performance impact of that a little, I think, but I have a hard time reconciling the changes with a 5 second slowdown.

Comment 14

[Nicholas Nethercote [inactive]]

I'm using clang and --enable-warnings-as-errors, but I just tried disabling --enable-warnings-as-errors and it's still slow. So I think that's a red herring.

Comment 15

[Martin Thomson [:mt:]]

It's likely that it's using clang that is causing the slowdown.

Comment 16

[Martin Thomson [:mt:]]

Attachment: faster.patch

Does this make things better?  There are other calls that I could optimize out, and I'll add a followup that is even more aggressive.

Comment 17

[Martin Thomson [:mt:]]

Attachment: fasterfaster.diff

This might be even faster, but it's a bigger change.

Comment 18

[Nicholas Nethercote [inactive]]

clang times:
- no patch:     7.8s
- first patch:  3.4s
- second patch: 2.4s

gcc times:
- no patch:     5.5s
- first patch:  2.4s
- second patch: 2.4s

Interesting -- I'm used to clang being significantly faster than gcc.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8679208 [details] [diff] [review]
faster.patch

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

As per comment 18.

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8679210 [details] [diff] [review]
fasterfaster.diff

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

As per comment 18.

Comment 21

[Martin Thomson [:mt:]]

Comment on attachment 8679210 [details] [diff] [review]
fasterfaster.diff

I think that we should land this.  It won't completely fix the problem with performance that firefox builds are seeing as a result of this, but those few seconds are pretty important.  If you prefer the other patch, the difference isn't huge between the two, so I'd be OK with landing that instead.

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8679210 [details] [diff] [review]
fasterfaster.diff

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

First and foremost, the same kind of thing was done in Darwin.mk (with different variables names), and should be fixed as well.

The exports you add would work for standalone NSS, but will fail to do anything useful for Gecko builds, which don't recurse through the NSS build system such that those exports would do something useful.

Small digression, but I wonder if the NSS build system shouldn't be owned by the Build Config module (meaning changes to those coreconf makefiles should be reviewed by Build Config peers), although that's not something I'd look forward to, considering the horror show the NSS build system is. Which brings me to a second digression, that I'm /very/ tempted to just stop relying on the NSS build system for Gecko builds.

::: security/nss/coreconf/Linux.mk
@@ +132,5 @@
>  endif
>  
>  ifndef COMPILER_TAG
> +COMPILER_TAG := _$(shell $(CC) -? 2>&1 >/dev/null | sed -e 's/:.*//;1q')
> +CCC_COMPILER_TAG := _$(shell $(CCC) -? 2>&1 >/dev/null | sed -e 's/:.*//;1q')

CCC_COMPILER_TAG is never used. It's a waste of time to set it.

@@ +155,5 @@
> +    # -Qunused-arguments : clang objects to arguments that it doesn't understand
> +    #    and fixing this would require rearchitecture
> +    WARNING_CFLAGS += -Qunused-arguments
> +    # -Wno-parentheses-equality : because clang warns about macro expansions
> +    OS_CFLAGS += $(call disable_warning,parentheses-equality)

You're using -Qunused-arguments, why do you care to test that the individual options are selected depending on whether clang supports them?

@@ +181,5 @@
> +      else
> +        NSS_ENABLE_WERROR := $(shell \
> +          [ `$(CC) -dumpversion | cut -f 1 -d . -` -eq 4 -a \
> +            `$(CC) -dumpversion | cut -f 2 -d . -` -ge 8 -o \
> +            `$(CC) -dumpversion | cut -f 1 -d . -` -ge 5 ] && \

Each directory is going to do this test, still because NSS_ENABLE_WERROR is not exported. And you don't need to execute the compiler three times to do this test. You can either use awk to avoid the triple cut, or use an intermediate variable to store the output of $(CC) -dumpversion and cut that.

Comment 23

[Martin Thomson [:mt:]]

> You're using -Qunused-arguments, why do you care to test that the individual options are selected depending on whether clang supports them?

Because even with -Qunused-arguments, an unknown -Wno-foo argument still generates a fatal error.

I'm personally OK with someone else (presumably someone who knows what they are doing) looking after the build system here, since I do not have the time to do more than the minimal possible thing.

Comment 24

[Martin Thomson [:mt:]]

Attachment: faster2.patch

This shaves 6 seconds off a no-op NSS build on my machine.  I don't expect a similar gain on Firefox builds, but I did what I could.  I made the changes you suggested.

Note that NSS_ENABLE_WERROR doesn't need to be exported because that is all guarded by the ifndef WARNING_CFLAGS, which are exported.

Comment 25

[Nicholas Nethercote [inactive]]

(In reply to Martin Thomson [:mt:] from comment #24)
> Created attachment 8680974 [details] [diff] [review]
> faster2.patch

I tried this one (I had to apply it on top of fasterfaster.diff, I assume this is expected?) and it got no-op |mach build binaries| builds back down from 7.8s to 2.3s, where they were before this bug landed.

Comment 26

[Wan-Teh Chang]

Comment on attachment 8680974 [details] [diff] [review]
faster2.patch

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

::: coreconf/Darwin.mk
@@ +119,5 @@
> +        NSS_ENABLE_WERROR = 0
> +      endif
> +    endif
> +  endif # ndef NSS_ENABLE_WERROR
> +  

Nit: delete the spaces on line 89 and this line.

::: coreconf/Linux.mk
@@ +176,5 @@
>        ifeq ($(COMPILER_TAG),_clang)
>          # Clang reports its version as an older gcc, but it's OK
>          NSS_ENABLE_WERROR = 1
>        else
> +        CC_VERSION = $(subst ., ,$(shell $(CC) -dumpversion))

Should we use := here?

@@ +184,5 @@
> +        ifeq ($(word 1,$(CC_VERSION)).$(word 2,$(CC_VERSION)),4.9)
> +          NSS_ENABLE_WERROR = 1
> +        endif
> +        ifndef NSS_ENABLE_WERROR
> +          NSS_ENABLE_WERROR = $(shell [ $(word 1,$(CC_VERSION)) -ge 5 ] \

Should we use := here?

Comment 27

[Martin Thomson [:mt:]]

Attachment: faster3.patch

I merged the two patches (sorry :njn) and made a few more performance tweaks, as suggested by Wan-Teh.

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8681013 [details] [diff] [review]
faster3.patch

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

::: coreconf/Darwin.mk
@@ +111,5 @@
> +        NSS_ENABLE_WERROR = 1
> +      endif
> +      ifndef NSS_ENABLE_WERROR
> +        NSS_ENABLE_WERROR := $(shell [ $(word 1,$(CC_VERSION)) -ge 5 ] \
> +                                      && echo 1 || echo 0)

This can be written as
ifeq (,$(filter 0 1 2 3 4,$(word 1,$(CC_VERSION))))
  NSS_ENABLE_WERROR = 1
endif

which avoids invoking the shell here.

::: coreconf/Linux.mk
@@ +132,5 @@
>  endif
>  
>  ifndef COMPILER_TAG
> +COMPILER_TAG := _$(shell $(CC) -? 2>&1 >/dev/null | sed -e 's/:.*//;1q')
> +export COMPILER_TAG

It would be better if this were handled the same way on both Darwin and Linux. With the same variable name, and within the same context (on Darwin, this is all in the ifndef WARNING_CFLAGS, not on Linux ; I think the Darwin approach is fine)

That said, it might even be better to move it all to a separate .mk file (Werror.mk?) and include it here.

Comment 29

[Martin Thomson [:mt:]]

Attachment: faster4.patch

I moved the warnings setup to a new file.  Took the other suggestions as well.  I didn't think of using $(filter), which is neat.

Comment 30

[Daniel Holbert [:dholbert]]

(Would it be worth spinning off a new bug for the not-yet-landed patch here, from a bug/version-tracking perspective?  The original patches landed ~2 weeks ago (comment 5 / comment 7), when trunk was version 44, and now trunk is version 45.  It's a bit confusing for one bug to have multiple patches which land across multiple releases, particularly when backports end up getting involved, as I suspect they will here for the yet-to-land patch.)

(If you'd rather not bother, feel free to tag this comment as offtopic to autohide it, so that I'm not adding too much to the confusion here. :) )

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8683404 [details] [diff] [review]
faster4.patch

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

Note this does help on Firefox builds because contrary to what I said earlier, Firefox builds /do/ use /some/ of the NSS build system recursion, and this change still reduces the amount of $(shell) for those builds. That said, with this change, we can also set CC_NAME and WARNING_CFLAGS on Firefox's end to reduce that even further.

::: coreconf/Werror.mk
@@ +44,5 @@
> +        # Clang reports its version as an older gcc, but it's OK
> +        NSS_ENABLE_WERROR = 1
> +      else
> +        CC_VERSION := $(subst ., ,$(shell $(CC) -dumpversion))
> +        ifeq ($(word 1,$(CC_VERSION)).$(word 2,$(CC_VERSION)),4.8)

You could even do ifneq (,$(filter 4.8 4.9,$(word 1,$(CC_VERSION)).$(word 2,$(CC_VERSION)))) to do both 4.8 and 4.9 in one pass.

Comment 32

[Martin Thomson [:mt:]]

dholbert, I've asked on NSS developers whether we can include this fix in 3.21, which will be uplifted to Aurora (I hope).  That will eliminate any confusion about version numbers (I hope).

Separately, I'll take gladium's suggestion re filtering.

Comment 33

[Martin Thomson [:mt:]]

Attachment: faster5.patch

Carry r=glandium  Hopefully this enables a bit of a performance boost over even previous versions of NSS.

Comment 34

[Kai Engert [:KaiE:]]

Comment on attachment 8683525 [details] [diff] [review]
faster5.patch

I see Martin has landed this patch at
https://hg.mozilla.org/projects/nss/rev/b9bdca563fba

Because of this patch, and because it enabled -Werror, my Fedora 22 system with gcc 5.1.1 can no longer build NSS.


sectime.c: In function ‘CERT_UTCTime2FormattedAscii’:
sectime.c:67:16: error: implicit declaration of function ‘PR_FormatTime’ [-Werror=implicit-function-declaration]
         if ( ! PR_FormatTime( timeString, 256, format, &printableTime )) {
                ^

That's because we define NO_NSPR_10_SUPPORT, which prevents the prtime.h header to declare PR_FormatTime.

Prior to this patch, my system reports this as a warning, only.

I'm surprised that I don't see this warning on the new Fedora builder that we recently added to build, which also uses Fedora 22.

Comment 35

[Kai Engert [:KaiE:]]

Please ignore comment 34, which was my fault.

My local tree was using NSPR 4.8.9 (to which I had changed my tree for some backporting/comparison stuff).

Comment 37

[Martin Thomson [:mt:]]

Attachment: bug1211568-2.patch

This is a version of the patch on bug 1216318, modified to include a check for Android.  The gecko build doesn't disable warnings-as-errors for Android builds because it doesn't pass in OS_TARGET.  Since I don't want to risk hitting some new code paths by exporting OS_TARGET, this seemed best.

Comment 38

[Kai Engert [:KaiE:]]

The NSS 3.21 release has been completed.

We are ready to uplift mozilla-central and aurora to NSS_3_21_RTM.

Comment 39

[Kai Engert [:KaiE:]]

Attachment: placeholder-3.21

Aurora currently uses a Beta snapshot of NSS 3.21.

We must upgrade it to a final version. The final version 3.21 is ready.

Comment 40

[Martin Thomson [:mt:]]

n.b., attachment 8684723 [details] [diff] [review] needs to land as well.

Comment 41

[Kai Engert [:KaiE:]]

Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

A build system fix, to ensure building NSS keeps working.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bda43f333e1a

Comment 43

[Kai Engert [:KaiE:]]

Comment on attachment 8683525 [details] [diff] [review]
faster5.patch

I believe this is the patch that landed at
https://hg.mozilla.org/projects/nss/rev/b9bdca563fba

Comment 44

[Kai Engert [:KaiE:]]

Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

I've landed NSS_3_21_RTM and this patch into m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bda43f333e1a

Comment 45

[Kai Engert [:KaiE:]]

[Tracking Requested - why for this release]: Because we cannot ship a Firefox release with a Beta version of NSS.

Comment 46

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/037285157301 for "Could not find EV root in NSS storage" assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17078993&repo=mozilla-inbound

Comment 47

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Kai, is this really ready for uplift to Aurora44? I noticed comment 46 and wanted to re-confirm.

Comment 48

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

Bug 1223466 will need to land and be uplifted first.

Comment 49

[Kai Engert [:KaiE:]]

(In reply to Ritu Kothari (:ritu) from comment #47)
> Kai, is this really ready for uplift to Aurora44? I noticed comment 46 and
> wanted to re-confirm.

It is blocked by bug 1223466, as David said.

We need approvals for both this bug and bug 1223466.

Bug 1223466 removes a test for the thing we must remove.

Comment 50

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kai Engert (:kaie) from comment #49)
> (In reply to Ritu Kothari (:ritu) from comment #47)
> > Kai, is this really ready for uplift to Aurora44? I noticed comment 46 and
> > wanted to re-confirm.
> 
> It is blocked by bug 1223466, as David said.
> 
> We need approvals for both this bug and bug 1223466.
> 
> Bug 1223466 removes a test for the thing we must remove.

Thanks David and Kai. In the case, could you please request aurora uplift in that bug as well? I don't think it is nominated yet.

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as NSS upgrades matter.

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64df3815df9c

Comment 53

[Kai Engert [:KaiE:]]

Second landing attempt.
https://hg.mozilla.org/integration/mozilla-inbound/rev/64df3815df9c

(try run seemed ok
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb468775d5ea )

Comment 54

[Mike Hommey [:glandium]]

Bugs are not closed until they hit mozilla-central.

Comment 55

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/64df3815df9c

Comment 56

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

Planned NSS upgrade for FF44. Approved.

Comment 57

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685054 [details]
placeholder-3.21

Please see my last comment.

Comment 58

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd168084d8a4

Comment 59

[Martin Thomson [:mt:]]

[Tracking Requested - why for this release]: As discussed elsewhere.  It would be good to get the latest NSS in the next ESR and maybe (maybe) Beta now that this has been shown to be fairly stable on Nightly.

Comment 60

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, please request uplift to beta so we can release this with 43. We will go to build on Monday as soon as everything is in place. Thanks.

Comment 61

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking and marking affected since we intend to uplift this to 43.

Comment 62

[Martin Thomson [:mt:]]

Comment on attachment 8685054 [details]
placeholder-3.21

Approval Request Comment
[Feature/regressing bug #]: 1211568
[User impact if declined]: NSS 3.21 contains a number of security fixes
[Describe test coverage new/current, TreeHerder]: NSS automatic testing, plus a few weeks in Nightly
[Risks and why]: None identified, there were a good number of changes in 3.21 though.
[String/UUID change made/needed]: None

Note that this depends on bug 1223466 and bug 1224481.  I'll request approval on those too.

Comment 63

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8685054 [details]
placeholder-3.21

[Triage Comment]

Please uplift to beta and m-r. 
We'd like to include this in today's 43 RC build.

Comment 64

[Kai Engert [:KaiE:]]

Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

Martin, I believe this patch will be required, too?

Comment 65

[Liz Henry (:lizzard) (relman/hg->git project)]

I hope Martin can confirm, but I haven't started the build yet (nor have we merged these patches) so there is still time to get this into the RC build. I hope to start the build later tonight or tomorrow morning.

Comment 66

[Martin Thomson [:mt:]]

Yes, Kai is right.

Comment 67

[Kathleen Wilson]

Perhaps it is a bit rushed to try to uplift NSS 3.21 into Firefox 43 one week before release?
Unless there is compelling reason to rush this, my recommendation is to wait until Firefox 44 (ESR 38.6).

Comment 68

[Kai Engert [:KaiE:]]

The CA certificate changes in the NSS 3.21 release had been timed to go final with the Firefox 44 schedule.

If you'd like to keep that original plan, but still want the code changes from NSS 3.21 in Firefox 43, we theoretically could create an NSS 3.21 with CA-Certificates version 2.5 (from NSS 3.20.1) tag. We'd need that, because that's what Distributions would use that ship NSS indepedently from Firefox.

But it's already 11 pm at my place, and I'd have to do that in about 12 hours from now (if you decided that's what you want).

I'll defer to Kathleen to make a final call if the root CA upgrade is acceptable for the Firefox 43 schedule.

Comment 69

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8685054 [details]
placeholder-3.21

Thanks Kai and Kathleen. Yes, maybe it is too rushed. 
Let's not uplift this to 43 now.

Comment 70

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

We decided not to take this in 43 (which was beta until last week) so just updating tracking flag to A-. Please let me know if I am mistaken.

Comment 71

[Liz Henry (:lizzard) (relman/hg->git project)]

From discussion with Tim and others from the project it sounds like this is not ready to go into ESR 38.6.0 but we want to land it for 38.7.0.

Comment 72

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Kai, Al: If I understand it right from https://bugzilla.mozilla.org/show_bug.cgi?id=1185033#c36, we need this NSS update to be uplifted to ESR38.7 in order to fix bug 1185033. I would like to gtb esr38.7 RC tomorrow. Can we get this landed before that? Please let me know. Thanks!

Comment 73

[Kai Engert [:KaiE:]]

(In reply to Ritu Kothari (:ritu) from comment #72)
> Hi Kai, Al: If I understand it right from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1185033#c36, we need this NSS
> update to be uplifted to ESR38.7 in order to fix bug 1185033. I would like
> to gtb esr38.7 RC tomorrow. Can we get this landed before that? Please let
> me know. Thanks!

The ESR 38.x branch uses a version of NSS that contains an older set of root CA certificates. The usual preference in the past was: avoid upgrading that list in ESR, unless there's an emergency need to do so.

I'm a bit surprised and slightly unhappy about the timing of your request.

I would have preferred an earlier statement from Mozilla drivers, that you seriously consider to upgrade the old ESR branch to a newer version of NSS.

First, it's very late in the current release cycle, just one week prior to release. I'm surprised you're willing to risk upgrading NSS to a newer version of the stable ESR branch that late in a 6 week cycle.

Second, I just recently did some extra work, to create a special release of NSS, based on the old NSS branch that ESR 38.x is currently using. If I had known earlier, I could have avoided that work.

So, if you really wanted to get NSS 3.21 uplifted to the ESR 38 branch, you would have to make the decision which root CAs you want.

I had already clarified that, almost 3 months ago, in comment 68, but nobody has made decisions regarding my explanation yet.

You would still have to make the decisions, before we can act. And if you decide not to upgrade the root CA list, then we'd still have to prepare a special NSS snapshot for that.

Comment 74

[Kai Engert [:KaiE:]]

Please respond to my comment 68 prior to asking for uplifting.

Comment 75

[Al Billings [:abillings - ex-MoCo]]

It seems way too late in the cycle to take an NSS update on ESR.

Comment 76

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kai Engert (:kaie) from comment #73)
> (In reply to Ritu Kothari (:ritu) from comment #72)
> > Hi Kai, Al: If I understand it right from
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1185033#c36, we need this NSS
> > update to be uplifted to ESR38.7 in order to fix bug 1185033. I would like
> > to gtb esr38.7 RC tomorrow. Can we get this landed before that? Please let
> > me know. Thanks!
> 
> The ESR 38.x branch uses a version of NSS that contains an older set of root
> CA certificates. The usual preference in the past was: avoid upgrading that
> list in ESR, unless there's an emergency need to do so.
> 
> I'm a bit surprised and slightly unhappy about the timing of your request.
> 
> I would have preferred an earlier statement from Mozilla drivers, that you
> seriously consider to upgrade the old ESR branch to a newer version of NSS.
> 
> First, it's very late in the current release cycle, just one week prior to
> release. I'm surprised you're willing to risk upgrading NSS to a newer
> version of the stable ESR branch that late in a 6 week cycle.
> 
> Second, I just recently did some extra work, to create a special release of
> NSS, based on the old NSS branch that ESR 38.x is currently using. If I had
> known earlier, I could have avoided that work.

I apologize for not doing my due diligence before requesting an uplift on this one.
> 
> So, if you really wanted to get NSS 3.21 uplifted to the ESR 38 branch, you
> would have to make the decision which root CAs you want.

I was unaware that we have different versions of NSS on the pre-release (esr38.7, beta45, aurora46) channels (as landed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1245528#c53, c54, c55).
> 
> I had already clarified that, almost 3 months ago, in comment 68, but nobody
> has made decisions regarding my explanation yet.

I can clearly see that we dropped the ball on this one between comment 67 and 68. While the timing was not ok to upgrade to 3.21 a week before ESR38.6, we could have handled this better for ESR38.7. I wish I had followed up on this one sooner. :(
> 
> You would still have to make the decisions, before we can act. And if you
> decide not to upgrade the root CA list, then we'd still have to prepare a
> special NSS snapshot for that.

As you and Al have both pointed out, it is too late to take another NSS update so late in esr38.7 cycle. Let's reconsider early in esr 38.8 cycle.

Comment 77

[Kai Engert [:KaiE:]]

FYI, on this page there's an overview which versions of NSPR, NSS and the CA-certificates list are used by the various branches of Firefox. It's updated automatically, daily.
http://kuix.de/mozilla/versions/

Comment 78

[Kathleen Wilson]

(In reply to Ritu Kothari (:ritu) from comment #76)
> Let's reconsider early in esr 38.8 cycle.

It sounds like there needs to be a version of NSS with a certain set of bug fixes in it, but with the ESR 38 version of certdata.txt (CA Certificates).

From http://kuix.de/mozilla/versions/ ...
Mozilla software version on branch mozilla-esr38: 38.6.1esrpre
NSPR (portable runtime) version: NSPR_4_10_10_RTM
NSS (security library) version: NSS_3_19_2_3_RTM
CKBI (builtin trust) version: 2.4

I wonder if the best approach would be to identify the NSS bug fixes that need to get into ESR, and then create an NSS_3_19_2_4 specifically for ESR 38.8?

Who can determine the list of NSS bugs that need to go into ESR 38.8?

Comment 79

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Kathleen Wilson from comment #78)
> (In reply to Ritu Kothari (:ritu) from comment #76)
> > Let's reconsider early in esr 38.8 cycle.
> 
> It sounds like there needs to be a version of NSS with a certain set of bug
> fixes in it, but with the ESR 38 version of certdata.txt (CA Certificates).
> 
> From http://kuix.de/mozilla/versions/ ...
> Mozilla software version on branch mozilla-esr38: 38.6.1esrpre
> NSPR (portable runtime) version: NSPR_4_10_10_RTM
> NSS (security library) version: NSS_3_19_2_3_RTM
> CKBI (builtin trust) version: 2.4
> 
> I wonder if the best approach would be to identify the NSS bug fixes that
> need to get into ESR, and then create an NSS_3_19_2_4 specifically for ESR
> 38.8?
> 
> Who can determine the list of NSS bugs that need to go into ESR 38.8?

Hi Kathleen, as far as I can tell, that list of bugs is 1185033, 1209546, 1190248. Dan also suggested the same idea.

Comment 80

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Also I believe Dan was suggesting we backport these 3 fixes to esr38.7. Is that even possible?

Comment 81

[Kai Engert [:KaiE:]]

clearing needinfo, as we're discussing the plan by email.

There seems to be preference to not uplift 3.21.1, but rather to create another 3.19.2.x release for 38.8