Last Comment Bug 648407 - Fold NSPR and NSS into mozjs (for Windows) or libxul (for other platforms)
: Fold NSPR and NSS into mozjs (for Windows) or libxul (for other platforms)
Status: RESOLVED FIXED
[ts]
: addon-compat, perf
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla22
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 853296 709193 750661 832272 838165 852068 853364 855173 855325 857147 867861 937005 1024620
Blocks: 561842 915574
  Show dependency treegraph
 
Reported: 2011-04-07 16:22 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2014-06-12 18:10 PDT (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Support expandlibs_exec.py --extract on windows (9.72 KB, patch)
2013-02-07 00:44 PST, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
Allow to fold NSPR, NSS and SQLite libraries all together (24.14 KB, patch)
2013-02-07 02:00 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
Allow to fold NSPR, NSS and SQLite libraries all together (24.82 KB, patch)
2013-02-07 06:43 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
Support loading the folded library on Android (4.83 KB, patch)
2013-02-07 14:28 PST, Mike Hommey [:glandium]
blassey.bugs: review+
Details | Diff | Review
Fold NSPR, NSS and SQLite libraries all together on B2G, Android, OSX and Windows (1.90 KB, patch)
2013-02-07 14:37 PST, Mike Hommey [:glandium]
blassey.bugs: review+
cjones.bugs: review+
benjamin: review+
Details | Diff | Review
Allow to fold NSPR, NSS and SQLite libraries all together (25.03 KB, patch)
2013-02-08 01:02 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
Allow to fold NSPR, NSS and SQLite libraries all together (25.03 KB, patch)
2013-02-08 05:47 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
Support expandlibs_exec.py --extract on windows (9.74 KB, patch)
2013-03-03 23:50 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
Allow to fold NSPR, NSS and SQLite libraries all together (24.66 KB, patch)
2013-03-04 00:50 PST, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-04-07 16:22:50 PDT

    
Comment 1 Ted Mielczarek [:ted.mielczarek] 2011-04-08 09:39:58 PDT
If we're going to fold them into mozjs on Windows, I'd rather that we rename the resulting binary. We'll have to do some fiddling in the build system either way, since JS gets linked way before NSS gets built anyway.
Comment 2 (dormant account) 2011-04-08 09:44:35 PDT
(In reply to comment #1)
> If we're going to fold them into mozjs on Windows, I'd rather that we rename
> the resulting binary. We'll have to do some fiddling in the build system either
> way, since JS gets linked way before NSS gets built anyway.

Good point. restofxulcosmsvcsucks.dll?
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-04-08 09:46:05 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > If we're going to fold them into mozjs on Windows, I'd rather that we rename
> > the resulting binary. We'll have to do some fiddling in the build system either
> > way, since JS gets linked way before NSS gets built anyway.
> 
> Good point. restofxulcosmsvcsucks.dll?

Have we tested bigxul.dll with MSVC 2010?
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-04-08 09:49:04 PDT
I reproduced the linker crash with VC2010 some time ago, but I haven't tried with SP1.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-04-08 09:53:20 PDT
Linker crash?  I thought js linked into xul fine, but the resulting binary failed some js tests on a semi-regular basis.  Does it die completely when you add nspr/nss?
Comment 6 Ted Mielczarek [:ted.mielczarek] 2011-04-08 09:55:24 PDT
The patch from bug 591836 was causing a linker error, bug 598663. I filed bug 611053 to remember to file it with Microsoft, but I couldn't quite get a reproducible test case together.
Comment 7 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-19 20:46:13 PST
I started a discussion somewhat indirectly related to this on dev.platform. Because of addons that are linking to NSS and NSPR, it is more complicated than it seems. If we go ahead with this, we will have to be very conscious of how we maintain backward compatibility. e.g. we may need to create forwarder DLLs the the same names as the current NSS libraries, that forward function calls to to libxul or wherever the function ends up living.

Sean has said that, when he has some time, he will offer some counter-arguments against doing this.
Comment 8 Max 2012-01-30 11:54:29 PST
I am a CS professor at a major university and I also run a large open-source security-related project. 

I would really discourage to proceed with the proposed plan. Folding NSS and NSPR into mozjs or libxul is a bad move for many reasons.

Besides the fact that many different projects that now rely on the availability of those libraries to build extensions will probably switch to different browsers (e.g., research - academia - and commercial extensions) for their implementation, the proposed action is also really bad for the FIPS certification of the security module. Indeed, it would be impossible to have FIPS certified security on Firefox, thus forcing many millions of users to switch to other platforms (especially to Win + IE) for secure applications (not only extensions.. but also server-side: using FF without a certified FIPS security module in many environments - like federal government - would be banned!).

Please, Please, Please... don't even think about proceeding in this direction as it would probably mean the definitive decline of the overall development platform and, in the end, the impossibility of using FF for secure applications (e.g., government apps, etc.)

The same considerations apply to Thunderbird if the proposed plan is meant to be implemented for that app too.
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-03 09:44:22 PST
(In reply to Max from comment #8)
> Besides the fact that many different projects that now rely on the
> availability of those libraries to build extensions will probably switch to
> different browsers (e.g., research - academia - and commercial extensions)

Please see my most recent post on the dev-platform mailing list. If/when this is done, we will likely do so in a way that is as backward-compatible with as many extensions as possible--e.g. by creating forwarder DLLs that forward calls to the old libraries to libxul.

> for their implementation, the proposed action is also really bad for the
> FIPS certification of the security module. 

My plan is not to fold Softoken into libxul, at least in the first round. I may combine freebl and softokn into one library, which shouldn't affect (too much) its ability to get certified. But, note that Firefox hasn't shipped with a FIPS-certified security module since before Firefox 4 and I don't know if/when we will pursue FIPS certification again.
Comment 10 Sean Leonard 2012-02-03 10:02:24 PST
[These remarks were prepared in advance after a fair amount of research.]

Please, do not this detestable thing!

As documented in bug 533014, bug 534471, and bug 561842, linking NSPR and NSS has intractable technical, policy, accessible and user acceptance problems.

"If a process has two copies of NSPR, and an application is not careful in isolating the copies of NSPR (for example, passing a memory block allocated by PR_Malloc), things won't work."

"NSS is FIPS validated in shared library form only. If you're using NSS for its FIPS validation, do *not* use NSS as static libraries."

FIPS compliant AND certified software is a requirement of all US Federal government agencies, and is increasingly required by non-US organizations and a variety of verticals (e.g., a procurement requirement among financial industry verticals). If you break FIPS compliance, you can permanently remove up to 17 million US Federal desktops (not to mention mobile devices) from your user list.

From a user or sysadmin's perspective, it is valuable to verify that the cryptographic components (nss, smime3, ssl3, softokn3, etc.) have not been tampered with. The easiest way to verify these components is to keep them separate components, and to use known component versions that have undergone some kind of vetting. It is impossible to verify component versions or the overall state of the modules when they have literally been fused and mixed into a huge mass of unrelated--and far less stable--code.

Now that Mozilla has been pushing "js-ctypes" and has been wreaking untold havoc on XPCOM that Mozilla promised years ago would be frozen and stable, developers have little choice in stable platform APIs *except for* C API interfaces, accessible through their own XPCOM or js-ctypes abstractions, if they want to do anything serious. Being system-class libraries, NSPR and NSS have been held out as very stable and platform-neutral. A fundamental software engineering requirement of stability is that the API entry points do not change. When you combine this requirement with the fact that NSS loses all of its advantages when in static form, it simply makes sense to keep the implementation in the libraries in the shared libraries in the first place. Note also that the FIPS Security Policy submitted by Mozilla, Red Hat, and Sun explicitly call out NSPR as a requirement in shared library form. See <http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140val-all.htm>, including <http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140sp/140sp1278.pdf>, <http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140sp/140sp1279.pdf>, and <http://csrc.nist.gov/groups/STM/cmvp/documents/140-1/140sp/140sp1280.pdf>.

Some squak has been made about a "native" API for crypto accessible to add-ons. There already is a perfectly functional native API: it's called NSS.† It's used directly and indirectly by Firefox components and add-ons alike. Removing NSS but requiring some sort of "native" API will just cause you to reinvent the wheel, which saves neither size or speed, and wastes a lot of money.

†PSM has a couple of useful objects that abstract the NSS functionality, like nsICryptoHash. These are mostly thin wrappers around the native NSS objects.

Any "JavaScript-oriented API" for cryptography or other basic services (e.g., NSPR's sockets, file handles, pipes, shared memory, etc.) will interact poorly with threading, because all chrome-stuff is supposed to happen on the main chrome thread in the main Mozilla process. In C/C++, threading is an issue but how to deal with threading is well-known and well-documented.

Speaking of documentation: while the documentation on NSS is lacking, at least NSS has documentation scattered about. There is no documentation for code or interfaces that have not even been written yet. Better to beef up the existing NSS documentation, than reinvent the wheel.

PSM--bless its soul--is fine for the bare minimum that it sets out to accomplish: a partial abstraction of certain NSS functions for the bare minimum requirements of XUL front-end components like Certificate Manager and Certificate Viewer. It's been around for a decade and nobody has given it a serious overhaul. What makes anyone think that removing NSS/NSPR is going to make PSM magically get all this new functionality with $0 budget?

Some say "usability is the enemy of security". That is wrong. Truly secure systems should also be usable by their target user populations. "PERFORMANCE IS THE ENEMY OF SECURITY!" Security means that you verify things all the time, every time, even when it's not necessary, and you make as few assumptions as possible. Performance means you verify as little as possible and assume as much as possible.

Here, we are talking about security and operating system components. The whole point of NSS is to provide security. (The whole point of NSPR is to provide operating system primitives in a non-OS-dependent fashion--and since a lot of OS primitives provide process isolation, it is fair to say that security is a significant part of NSPR too.) Security is harmed by making NSS static.

Assuming that Firefox does not actively hinder OS optimizations, loading NSS and NSPR is no different from loading any other countless number of shared libraries. Windows already has several technologies to improve application load time, such as the Windows Prefetcher (Windows XP+), SuperFetch (Vista+), and ReadyBoost (Vista+). Other OSes have similar facilities.

Making NSS faster is a laudable goal, but not at the expense of its security. Let's talk about loading NSS and PSM faster in an appropriate other bug, such as bug 711032.
Comment 11 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-03 10:26:02 PST
(In reply to Sean Leonard from comment #10)
> "If a process has two copies of NSPR, and an application is not careful in
> isolating the copies of NSPR (for example, passing a memory block allocated
> by PR_Malloc), things won't work."

This will be a non-issue. There will only be one copy of NSPR and one copy of NSS unless the extension is doing something very wrong. (Remember that we would not do this on Linux *at all*).

> "NSS is FIPS validated in shared library form only. If you're using NSS for
> its FIPS validation, do *not* use NSS as static libraries."

> If you break FIPS compliance, you can permanently
> remove up to 17 million US Federal desktops (not to mention mobile devices)
> from your user list.

The NSS that is shipped with Firefox 4 and later is ***NOT*** FIPS certified. I know of no plans to change this. This is something that should be discussed on dev.planning.

NSS is a collection of several libraries, and only a few of them (basically, softoken, freebl, and nssutil) affect the FIPS validation of NSS. As far as I am aware, linking the rest of the libraries together causes no problems regarding validation, or the ability to substitute a different (FIPS validated) module.

> From a user or sysadmin's perspective, it is valuable to verify that the
> cryptographic components (nss, smime3, ssl3, softokn3, etc.) have not been
> tampered with. The easiest way to verify these components is to keep them

The same argument can be made for libxul, which has components that are just as security-critical as any component in NSS.

> they want to do anything serious. Being system-class libraries

Only on Linux, where we are not planning to make this change.

> A fundamental software engineering requirement of stability is that the
> API entry points do not change.

Since Firefox 4, we have not even tried to have "stability." To what extent we make effort to have stable APIs is a discussion to be had in dev.platform or dev.planning.

> Some squak has been made about a "native" API for crypto accessible to
> add-ons. There already is a perfectly functional native API: it's called
> NSS.† It's used directly and indirectly by Firefox components and add-ons
> alike. 

With one exception (which is a bug), NSS is used exclusively in Gecko by PSM, and it is for all practical purposes an implementation detail of PSM that leaked out so that addons started depending on it. Like I said in the mailing list, we will deal with the compatibility-related aspects of this change in a reasonable way if/when this change is made.

> †PSM has a couple of useful objects that abstract the NSS functionality,
> like nsICryptoHash. These are mostly thin wrappers around the native NSS
> objects.

Yes, these are the *stable* (for JS-based components) APIs that (JS-based) components can rely on.

> Any "JavaScript-oriented API" for cryptography or other basic services
> (e.g., NSPR's sockets, file handles, pipes, shared memory, etc.) will
> interact poorly with threading, because all chrome-stuff is supposed to
> happen on the main chrome thread in the main Mozilla process.

We already know we have to deal with threading issues with the DOMCrypt APIs.

> Speaking of documentation: while the documentation on NSS is lacking, at
> least NSS has documentation scattered about. There is no documentation for
> code or interfaces that have not even been written yet. Better to beef up
> the existing NSS documentation, than reinvent the wheel.

I agree this is a sore point. This is why we are working with the rest of the industry to document DOMCrypt as a future W3C recommendation.

> and Certificate Viewer. It's been around for a decade and nobody has given
> it a serious overhaul. What makes anyone think that removing NSS/NSPR is
> going to make PSM magically get all this new functionality with $0 budget?

This bug is part of the long-overdue overhaul of PSM.

> "PERFORMANCE IS THE ENEMY OF SECURITY!"

http://www.chromium.org/Home/chromium-security/core-principles#TOC-Speed-matters.

> Here, we are talking about security and operating system components.

No, because NSS and NSPR are only OS components on Linux and Unix-based systems where this change isn't planned to happen.

> Assuming that Firefox does not actively hinder OS optimizations, loading NSS
> and NSPR is no different from loading any other countless number of shared
> libraries. Windows already has several technologies to improve application
> load time, such as the Windows Prefetcher (Windows XP+), SuperFetch
> (Vista+), and ReadyBoost (Vista+). Other OSes have similar facilities.

We won't do this if it turns out not to be a performance win. (And, let's not divert this bug into a discussion of the merits and demerits of the above technologies, please!)

> Making NSS faster is a laudable goal, but not at the expense of its
> security. Let's talk about loading NSS and PSM faster in an appropriate
> other bug, such as bug 711032.

To be clear, I filed bug 711032 and the other related bugs to keep track of possible alternatives to doing this work, partially because of the problems you noted above, and partially to avoid creating a Mozilla-specific build configuration of NSS. I do not know yet to what extent bug 711032 (et al.) will result in better or worse startup performance than fixing this bug will, or if a combination of this fix and those fixes will result in better performance.
Comment 12 Sean Leonard 2012-02-03 11:11:22 PST
(In reply to Brian Smith (:bsmith) from comment #11)
> (In reply to Sean Leonard from comment #10)
> > "If a process has two copies of NSPR, and an application is not careful in
> > isolating the copies of NSPR (for example, passing a memory block allocated
> > by PR_Malloc), things won't work."
> 
> This will be a non-issue. There will only be one copy of NSPR and one copy
> of NSS unless the extension is doing something very wrong. (Remember that we
> would not do this on Linux *at all*).

Please add Mac OS X and Windows to the list of not doing this. For no other reason, if a developer has to fork development efforts just to support Linux (a < 10% use case), it's not worth supporting at all.

> > "NSS is FIPS validated in shared library form only. If you're using NSS for
> > its FIPS validation, do *not* use NSS as static libraries."
> 
> > If you break FIPS compliance, you can permanently
> > remove up to 17 million US Federal desktops (not to mention mobile devices)
> > from your user list.
> 
> The NSS that is shipped with Firefox 4 and later is ***NOT*** FIPS
> certified. I know of no plans to change this. This is something that should
> be discussed on dev.planning.
> 
> NSS is a collection of several libraries, and only a few of them (basically,
> softoken, freebl, and nssutil) affect the FIPS validation of NSS. As far as
> I am aware, linking the rest of the libraries together causes no problems
> regarding validation, or the ability to substitute a different (FIPS
> validated) module.

NSS is *the* platform-neutral API that allows uniform access to a variety of different PKCS #11 cryptographic modules, in addition to uniform certificate and other crypto-related services. That means that a developer who wants to support many possible modules (including FIPS-certified smartcards, etc.) is best left to program to NSS, not to the underlying PKCS #11 crypto modules, which are definitely hidden away by NSS has much as possible.

NSS shipped with Firefox 4+ is not FIPS certified but uses the same compatible API. Should Mozilla or others feel that it is desirable to achieve FIPS certification, the cost is very minimal as long as the API remains the same and there are few security-relevant changes. The change log is mostly self-certified.

Also, due to the compatible API, it is possible to swap out the newer freebl and softokn3 with the one from Firefox 3.6 or below, and the code will STILL WORK. That makes sense, because NSS -> softokn3 is a stable API based on an industry standard, PKCS #11. I just tried it now for the lulz on Windows 7 with Firefox 10 and it still works. (Note: to anyone who tries this: you need to copy softokn3.dll, softokn3.chk, freebl3.dll, freebl3.chk, sqlite3.dll, and mozcrt19.dll.) It booted Firefox 10, all my certificates were visible, and I could go to SSL/TLS websites. Even EV with the green bar worked fine, Which implies that CERT_PKIXVerifyCert and other sundries are not affected! I even set a master password and enabled FIPS mode, and it's still working!

Saying "okay, program to PKCS #11" is very unhelpful. If you write directly to the PKCS #11 API to softokn3, for example, it will de-synchronize the cache of data that NSS has internally for certificates, public keys, and other metadata.

> > "PERFORMANCE IS THE ENEMY OF SECURITY!"
> 
> http://www.chromium.org/Home/chromium-security/core-principles#TOC-Speed-
> matters.

That document in Chomium talks about speed in the context of human developer response time, not speed in terms of the quickness of the code. Perhaps to help you out, you should have cited this instead:
http://www.webkit.org/projects/performance/index.html

Regarding speed, here we are talking about fractions of microseconds on startup--a one-time event that for most Firefox users is a non-issue once the browser is up and running. The bigger usability issue to face is reducing the number of required restarts. If we are talking about a "heavy" crypto operation such as RSA key verification, generation, or signing, the speed of the crypto operation significantly dwarfs the NSS scaffolding. Linking NSS statically is hardly the area to optimize.

> > Assuming that Firefox does not actively hinder OS optimizations, loading NSS
> > and NSPR is no different from loading any other countless number of shared
> > libraries. Windows already has several technologies to improve application
> > load time, such as the Windows Prefetcher (Windows XP+), SuperFetch
> > (Vista+), and ReadyBoost (Vista+). Other OSes have similar facilities.
> 
> We won't do this if it turns out not to be a performance win. (And, let's
> not divert this bug into a discussion of the merits and demerits of the
> above technologies, please!)
> 
> > Making NSS faster is a laudable goal, but not at the expense of its
> > security. Let's talk about loading NSS and PSM faster in an appropriate
> > other bug, such as bug 711032.
> 
> To be clear, I filed bug 711032 and the other related bugs to keep track of
> possible alternatives to doing this work, partially because of the problems
> you noted above, and partially to avoid creating a Mozilla-specific build
> configuration of NSS. I do not know yet to what extent bug 711032 (et al.)
> will result in better or worse startup performance than fixing this bug
> will, or if a combination of this fix and those fixes will result in better
> performance.

Thanks. I will confine my performance suggestions to bug 711032 (et al.).
Comment 13 Mike Hommey [:glandium] 2013-02-07 00:44:38 PST
Created attachment 711193 [details] [diff] [review]
Support expandlibs_exec.py --extract on windows
Comment 14 Mike Hommey [:glandium] 2013-02-07 02:00:53 PST
Created attachment 711216 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together

This is the bulk of the change. It expands what is done in bug 832272, and effectively makes the build even more parallelized than after bug 832272 as a byproduct. More importantly, if MOZ_FOLD_LIBS is defined, it builds a single libnss3 library containing all of nspr4, plc4, plds4, sqlite3, nss3, ssl3, smime3, nssutil3, and, as it happens, freebl (which was statically linked in ssl3).
The lib is still called nss3 for two main reasons:
- some part of NSS uses PR_GetLibraryFilePathname, and gives it the nss3 library name, and that is hardcoded in NSS at the moment
- at least the sync crypto stuff is opening nss3 with ctypes.

I wanted to leave NSS alone for now, thus this build system goo.

Note that there is a DllMain in nspr, which, fortunately, is the only library with a DllMain in all the ones folded here. Any additional folding will need to take this into account. For example, I wanted to add mozglue, but mozglue comes with its own DllMain through jemalloc (which, jemalloc3 doesn't have, btw). JS also has its own DllMain, although it does nothing.
Comment 15 Mike Hommey [:glandium] 2013-02-07 06:43:50 PST
Created attachment 711317 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together

Refreshed against bug 832272 and fixed b2g.
Comment 16 Mike Hommey [:glandium] 2013-02-07 14:28:11 PST
Created attachment 711522 [details] [diff] [review]
Support loading the folded library on Android

Other parts of this bug allow to build NSPR+NSS+SQLite as a single library ; these changes are the required bits to make Firefox for Android load the single library when such folding is enabled.
Comment 17 Mike Hommey [:glandium] 2013-02-07 14:37:33 PST
Created attachment 711540 [details] [diff] [review]
Fold NSPR, NSS and SQLite libraries all together on B2G, Android, OSX and Windows
Comment 18 Mike Hommey [:glandium] 2013-02-07 14:39:22 PST
Comment on attachment 711540 [details] [diff] [review]
Fold NSPR, NSS and SQLite libraries all together on B2G, Android, OSX and Windows

This makes us link all of nspr, plc, plds, nss, ssl, smime, nssutil and sqlite as a single library.
Tagging cjones for b2g, in case other things somehow rely on any of these libraries.
Tagging blassey for android
Tagging bsmedberg for desktop Firefox. We're not enabling on non-windows non-OSX because on these systems (especially Fedora Linux), nspr, plc, plds, nss, ssl, smime and nssutil may be DT_NEEDED by system libraries, and folding them can create subtle problems, especially considering the folded library doesn't have any symbol versioning, contrary to vanilla nss, ssl, smime and nssutil.

The nice thing is that in case it breaks things, it's easy to turn off.
Comment 19 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-02-07 18:18:43 PST
Mike, you should build libssl with NO_PKCS11_BYPASS defined because (a) we don't use the BYPASS feature and it is dead code to us, and (b) that removes the dependency from libssl on freebl, (c) it would be better to combine freebl with softoken than with the rest of NSS. In particular, if you avoided folding softoken in with the rest because of the issues discussed above, then it is important to know that those issues apply to freebl, as softoken+freebl are really one thing.
Comment 20 Wan-Teh Chang 2013-02-07 18:40:24 PST
Comment on attachment 711317 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together

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

::: security/build/Makefile.in
@@ +13,5 @@
>  CXX_WRAPPER =
>  
> +ifdef MOZ_FOLD_LIBS
> +MODULE = nss
> +LIBRARY_NAME = nss3

You should use a different library name to avoid confusion
with the real nss3 shared library/DLL.

@@ +368,5 @@
> +# Add all static libraries for nss, smime, ssl and nssutil
> +SHARED_LIBRARY_LIBS = $(addprefix ../,$(NSS_STATIC_LIBS))
> +# as well as freebl (yes, it does feel silly that we have a shared freebl
> +# library *and* statically link it ; that's how NSS is built currently)
> +SHARED_LIBRARY_LIBS += ../nss/lib/freebl/$(LIB_PREFIX)freebl.$(LIB_SUFFIX)

The freebl static library is a small stub that loads the
freebl shared library dynamically. You can prevent libssl
from linking with the freebl static library by
passing NSS_NO_PKCS11_BYPASS=1 on the NSS make command
line:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/config.mk&rev=1.33&mark=14-22#14
Comment 21 Mike Hommey [:glandium] 2013-02-08 00:40:45 PST
(In reply to Wan-Teh Chang from comment #20)
> Comment on attachment 711317 [details] [diff] [review]
> Allow to fold NSPR, NSS and SQLite libraries all together
> 
> Review of attachment 711317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/build/Makefile.in
> @@ +13,5 @@
> >  CXX_WRAPPER =
> >  
> > +ifdef MOZ_FOLD_LIBS
> > +MODULE = nss
> > +LIBRARY_NAME = nss3
> 
> You should use a different library name to avoid confusion
> with the real nss3 shared library/DLL.

It's not possible because of what i mention in comment 14.


(In reply to Brian Smith (:bsmith) from comment #19)
> Mike, you should build libssl with NO_PKCS11_BYPASS defined because (a) we
> don't use the BYPASS feature and it is dead code to us, and (b) that removes
> the dependency from libssl on freebl, (c) it would be better to combine
> freebl with softoken than with the rest of NSS. In particular, if you
> avoided folding softoken in with the rest because of the issues discussed
> above, then it is important to know that those issues apply to freebl, as
> softoken+freebl are really one thing.

I'm actually not touching how softoken and freebl are built (nor nssckbi).
Comment 22 Mike Hommey [:glandium] 2013-02-08 01:02:21 PST
Created attachment 711740 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together

With NSS_NO_PKCS11_BYPASS
Comment 23 Mike Hommey [:glandium] 2013-02-08 05:47:13 PST
Created attachment 711791 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together

Turns out i had something to fold that i folded on an unrelated patch, locally. Here it is.
Comment 24 Wan-Teh Chang 2013-02-08 09:54:39 PST
(In reply to Mike Hommey [:glandium] from comment #14)
>
> The lib is still called nss3 for two main reasons:
> - some part of NSS uses PR_GetLibraryFilePathname, and gives it the nss3
> library name, and that is hardcoded in NSS at the moment

We should fix this by using a macro for the library name in those NSS files.

I also remember another hardcoded library name in NSS:
http://mxr.mozilla.org/security/search?string=%22smime3

That code is probably unreadable or not used. Someone needs to verify that.
Comment 25 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-02-08 11:54:17 PST
(In reply to Mike Hommey [:glandium] from comment #21)
> I'm actually not touching how softoken and freebl are built (nor nssckbi).

OK. After seeing Wan-Teh's comments, I realize you meant that your patch linked the static freebl stub library into the combined library, not the freebl code that goes into the shared library.

I think it is a good idea to combine softoken and freebl together into one shared library, either in this bug or in a follow-up.
Comment 26 Ted Mielczarek [:ted.mielczarek] 2013-02-13 12:09:03 PST
Comment on attachment 711193 [details] [diff] [review]
Support expandlibs_exec.py --extract on windows

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

::: config/expandlibs_exec.py
@@ +70,5 @@
>              if os.path.splitext(arg)[1] == conf.LIB_SUFFIX:
>                  if os.path.exists(arg + conf.LIBS_DESC_SUFFIX):
>                      newlist += self._extract(self._expand_desc(arg))
> +                    continue
> +                elif os.path.exists(arg) and (len(ar_extract) or conf.AR == 'lib'):

I would probably feel more comfortable if this was more of a feature check than an exact string match, but I suppose this is not very likely to be broken.

@@ +75,4 @@
>                      tmp = tempfile.mkdtemp(dir=os.curdir)
>                      self.tmp.append(tmp)
> +                    if conf.AR == 'lib':
> +                        out = subprocess.check_output([conf.AR, '-NOLOGO', '-LIST', arg])

Seems a little weird that we have AR_EXTRACT in buildconfig but these arguments hardcoded here (although I guess it's not as trivial as AR_EXTRACT).
Comment 27 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-02-13 12:29:12 PST
Intead of making this conditional on MOZ_FOLD_LIBS, why not make this the default configuration in NSS's build system for the given platforms? That way, everybody will be testing and using the same configuration, except on Linux. And, in particular, because NSS is only having its automated tests run as part of the separate NSS build/test automation, that's the only way that this code would get automatically tested, at least for the near future.
Comment 28 Mike Hommey [:glandium] 2013-02-13 14:14:39 PST
(In reply to Brian Smith (:bsmith) from comment #27)
> Intead of making this conditional on MOZ_FOLD_LIBS, why not make this the
> default configuration in NSS's build system for the given platforms? That
> way, everybody will be testing and using the same configuration, except on
> Linux. And, in particular, because NSS is only having its automated tests
> run as part of the separate NSS build/test automation, that's the only way
> that this code would get automatically tested, at least for the near future.

Because it's a first step, and we'll be folding other things soon enough, and that won't be doable from NSS's build system.
Comment 29 Benjamin Smedberg [:bsmedberg] 2013-02-21 12:12:53 PST
Comment on attachment 711540 [details] [diff] [review]
Fold NSPR, NSS and SQLite libraries all together on B2G, Android, OSX and Windows

What are the measured benefits of this folding? And it's somewhat likely that this will affect the few vendors who are trying to build multi-version Firefox binaries still (Norton toolbar and Skype are the only ones left I know of). I'm willing to force them to work around this if the perf benefit is great enough, but after having read the bug, I don't see exactly what this gives us.

This affects the libraries that a binary component author would have to link against, correct? If so, we should make the same change in XULRunner as we make in Firefox so that when we make the SDK the link flags are correct.
Comment 30 Ted Mielczarek [:ted.mielczarek] 2013-02-22 12:18:01 PST
Comment on attachment 711791 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together

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

I have to be honest, this patch makes my eyes glaze over in security/build/Makefile.in. I'm ok with the concept, but I'm worried that it makes that file essentially unmaintainable.

::: b2g/installer/Makefile.in
@@ +31,5 @@
>  DEFINES += -DJAREXT=
>  
> +ifdef MOZ_FOLD_LIBS
> +DEFINES += -DMOZ_FOLD_LIBS=1
> +endif

Any reason not to just AC_DEFINE this?

::: security/build/Makefile.in
@@ +15,5 @@
> +ifdef MOZ_FOLD_LIBS
> +MODULE = nss
> +LIBRARY_NAME = nss3
> +FORCE_SHARED_LIB = 1
> +endif

So you're folding NSPR, sqlite and NSS all into a "libnss"? We should probably call this "moznss" or something like that.

@@ +380,5 @@
> +	echo LIBRARY nss3$(DLL_SUFFIX) > $@.tmp
> +	echo EXPORTS >> $@.tmp
> +	grep -v -h -e ^LIBRARY -e ^EXPORTS -e ^\; $^ >> $@.tmp
> +	mv $@.tmp $@
> +endif

I'd probably feel better about a little Python script to do this munging, even if it's not terribly complicated.

@@ +441,5 @@
> +
> +# For each directory where we build static libraries, force the NSS build system
> +# to only build static libraries.
> +$(addprefix libs-,$(NSS_STATIC_DIRS)): DEFAULT_GMAKE_FLAGS += SHARED_LIBRARY= IMPORT_LIBRARY=
> +endif # MOZ_FOLD_LIBS

This is all super painful. Are there changes we could make to NSS to make this simpler?
Comment 31 (dormant account) 2013-02-22 12:22:46 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> Comment on attachment 711540 [details] [diff] [review]
> Fold NSPR, NSS and SQLite libraries all together on B2G, Android, OSX and
> Windows
> 
> What are the measured benefits of this folding? And it's somewhat likely
> that this will affect the few vendors who are trying to build multi-version
> Firefox binaries still (Norton toolbar and Skype are the only ones left I
> know of). I'm willing to force them to work around this if the perf benefit
> is great enough, but after having read the bug, I don't see exactly what
> this gives us.
> 
This gets us close to chrome parity for cold startup.
Comment 32 Mike Hommey [:glandium] 2013-02-22 13:41:40 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #30)
> Comment on attachment 711791 [details] [diff] [review]
> Allow to fold NSPR, NSS and SQLite libraries all together
> 
> Review of attachment 711791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have to be honest, this patch makes my eyes glaze over in
> security/build/Makefile.in. I'm ok with the concept, but I'm worried that it
> makes that file essentially unmaintainable.
> 
> ::: b2g/installer/Makefile.in
> @@ +31,5 @@
> >  DEFINES += -DJAREXT=
> >  
> > +ifdef MOZ_FOLD_LIBS
> > +DEFINES += -DMOZ_FOLD_LIBS=1
> > +endif
> 
> Any reason not to just AC_DEFINE this?

Essentially because I'm not a fan of shipping such things in the SDK through mozilla-config.h.

> ::: security/build/Makefile.in
> @@ +15,5 @@
> > +ifdef MOZ_FOLD_LIBS
> > +MODULE = nss
> > +LIBRARY_NAME = nss3
> > +FORCE_SHARED_LIB = 1
> > +endif
> 
> So you're folding NSPR, sqlite and NSS all into a "libnss"? We should
> probably call this "moznss" or something like that.

See comment 14.
 
> @@ +380,5 @@
> > +	echo LIBRARY nss3$(DLL_SUFFIX) > $@.tmp
> > +	echo EXPORTS >> $@.tmp
> > +	grep -v -h -e ^LIBRARY -e ^EXPORTS -e ^\; $^ >> $@.tmp
> > +	mv $@.tmp $@
> > +endif
> 
> I'd probably feel better about a little Python script to do this munging,
> even if it's not terribly complicated.
> 
> @@ +441,5 @@
> > +
> > +# For each directory where we build static libraries, force the NSS build system
> > +# to only build static libraries.
> > +$(addprefix libs-,$(NSS_STATIC_DIRS)): DEFAULT_GMAKE_FLAGS += SHARED_LIBRARY= IMPORT_LIBRARY=
> > +endif # MOZ_FOLD_LIBS
> 
> This is all super painful. Are there changes we could make to NSS to make
> this simpler?

Changing its build system?
Comment 33 Ted Mielczarek [:ted.mielczarek] 2013-02-22 17:22:33 PST
(In reply to Mike Hommey [:glandium] from comment #32)
> > Any reason not to just AC_DEFINE this?
> 
> Essentially because I'm not a fan of shipping such things in the SDK through
> mozilla-config.h.

Ok, can we at least put this in config.mk?

> > This is all super painful. Are there changes we could make to NSS to make
> > this simpler?
> 
> Changing its build system?

Right, I mean, are there specific things we can fix in the NSS build system to make this easier?
Comment 34 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-02-22 17:37:12 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #33)
> I mean, are there specific things we can fix in the NSS build system
> to make this easier?

I am willing to help with reviewing changes to the NSS build system to make this task easier.

I would like as much of the the stuff that is in security/build to be moved into the NSS build system (especially the stuff to support Gecko-style objdirs). We already added Android support to the NSS build system so some of the Android stuff in security/build is (should be) unnecessary now (See bug 815912).

IMO, it is 100% beneficial to have the default configuration in NSS trunk match the configuration used in Gecko as much as possible. Even if we have to do the linking on our own (because we're going to fold more non-NSS stuff in, or combine NSPR and NSS), it is still useful to put all the support for the compilation stages into the NSPR and NSS build systems.

In particular, people who work on NSS, including Gecko contributors, use the NSS build system to build NSS more than we use the Gecko build system, so it would benefit us much more to have the parallel building work done within the NSS build system rather than in the Gecko build system, if at all practical.
Comment 35 Mike Hommey [:glandium] 2013-02-25 07:42:00 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #33)
> (In reply to Mike Hommey [:glandium] from comment #32)
> > > Any reason not to just AC_DEFINE this?
> > 
> > Essentially because I'm not a fan of shipping such things in the SDK through
> > mozilla-config.h.
> 
> Ok, can we at least put this in config.mk?

We don't do that for other variables used in package-manifest.in, why do it for this one?

> > Changing its build system?
> 
> Right, I mean, are there specific things we can fix in the NSS build system
> to make this easier?

I will file bugs for the necessary features in the NSS build system.
Comment 36 Ted Mielczarek [:ted.mielczarek] 2013-02-25 08:07:00 PST
(In reply to Mike Hommey [:glandium] from comment #35)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #33)
> > (In reply to Mike Hommey [:glandium] from comment #32)
> > > > Any reason not to just AC_DEFINE this?
> > > 
> > > Essentially because I'm not a fan of shipping such things in the SDK through
> > > mozilla-config.h.
> > 
> > Ok, can we at least put this in config.mk?
> 
> We don't do that for other variables used in package-manifest.in, why do it
> for this one?

The creep has finally started to bother me. It's not the end of the world, maybe just file a followup on cleaning that up?

> > > Changing its build system?
> > 
> > Right, I mean, are there specific things we can fix in the NSS build system
> > to make this easier?
> 
> I will file bugs for the necessary features in the NSS build system.

Okay, I'd appreciate it if you could file the bugs and put the bug numbers in TODO comments in the source as per usual.
Comment 37 Mike Hommey [:glandium] 2013-02-25 08:11:33 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #36)
> The creep has finally started to bother me. It's not the end of the world,
> maybe just file a followup on cleaning that up?

I guess most of the clutter would be addressed by bug 526333.
Comment 38 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-02-25 10:26:15 PST
Comment on attachment 711540 [details] [diff] [review]
Fold NSPR, NSS and SQLite libraries all together on B2G, Android, OSX and Windows

Sorry for review lag.  Sounds like a beneficial change perf-wise, but we don't have perf tests that would catch regressions here, so we'll see how things feel post-land.
Comment 39 Mike Hommey [:glandium] 2013-03-03 23:50:56 PST
Created attachment 720551 [details] [diff] [review]
Support expandlibs_exec.py --extract on windows

Refreshed after bug 812179
Comment 40 Mike Hommey [:glandium] 2013-03-03 23:56:28 PST
Comment on attachment 720551 [details] [diff] [review]
Support expandlibs_exec.py --extract on windows

Carrying over r+
Comment 41 Mike Hommey [:glandium] 2013-03-04 00:01:20 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #38)
> Sorry for review lag.  Sounds like a beneficial change perf-wise, but we
> don't have perf tests that would catch regressions here, so we'll see how
> things feel post-land.

mwu mentioned https://github.com/mozilla/Negatus as something that might break.
Comment 42 Mike Hommey [:glandium] 2013-03-04 00:50:47 PST
Created attachment 720567 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together
Comment 43 Ted Mielczarek [:ted.mielczarek] 2013-03-15 08:48:07 PDT
Comment on attachment 720567 [details] [diff] [review]
Allow to fold NSPR, NSS and SQLite libraries all together

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

::: config/nspr/Makefile.in
@@ +22,5 @@
>  # Copy NSPR to the SDK
>  ABS_DIST = $(call core_abspath,$(DIST))
>  
> +ifdef MOZ_FOLD_LIBS
> +EXTRA_MAKE_FLAGS := SHARED_LIBRARY= IMPORT_LIBRARY= SHARED_LIB_PDB=

Can you file a bug on adding support to NSPR's build system for this?

::: configure.in
@@ +3965,5 @@
>  if test -n "$MOZ_NATIVE_NSS"; then
>     NSS_LIBS="$NSS_LIBS -lcrmf"
>  else
>     NSS_CFLAGS='-I$(LIBXUL_DIST)/include/nss'
> +   if test -z "$IMPORT_LIB_SUFFIX"; then

We could ostensibly define a new variable for this and just use that instead, right? It'd be IMPORT_LIB_SUFFIX on Windows and DLL_SUFFIX otherwise.

::: security/build/nss.mk
@@ +1,1 @@
> +include $(DEPTH)/config/autoconf.mk

Wants a license header.

@@ +19,5 @@
> +endef
> +$(foreach dir,$(dirs),$(eval $(call add_lib,$(dir))))
> +
> +echo-variable-%:
> +	@echo $($*)

This is horrible. :-/ Will we be able to push some of this into the NSS build?
Comment 45 Mike Hommey [:glandium] 2013-03-17 01:47:12 PDT
Fixup for windows
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd1c5d03676
Comment 47 Nick Thomas [:nthomas] 2013-03-17 18:17:05 PDT
glandium, perhaps you're missing a change to browser/installer/removed-files.in to match the desktop package-manifest.in ? What happens if a user gets a complete update to a MOZ_FOLD_LIBS build that doesn't remove the old files ?
Comment 48 Alex Keybl [:akeybl] 2013-03-29 13:14:20 PDT
No clear user/web impact, so no need to release note for FF22.
Comment 49 Max 2013-05-01 09:07:07 PDT
(In reply to Alex Keybl [:akeybl] from comment #48)
> No clear user/web impact, so no need to release note for FF22.

It seems to me that this change, as reported by many people in the comments, will, in fact, IMPACT ON MANY USERS and EXTENSION DEVELOPERS. 

I still advocate for not enabling this by default at least for another 2/3 releases (maybe FF 25 is a good candidate because it will be the first after the ESR).

Therefore, there is ABSOLUTELY the necessity to, at least:
(1) Inform developers about the change and the possible impact of the availability of libraries.
(2) Write documentation on which symbols will be available. In particular, which calls from the folded libraries will be available to extensions and which ones will not be available anymore

I still think this is a bad decision and more regards to backward compatibility should be there. I still wish that the project managers at Mozilla will give more thoughts about the importance of providing what Chrome can not - backward compatibility. We have (somehow) managed to go through changes in the interfaces.. but this change does introduce differences among platforms so that it might not be possible to support, in one extension, different platforms because of the differences in Firefox for Windows vs Firefox for Linux.

Please Please Please - Reconsider including this patch in FF 22... or, if that is not possible, at least provide EXTENSIVE documentation about the change so that people who still want to support Firefox can do it.
Comment 50 Mike Hommey [:glandium] 2013-05-01 10:29:28 PDT
(In reply to Max from comment #49)
> (In reply to Alex Keybl [:akeybl] from comment #48)
> > No clear user/web impact, so no need to release note for FF22.
> 
> It seems to me that this change, as reported by many people in the comments,
> will, in fact, IMPACT ON MANY USERS 

how so?

> and EXTENSION DEVELOPERS. 

Extension developers are either using these libraries directly with a binary component, which they need to rebuild for each new release of Firefox anyways, or through ctypes, in which case they need to adapt, and that's not a lot of work to do, and I doubt so many extensions are actually doing that.
Comment 51 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-01 11:38:43 PDT
(In reply to Max from comment #49)
> (2) Write documentation on which symbols will be available. In particular,
> which calls from the folded libraries will be available to extensions and
> which ones will not be available anymore

Right now, all symbols will be available. However, in the future, many fewer symbols will be available because I am planning to remove all the symbols that aren't being directly used by Gecko. (I already have a patch to do this). I will post on dev.tech.crypto soon about this.

Basically, NSS is an implementation detail of Gecko. And, just like the rest of Gecko, we (Mozilla) don't promise a stable API for it or that changes to NSS will affect Gecko. For example, if we were to switch from NSS to something else for certificate validation, then it is likely that many certificate-validation-related things would go away and/or that changes to the NSS certificate databases may no longer have an effect on Firefox.

Now, there is a lot of work being done on PSM and NSS-related parts of Gecko. An unfortunate side effect of that is that there will be a period of instability of APIs. Hopefully things will calm down in six months to a year. Even outside of NSS-specific changes, there are going to be significant changes/reductions to the PSM API. See bug 775698, for example.
Comment 52 Denis 2013-05-13 05:01:23 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> ...
> This affects the libraries that a binary component author would have to link
> against, correct? If so, we should make the same change in XULRunner as we
> make in Firefox so that when we make the SDK the link flags are correct.

You are right. I already faced this problem and filed the bug
https://bugzilla.mozilla.org/show_bug.cgi?id=865655
Please someone make the change for SDK too.

Note You need to log in before you can comment on or make changes to this bug.