Closed Bug 1415795 Opened 2 years ago Closed 2 years ago

Firefox 57.0 fails to build --with-system-nss where NSS >= 3.35

Categories

(NSS :: Libraries, defect)

defect
Not set

Tracking

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 wontfix, firefox58 fixed, firefox59 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jbeich, Assigned: franziskus)

Details

(Keywords: regression)

Attachments

(4 files)

After bug 1391506 Firefox consumes experimental NSS API which was renamed in bug 1411475. --with-system-nss is used by a few distrubutions which maybe confused given both Firefox 57.0 and NSS 3.34 are close to be released.

$ echo "ac_add_options --with-system-nss" >>.mozconfig
$ ./mach build
[...]
In file included from objdir/security/manager/ssl/Unified_cpp_security_manager_ssl2.cpp:20:
security/manager/ssl/nsNSSIOLayer.cpp:2595:23: error:
      use of undeclared identifier 'SSL_UseAltServerHelloType'
    if (SECSuccess != SSL_UseAltServerHelloType(fd, PR_TRUE)) {
                      ^
1 error generated.

https://hg.mozilla.org/projects/nss/diff/cf8adf59ff56/lib/ssl/sslexp.h requires
https://hg.mozilla.org/mozilla-central/diff/996440f4c257/security/manager/ssl/nsNSSIOLayer.cpp
Version: Trunk → 57 Branch
The issue is that Firefox is using the experimental API directly instead of calling it the way it's supposed to (querying for a name and ignoring the function call if it's not present).
So this requires us either to revert the name change in NSS or land a hot fix in Firefox 57 (not a great idea) to use the experimental API properly.
Flags: needinfo?(martin.thomson)
Comment on attachment 8926793 [details]
Bug 1415795 - revert renaming of SSL_UseAltServerHelloType, r=ttaubert

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D209#5211
Attachment #8926793 - Flags: review+
Comment on attachment 8926819 [details]
Bug 1415795 - revert name change of NSS API, r=ttaubert

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D211#5232
Attachment #8926819 - Flags: review+
Landed on 3.34 branch https://hg.mozilla.org/projects/nss/rev/2b0ba0c7128c173b2307509f110183e47b9ed1fe.
Martin, can you handle this for trunk? Not sure how we want to handle it there. But probably similar.
Is this on track for the 57 rcs ? from the tracking flags, not sure...
This is NOT a problem.  All this work you have done isn't necessary.  In fact, it's just disruptive.  This is not an exported API (see ssl.def).

Please read and understand sslexp.h before making these sorts of changes.
Flags: needinfo?(martin.thomson)
Oh, I see what happened here.  You have 3.34.  My original patch - as it landed in m-i, was for 3.35, which includes the right sslexp.h version.  Renaming is fine in that case (the function it might not work, but that's OK).
https://hg.mozilla.org/mozilla-central/rev/af86f905265d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
OK, now that I've stopping being stupid about this, I think that this is OK for now, but not the solution that I would prefer long term.

I will propose that we retain a definition for APIs that we remove as follows:

#define SSL_DEPRECATED_EXPERIMENTAL_API (PORT_SetError(SSL_ERROR_UNSUPPORTED_EXPERIMENTAL_API), SECFailure)
#define SSL_UseAltServerHelloType(x, y) SSL_DEPRECATED_EXPERIMENTAL_API
Assignee: nobody → franziskuskiefer
Status: RESOLVED → REOPENED
Component: Security → Libraries
Product: Core → NSS
Resolution: FIXED → ---
Summary: Firefox 57.0 fails to build --with-system-nss where NSS >= 3.34 → Firefox 57.0 fails to build --with-system-nss where NSS >= 3.35
Target Milestone: mozilla58 → 3.35
Version: 57 Branch → trunk
Comment on attachment 8927519 [details]
Bug 1415795 - Provide a macro to support removal of experimental APIs, r?franziskus

Eric Rescorla (:ekr) has approved the revision.

https://phabricator.services.mozilla.com/D222#6296
Attachment #8927519 - Flags: review+
Comment on attachment 8929994 [details]
Bug 1415795 - revert renaming of SSL_UseAltServerHelloType

Martin Thomson [:mt:] has approved the revision.

https://phabricator.services.mozilla.com/D261#6390
Attachment #8929994 - Flags: review+
Comment on attachment 8927519 [details]
Bug 1415795 - Provide a macro to support removal of experimental APIs, r?franziskus

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.

https://phabricator.services.mozilla.com/D222#6467
Attachment #8927519 - Flags: review+
Everything in here should have landed and all breakage should be fixed.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Do we need to uplift something to Beta here?
Flags: needinfo?(franziskuskiefer)
Nope, this was fixed in 58 (comment 4 and comment 5). Everything after that is about fixing it on trunk (59).
Flags: needinfo?(franziskuskiefer)
You need to log in before you can comment on or make changes to this bug.