Closed Bug 1211568 Opened 4 years ago Closed 4 years ago

Upgrade Firefox 44 to NSS 3.21

Categories

(Core :: Security: PSM, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 + wontfix
firefox44 + fixed
firefox45 --- fixed
firefox-esr38 45+ ---

People

(Reporter: KaiE, Assigned: mt)

References

Details

Attachments

(3 files, 5 obsolete files)

Upgrade Firefox 44 to NSS 3.21
r=me
Landed Beta2.
Keywords: leave-open
Depends on: 1216318
clearing needinfo, Martin worked on the fix.
Flags: needinfo?(kaie)
No longer depends on: 1216318
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(martin.thomson)
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.
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.
(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.)
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.
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.
It's likely that it's using clang that is causing the slowdown.
Attached patch faster.patch (obsolete) — Splinter Review
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.
Attachment #8679208 - Flags: feedback?(n.nethercote)
Attached patch fasterfaster.diff (obsolete) — Splinter Review
This might be even faster, but it's a bigger change.
Attachment #8679210 - Flags: feedback?(n.nethercote)
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 on attachment 8679208 [details] [diff] [review]
faster.patch

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

As per comment 18.
Attachment #8679208 - Flags: feedback?(n.nethercote) → feedback+
Comment on attachment 8679210 [details] [diff] [review]
fasterfaster.diff

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

As per comment 18.
Attachment #8679210 - Flags: feedback?(n.nethercote) → feedback+
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.
Attachment #8679210 - Flags: review?(wtc)
Attachment #8679210 - Flags: review?(kaie)
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.
Attachment #8679210 - Flags: review?(wtc)
Attachment #8679210 - Flags: review?(kaie)
Attachment #8679210 - Flags: review-
> 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.
Flags: needinfo?(martin.thomson)
Attached patch faster2.patch (obsolete) — Splinter Review
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.
Attachment #8679208 - Attachment is obsolete: true
Attachment #8679210 - Attachment is obsolete: true
Attachment #8680974 - Flags: review?(mh+mozilla)
(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 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?
Attached patch faster3.patch (obsolete) — Splinter Review
I merged the two patches (sorry :njn) and made a few more performance tweaks, as suggested by Wan-Teh.
Attachment #8680974 - Attachment is obsolete: true
Attachment #8680974 - Flags: review?(mh+mozilla)
Attachment #8681013 - Flags: review?(mh+mozilla)
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.
Attachment #8681013 - Flags: review?(mh+mozilla) → feedback+
Attached patch faster4.patch (obsolete) — Splinter Review
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.
Attachment #8681013 - Attachment is obsolete: true
Attachment #8683404 - Flags: review?(mh+mozilla)
(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. :) )
Flags: needinfo?(martin.thomson)
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.
Attachment #8683404 - Flags: review?(mh+mozilla) → review+
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.
Flags: needinfo?(martin.thomson)
Attached patch faster5.patchSplinter Review
Carry r=glandium  Hopefully this enables a bit of a performance boost over even previous versions of NSS.
Attachment #8683404 - Attachment is obsolete: true
Attachment #8683525 - Flags: review+
Assignee: nobody → martin.thomson
Duplicate of this bug: 1222657
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.
Attachment #8684723 - Flags: review?(ted)
The NSS 3.21 release has been completed.

We are ready to uplift mozilla-central and aurora to NSS_3_21_RTM.
Attached file 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.
Attachment #8685054 - Flags: review?(martin.thomson)
Attachment #8685054 - Flags: approval-mozilla-aurora?
Attachment #8685054 - Flags: review?(martin.thomson) → review+
n.b., attachment 8684723 [details] [diff] [review] needs to land as well.
Attachment #8684723 - Flags: review?(ted) → review+
Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

A build system fix, to ensure building NSS keeps working.
Attachment #8684723 - Flags: approval-mozilla-aurora?
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
Attachment #8683525 - Flags: checkin+
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
Attachment #8684723 - Flags: review+
Attachment #8684723 - Flags: checkin+
Attachment #8685054 - Flags: checkin+
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
[Tracking Requested - why for this release]: Because we cannot ship a Firefox release with a Beta version of NSS.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Kai, is this really ready for uplift to Aurora44? I noticed comment 46 and wanted to re-confirm.
Flags: needinfo?(kaie)
Bug 1223466 will need to land and be uplifted first.
(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.
Flags: needinfo?(kaie)
(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.
Tracked as NSS upgrades matter.
Depends on: 1224481
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 )
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Bugs are not closed until they hit mozilla-central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/64df3815df9c
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1224875
Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

Planned NSS upgrade for FF44. Approved.
Attachment #8684723 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8685054 [details]
placeholder-3.21

Please see my last comment.
Attachment #8685054 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[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.
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.
Flags: needinfo?(martin.thomson)
Tracking and marking affected since we intend to uplift this to 43.
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.
Flags: needinfo?(martin.thomson)
Attachment #8685054 - Flags: approval-mozilla-beta?
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.
Attachment #8685054 - Flags: approval-mozilla-release+
Attachment #8685054 - Flags: approval-mozilla-beta?
Attachment #8685054 - Flags: approval-mozilla-beta+
Comment on attachment 8684723 [details] [diff] [review]
bug1211568-2.patch

Martin, I believe this patch will be required, too?
Attachment #8684723 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(martin.thomson)
Yes, Kai is right.
Flags: needinfo?(martin.thomson)
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).
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 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.
Attachment #8685054 - Flags: approval-mozilla-release-
Attachment #8685054 - Flags: approval-mozilla-release+
Attachment #8685054 - Flags: approval-mozilla-beta-
Attachment #8685054 - Flags: approval-mozilla-beta+
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.
Attachment #8684723 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
Depends on: 1244069
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!
Flags: needinfo?(kaie)
Flags: needinfo?(abillings)
(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.
Flags: needinfo?(kaie)
Please respond to my comment 68 prior to asking for uplifting.
It seems way too late in the cycle to take an NSS update on ESR.
Flags: needinfo?(abillings)
(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.
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/
(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?
(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.
Flags: needinfo?(kwilson)
Also I believe Dan was suggesting we backport these 3 fixes to esr38.7. Is that even possible?
Flags: needinfo?(kwilson) → needinfo?(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
Flags: needinfo?(kaie)
You need to log in before you can comment on or make changes to this bug.