Enable peerconnection to use the NSS RNG.

Enable peerconnection to use the NSS RNG.

TEST=manually with apprtc.appspot.com
BUG=http://code.google.com/p/webrtc/issues/detail?id=591

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162055

[ekr, 2012-10-10 23:48:34]

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:356: // Init NSS which
will be needed by PeerConnection.
I may have lost the thread but I thought we needed ForceNoNSSDbInit() here.

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:363: 
Whitespace only change.

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
third_party/libjingle/libjingle.gyp:25: 'SSL_USE_NSS',
Is this what we want? I notice that in sslconfig.h SSL_USE_OPENSSL is currently
defined as HAVE_OPENSSL_H. Do you want instead to replicate that for NSS? I.e.,

#define SSL_USE_NSS HAVE_NSS_SSL_H

and then define HAVE_NSS_SSL_H in libjingle.gyp?

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
third_party/libjingle/libjingle.gyp:46: '<(DEPTH)/crypto/crypto.gyp:crypto',
Is this needed now? I guess it will be needed when you land the rest of the
patch?

[Sergey Ulanov, 2012-10-10 23:59:00]

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:357:
net::EnsureNSSSSLInit();
NSS is initialized in
ChromeRenderProcessObserver::ChromeRenderProcessObserver(). maybe it's better to
change that function to call net::EnsureNSSSSLInit() instead of
crypto::EnsureNSSInit()

[Ryan Sleevi, 2012-10-10 23:59:15]

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:22: #include
"net/socket/nss_ssl_util.h"
#if !defined(USE_OPENSSL)
#include "net/socket/nss_ssl_util.h"
#endif

(moved to line 27)

Not sure if Android is using this code yet, but Android won't have this function

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:357:
net::EnsureNSSSSLInit();
#if !defined(USE_OPENSSL)
// Init NSS which will be needed by PeerConnection
net::EnsureNSSSSLInit();
#endif

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
third_party/libjingle/libjingle.gyp:25: 'SSL_USE_NSS',
Pretty sure you want this in a 'conditions' block (line 135)

['OS!="android"', {
  ...
}]

Probably the same with line 46

[Ryan Sleevi, 2012-10-11 00:00:47]

On 2012/10/10 23:59:00, sergeyu wrote:
>
https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
> File content/renderer/media/media_stream_dependency_factory.cc (right):
> 
>
https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
> content/renderer/media/media_stream_dependency_factory.cc:357:
> net::EnsureNSSSSLInit();
> NSS is initialized in
> ChromeRenderProcessObserver::ChromeRenderProcessObserver(). maybe it's better
to
> change that function to call net::EnsureNSSSSLInit() instead of
> crypto::EnsureNSSInit()

Please don't.

SSL init allocates a number of structures (eg: SSL session cache) that it would
be nice to not have to initialize for every process. Unless and until we know
every renderer needs it, I'd much rather avoid such init.

It's currently done in ChromeRenderProcessObserver because it's *prior* to the
sandbox and is used to ensure the SOs get loaded before the sandbox is closed.
It's the absolute least amount of work we can do to prime the sandbox.

[Ronghua Wu (Left Chromium), 2012-10-11 00:17:18]

Thanks.

What else platforms we don't have NSS? I also got the errors on linux_clang
build, not sure why.

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:22: #include
"net/socket/nss_ssl_util.h"
On 2012/10/10 23:59:15, Ryan Sleevi wrote:
> #if !defined(USE_OPENSSL)
> #include "net/socket/nss_ssl_util.h"
> #endif
> 
> (moved to line 27)
> 
> Not sure if Android is using this code yet, but Android won't have this
function

Done.

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:356: // Init NSS which
will be needed by PeerConnection.
On 2012/10/10 23:48:34, ekr wrote:
> I may have lost the thread but I thought we needed ForceNoNSSDbInit() here.

Last reply from Ryan seems to say the ForceNSSNoDBInit is not needed. Maybe I
took it wrong.

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:357:
net::EnsureNSSSSLInit();
On 2012/10/10 23:59:15, Ryan Sleevi wrote:
> #if !defined(USE_OPENSSL)
> // Init NSS which will be needed by PeerConnection
> net::EnsureNSSSSLInit();
> #endif

Done.

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:363: 
On 2012/10/10 23:48:34, ekr wrote:
> Whitespace only change.

Done.

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
third_party/libjingle/libjingle.gyp:25: 'SSL_USE_NSS',
On 2012/10/10 23:48:34, ekr wrote:
> Is this what we want? I notice that in sslconfig.h SSL_USE_OPENSSL is
currently
> defined as HAVE_OPENSSL_H. Do you want instead to replicate that for NSS?
I.e.,
> 
> #define SSL_USE_NSS HAVE_NSS_SSL_H
> 
> and then define HAVE_NSS_SSL_H in libjingle.gyp?

I don't know exactly what is the purpose of HAVE_OPENSSL_SSL_H if we already
have the SSL_USE_OPENSSL.

I will check with Justin.

https://codereview.chromium.org/11093061/diff/1/third_party/libjingle/libjing...
third_party/libjingle/libjingle.gyp:46: '<(DEPTH)/crypto/crypto.gyp:crypto',
On 2012/10/10 23:48:34, ekr wrote:
> Is this needed now? I guess it will be needed when you land the rest of the
> patch?

This is needed because in the helper.cc, I've added a NSS RNG.

https://code.google.com/p/libjingle/source/browse/trunk/talk/base/helpers.cc#37
https://code.google.com/p/libjingle/source/browse/trunk/talk/base/helpers.cc#108

[Ryan Sleevi, 2012-10-11 00:26:06]

I'm not sure the linux_clang failure. crypto.gyp has 'export_dependent_settings'
that exports the dependencies on NSS & NSPR, and build/linux/system.gyp:ssl has
the proper link_settings to force a dependency on NSS & NSPR.

I'm wondering if it's some sort of conflict with the processing order of
target_defaults and dependency processing... For sake of testing, if it still
fails, can you directly add it to the relevant target(s) and see if it's any
better?

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:356: // Init NSS which
will be needed by PeerConnection.
On 2012/10/11 00:17:18, Ronghua Wu wrote:
> On 2012/10/10 23:48:34, ekr wrote:
> > I may have lost the thread but I thought we needed ForceNoNSSDbInit() here.
> 
> Last reply from Ryan seems to say the ForceNSSNoDBInit is not needed. Maybe I
> took it wrong.

No, you are correct, it is not needed.

If/when it is, the ChromeRenderProcessObserver should be doing it, as part of
the sandbox prep.

https://codereview.chromium.org/11093061/diff/11002/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/11002/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:26: #if
!defined(USE_OPENSSL)
nit: linebreak between lines 25 and 26 (
http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code )

https://codereview.chromium.org/11093061/diff/11002/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:359: // Init NSS which
will be needed by PeerConnection
nit: add comma

// Init NSS, which is used by PeerConnection.

https://codereview.chromium.org/11093061/diff/11002/third_party/libjingle/lib...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/11002/third_party/libjingle/lib...
third_party/libjingle/libjingle.gyp:140: ],
nit: indent is off here (lines 138-140)

[Ronghua Wu (Left Chromium), 2012-10-11 00:34:29]

https://codereview.chromium.org/11093061/diff/11002/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/11002/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:26: #if
!defined(USE_OPENSSL)
On 2012/10/11 00:26:06, Ryan Sleevi wrote:
> nit: linebreak between lines 25 and 26 (
> http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code )

Done.

https://codereview.chromium.org/11093061/diff/11002/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:359: // Init NSS which
will be needed by PeerConnection
On 2012/10/11 00:26:06, Ryan Sleevi wrote:
> nit: add comma
> 
> // Init NSS, which is used by PeerConnection.

Done.

https://codereview.chromium.org/11093061/diff/11002/third_party/libjingle/lib...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/11002/third_party/libjingle/lib...
third_party/libjingle/libjingle.gyp:140: ],
On 2012/10/11 00:26:06, Ryan Sleevi wrote:
> nit: indent is off here (lines 138-140)

Done.

[Ronghua Wu (Left Chromium), 2012-10-11 17:46:24]

The linux_clange gives me the linking error on remoting_me2me_host and
remoting_simple_host, so I tried to add the dependency directly to crypto see
how it goes.

The linux_chromeos gives the error:
libcontent.so: error: undefined reference to 'net::EnsureNSSSSLInit()'

I changed #if !defined(USE_OPENSSL) to #if defined(USE_NSS), does it make sense?

PTAL

[Ronghua Wu (Left Chromium), 2012-10-11 18:00:54]

Both of my attempt to fix the build errors failed with the same errors. :(

For the clang issue, I couldn't reproduce locally with GYP_DEFINES=clang=1. Any
suggestions?

For the chromeos issue, only guess is the "use_openssl" is defined so that the
nss_ssl_util.cc is removed from the build:
http://code.google.com/searchframe#OAMlx_jo-ck/src/net/net.gyp&type=cs&l=942
However if that's the case, we shouldn't even have called EnsureNSSSSLInit.

[Ryan Sleevi, 2012-10-11 18:01:47]

On 2012/10/11 17:46:24, Ronghua Wu wrote:
> The linux_clange gives me the linking error on remoting_me2me_host and
> remoting_simple_host, so I tried to add the dependency directly to crypto see
> how it goes.
> 
> The linux_chromeos gives the error:
> libcontent.so: error: undefined reference to 'net::EnsureNSSSSLInit()'
> 
> I changed #if !defined(USE_OPENSSL) to #if defined(USE_NSS), does it make
sense?
> 
> PTAL

No, USE_NSS is not the mirror of USE_OPENSSL. USE_NSS is "Use NSS for trust
anchor management", while "USE_OPENSSL" is "Use OpenSSL for all crypto"

[Ryan Sleevi, 2012-10-11 18:02:34]

On Thu, Oct 11, 2012 at 11:00 AM,  <ronghuawu@chromium.org> wrote:
> Both of my attempt to fix the build errors failed with the same errors. :(
>
> For the clang issue, I couldn't reproduce locally with GYP_DEFINES=clang=1.
> Any
> suggestions?
>
> For the chromeos issue, only guess is the "use_openssl" is defined so that
> the
> nss_ssl_util.cc is removed from the build:
> http://code.google.com/searchframe#OAMlx_jo-ck/src/net/net.gyp&type=cs&l=942
> However if that's the case, we shouldn't even have called EnsureNSSSSLInit.
>
>
>
> https://codereview.chromium.org/11093061/

The only time USE_OPENSSL is defined on Android. So no, it's not
possible that use_openssl is defined.

[Sergey Ulanov, 2012-10-11 18:18:28]

On 2012/10/11 18:00:54, Ronghua Wu wrote:
> Both of my attempt to fix the build errors failed with the same errors. :(
> 
> For the clang issue, I couldn't reproduce locally with GYP_DEFINES=clang=1.
Any
> suggestions?
> 

I think you should be able to reproduce it with component build:
http://www.chromium.org/developers/how-tos/component-build 

To fix it you need to add direct dependency on "build/linux/system.gyp:ssl" for
libjingle, otherwise linker doesn't know that it needs to link with NSS
libraries. 

BTW it's good idea to always use clang and component build by default - you'll
get shorter build time and better error reporting.


> For the chromeos issue, only guess is the "use_openssl" is defined so that the
> nss_ssl_util.cc is removed from the build:
> http://code.google.com/searchframe#OAMlx_jo-ck/src/net/net.gyp&type=cs&l=942
> However if that's the case, we shouldn't even have called EnsureNSSSSLInit.

[wtc, 2012-10-11 18:22:16]

Review comments on patch set 5:

https://codereview.chromium.org/11093061/diff/9006/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/9006/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:27: #if
defined(USE_NSS)

The two ifdefs you added should be either

  #if !defined(USE_OPENSSL)

or

  #if defined(USE_NSS) || defined(OS_MACOSX) || defined(OS_WIN)

The former is more concise.

USE_NSS is only relevent to Linux (including Chrome OS)
and Android. This is why defined(OS_MACOSX) and defined(OS_WIN)
are necessary if you check defined(USE_NSS). I understand
this is complicated. Sorry about that.

[ekr, 2012-10-11 18:29:48]

https://codereview.chromium.org/11093061/diff/9006/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/9006/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:27: #if
defined(USE_NSS)
I wonder if what you want is SSL_USE_OPENSSL and SSL_USE_NSS (in libjingle's
sslconfig.h). We could then arrange for these to be set properly by the platform

[Ryan Sleevi, 2012-10-11 18:40:00]

On Thu, Oct 11, 2012 at 11:18 AM,  <sergeyu@chromium.org> wrote:
> On 2012/10/11 18:00:54, Ronghua Wu wrote:
>>
>> Both of my attempt to fix the build errors failed with the same errors. :(
>
>
>> For the clang issue, I couldn't reproduce locally with
>> GYP_DEFINES=clang=1.
>
> Any
>>
>> suggestions?
>
>
>
> I think you should be able to reproduce it with component build:
> http://www.chromium.org/developers/how-tos/component-build
>
> To fix it you need to add direct dependency on "build/linux/system.gyp:ssl"
> for
> libjingle, otherwise linker doesn't know that it needs to link with NSS
> libraries.

sergey: crypto.gyp is using export_dependent_settings to directly
export (to all of crypto's dependents) a dependency on NSS, which uses
'direct_dependent_settings' to modify the link_settings.

As such, it should not be *necessary* to depend on
"build/linux.system.gyp:ssl". That it is suggests a bug in either GYP
or in Chromium's GYP files (it should be necessary for dependencies to
be transitively exported). I suspect I will need to look into this.

>
> BTW it's good idea to always use clang and component build by default -
> you'll
> get shorter build time and better error reporting.
>
>
>
>> For the chromeos issue, only guess is the "use_openssl" is defined so that
>> the
>> nss_ssl_util.cc is removed from the build:
>>
>> http://code.google.com/searchframe#OAMlx_jo-ck/src/net/net.gyp&type=cs&l=942
>> However if that's the case, we shouldn't even have called
>> EnsureNSSSSLInit.
>
>
>
> https://codereview.chromium.org/11093061/

[Ronghua Wu (Left Chromium), 2012-10-11 21:13:53]

Please see my changes to the libjingle.gyp where I basically added the crypo
dependencies. This should fix the undefined problem for libjingle. But we will
need to do similar in content_renderer.gypi.

Maybe I should wait for Ryan's investigation on the gyp?

https://codereview.chromium.org/11093061/diff/9006/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11093061/diff/9006/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:27: #if
defined(USE_NSS)
On 2012/10/11 18:22:16, wtc wrote:
> 
> The two ifdefs you added should be either
> 
>   #if !defined(USE_OPENSSL)
> 
> or
> 
>   #if defined(USE_NSS) || defined(OS_MACOSX) || defined(OS_WIN)
> 
> The former is more concise.
> 
> USE_NSS is only relevent to Linux (including Chrome OS)
> and Android. This is why defined(OS_MACOSX) and defined(OS_WIN)
> are necessary if you check defined(USE_NSS). I understand
> this is complicated. Sorry about that.

Done.

[Sergey Ulanov, 2012-10-12 18:11:38]

On Thu, Oct 11, 2012 at 11:39 AM, Ryan Sleevi <rsleevi@chromium.org> wrote:

> On Thu, Oct 11, 2012 at 11:18 AM,  <sergeyu@chromium.org> wrote:
> > On 2012/10/11 18:00:54, Ronghua Wu wrote:
> >>
> >> Both of my attempt to fix the build errors failed with the same errors.
> :(
> >
> >
> >> For the clang issue, I couldn't reproduce locally with
> >> GYP_DEFINES=clang=1.
> >
> > Any
> >>
> >> suggestions?
> >
> >
> >
> > I think you should be able to reproduce it with component build:
> > http://www.chromium.org/developers/how-tos/component-build
> >
> > To fix it you need to add direct dependency on
> "build/linux/system.gyp:ssl"
> > for
> > libjingle, otherwise linker doesn't know that it needs to link with NSS
> > libraries.
>
> sergey: crypto.gyp is using export_dependent_settings to directly
> export (to all of crypto's dependents) a dependency on NSS, which uses
> 'direct_dependent_settings' to modify the link_settings.
>
> As such, it should not be *necessary* to depend on
> "build/linux.system.gyp:ssl". That it is suggests a bug in either GYP
> or in Chromium's GYP files (it should be necessary for dependencies to
> be transitively exported). I suspect I will need to look into this.
>

I believe that GYP behavior is correct in this case. It does propagate
dependency for static builds. But in case of component build crypto is
compiled to libcrccrypto.so, and libcrccrypto.so is linked with NSS
libraries. Because it is a dynamically-linked library, GYP doesn't need
to propagate crypto dependencies to the targets that depend on crypto. With
that change libjingle calls NSS directly and so we need to specify NSS
dependency explicitly.


>
> >
> > BTW it's good idea to always use clang and component build by default -
> > you'll
> > get shorter build time and better error reporting.
> >
> >
> >
> >> For the chromeos issue, only guess is the "use_openssl" is defined so
> that
> >> the
> >> nss_ssl_util.cc is removed from the build:
> >>
> >>
> http://code.google.com/searchframe#OAMlx_jo-ck/src/net/net.gyp&type=cs&l=942
> >> However if that's the case, we shouldn't even have called
> >> EnsureNSSSSLInit.
> >
> >
> >
> > https://codereview.chromium.org/11093061/
>

[Ronghua Wu (Left Chromium), 2012-10-12 19:06:15]

Another problem is also in the components build.

We called net::EnsureNSSSSLInit in media_stream_dependency_factory.cc, and
as you can see the net.gyp:net is the dependency. But I got error when
building perf_tests (basically any target that depends on content). I got:
 ../../third_party/gold/gold64: lib/libcontent.so: error: undefined
reference to 'net::EnsureNSSSSLInit()'

$ nm out/Debug/lib/libcontent.so  | grep EnsureNSSSSLInit
                U _ZN3net16EnsureNSSSSLInitEv
$ nm out/Debug/lib/libnet.so  | grep EnsureNSSSSLInit
0000000000481720 t _ZN3net16EnsureNSSSSLInitEv

I also tried to add net.gyp directly in content.gyp and to the perf_tests
target, but same errors.

Thoughts?

On Fri, Oct 12, 2012 at 11:11 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:

>
>
>
> On Thu, Oct 11, 2012 at 11:39 AM, Ryan Sleevi <rsleevi@chromium.org>wrote:
>
>> On Thu, Oct 11, 2012 at 11:18 AM,  <sergeyu@chromium.org> wrote:
>> > On 2012/10/11 18:00:54, Ronghua Wu wrote:
>> >>
>> >> Both of my attempt to fix the build errors failed with the same
>> errors. :(
>> >
>> >
>> >> For the clang issue, I couldn't reproduce locally with
>> >> GYP_DEFINES=clang=1.
>> >
>> > Any
>> >>
>> >> suggestions?
>> >
>> >
>> >
>> > I think you should be able to reproduce it with component build:
>> > http://www.chromium.org/developers/how-tos/component-build
>> >
>> > To fix it you need to add direct dependency on
>> "build/linux/system.gyp:ssl"
>> > for
>> > libjingle, otherwise linker doesn't know that it needs to link with NSS
>> > libraries.
>>
>> sergey: crypto.gyp is using export_dependent_settings to directly
>> export (to all of crypto's dependents) a dependency on NSS, which uses
>> 'direct_dependent_settings' to modify the link_settings.
>>
>> As such, it should not be *necessary* to depend on
>> "build/linux.system.gyp:ssl". That it is suggests a bug in either GYP
>> or in Chromium's GYP files (it should be necessary for dependencies to
>> be transitively exported). I suspect I will need to look into this.
>>
>
> I believe that GYP behavior is correct in this case. It does propagate
> dependency for static builds. But in case of component build crypto is
> compiled to libcrccrypto.so, and libcrccrypto.so is linked with NSS
> libraries. Because it is a dynamically-linked library, GYP doesn't need
> to propagate crypto dependencies to the targets that depend on crypto. With
> that change libjingle calls NSS directly and so we need to specify NSS
> dependency explicitly.
>
Make senses. Then my changes to the libjingle.gyp is needed.

>
>
>>
>> >
>> > BTW it's good idea to always use clang and component build by default -
>> > you'll
>> > get shorter build time and better error reporting.
>> >
>> >
>> >
>> >> For the chromeos issue, only guess is the "use_openssl" is defined so
>> that
>> >> the
>> >> nss_ssl_util.cc is removed from the build:
>> >>
>> >>
>> http://code.google.com/searchframe#OAMlx_jo-ck/src/net/net.gyp&type=cs&l=942
>> >> However if that's the case, we shouldn't even have called
>> >> EnsureNSSSSLInit.
>> >
>> >
>> >
>> > https://codereview.chromium.org/11093061/
>>
>
>

[Ryan Sleevi, 2012-10-12 19:11:37]

On Fri, Oct 12, 2012 at 11:11 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:

>
>
>
> On Thu, Oct 11, 2012 at 11:39 AM, Ryan Sleevi <rsleevi@chromium.org>wrote:
>
>> On Thu, Oct 11, 2012 at 11:18 AM,  <sergeyu@chromium.org> wrote:
>> > On 2012/10/11 18:00:54, Ronghua Wu wrote:
>> >>
>> >> Both of my attempt to fix the build errors failed with the same
>> errors. :(
>> >
>> >
>> >> For the clang issue, I couldn't reproduce locally with
>> >> GYP_DEFINES=clang=1.
>> >
>> > Any
>> >>
>> >> suggestions?
>> >
>> >
>> >
>> > I think you should be able to reproduce it with component build:
>> > http://www.chromium.org/developers/how-tos/component-build
>> >
>> > To fix it you need to add direct dependency on
>> "build/linux/system.gyp:ssl"
>> > for
>> > libjingle, otherwise linker doesn't know that it needs to link with NSS
>> > libraries.
>>
>> sergey: crypto.gyp is using export_dependent_settings to directly
>> export (to all of crypto's dependents) a dependency on NSS, which uses
>> 'direct_dependent_settings' to modify the link_settings.
>>
>> As such, it should not be *necessary* to depend on
>> "build/linux.system.gyp:ssl". That it is suggests a bug in either GYP
>> or in Chromium's GYP files (it should be necessary for dependencies to
>> be transitively exported). I suspect I will need to look into this.
>>
>
> I believe that GYP behavior is correct in this case. It does propagate
> dependency for static builds. But in case of component build crypto is
> compiled to libcrccrypto.so, and libcrccrypto.so is linked with NSS
> libraries. Because it is a dynamically-linked library, GYP doesn't need
> to propagate crypto dependencies to the targets that depend on crypto. With
> that change libjingle calls NSS directly and so we need to specify NSS
> dependency explicitly.
>

I don't believe it's correct, because an explicit "link_settings" is being
exported from the transitive dependencies. While I understand that
dependencies at the shared library (or loadable_module or executable)
should no longer be transitive, an "export_dependent_settings" from such a
target should handle the proper injection into dependents. That's why I
think more investigation is needed.

The short-term turn-around is

'conditions': [
  ['os_posix == 1 and OS != "mac" and OS != "ios" and OS != "android"', {
    'dependencies': [
      '../build/linux/system.gyp:ssl',
    ],
  }],
  ['OS == "mac" or OS == "windows" or OS == "ios"', {
    'dependencies': [
      '../third_party/nss/nss.gyp:nspr',
      '../third_party/nss/nss.gyp:nss',
    ],
  }
],

Assuming that no NSS types appear in the headers, this should work. But the
export_dependent_settings trick should be exporting dependencies across
shared_library boundaries, and should not be filtering out settings link
link_settings.

[Ryan Sleevi, 2012-10-12 19:13:21]

On Fri, Oct 12, 2012 at 12:05 PM, Ronghua Wu <ronghuawu@chromium.org> wrote:

> Another problem is also in the components build.
>
> We called net::EnsureNSSSSLInit in media_stream_dependency_factory.cc,
> and as you can see the net.gyp:net is the dependency. But I got error
> when building perf_tests (basically any target that depends on content).
> I got:
>  ../../third_party/gold/gold64: lib/libcontent.so: error: undefined
> reference to 'net::EnsureNSSSSLInit()'
>
> $ nm out/Debug/lib/libcontent.so  | grep EnsureNSSSSLInit
>                 U _ZN3net16EnsureNSSSSLInitEv
> $ nm out/Debug/lib/libnet.so  | grep EnsureNSSSSLInit
> 0000000000481720 t _ZN3net16EnsureNSSSSLInitEv
>
> I also tried to add net.gyp directly in content.gyp and to the perf_tests
> target, but same errors.
>
> Thoughts?
>

net::EnsureNSSSSLInit() isn't marked as NET_EXPORT, hence it's not being
marked as a public symbol in libnet.so

Marking it exported should be fine ( much like crypto::EnsureNSSInit is
marked CRYPTO_EXPORT)


>
> On Fri, Oct 12, 2012 at 11:11 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>>
>>
>>
>> On Thu, Oct 11, 2012 at 11:39 AM, Ryan Sleevi <rsleevi@chromium.org>wrote:
>>
>>> On Thu, Oct 11, 2012 at 11:18 AM,  <sergeyu@chromium.org> wrote:
>>> > On 2012/10/11 18:00:54, Ronghua Wu wrote:
>>> >>
>>> >> Both of my attempt to fix the build errors failed with the same
>>> errors. :(
>>> >
>>> >
>>> >> For the clang issue, I couldn't reproduce locally with
>>> >> GYP_DEFINES=clang=1.
>>> >
>>> > Any
>>> >>
>>> >> suggestions?
>>> >
>>> >
>>> >
>>> > I think you should be able to reproduce it with component build:
>>> > http://www.chromium.org/developers/how-tos/component-build
>>> >
>>> > To fix it you need to add direct dependency on
>>> "build/linux/system.gyp:ssl"
>>> > for
>>> > libjingle, otherwise linker doesn't know that it needs to link with NSS
>>> > libraries.
>>>
>>> sergey: crypto.gyp is using export_dependent_settings to directly
>>> export (to all of crypto's dependents) a dependency on NSS, which uses
>>> 'direct_dependent_settings' to modify the link_settings.
>>>
>>> As such, it should not be *necessary* to depend on
>>> "build/linux.system.gyp:ssl". That it is suggests a bug in either GYP
>>> or in Chromium's GYP files (it should be necessary for dependencies to
>>> be transitively exported). I suspect I will need to look into this.
>>>
>>
>> I believe that GYP behavior is correct in this case. It does propagate
>> dependency for static builds. But in case of component build crypto is
>> compiled to libcrccrypto.so, and libcrccrypto.so is linked with NSS
>> libraries. Because it is a dynamically-linked library, GYP doesn't need
>> to propagate crypto dependencies to the targets that depend on crypto. With
>> that change libjingle calls NSS directly and so we need to specify NSS
>> dependency explicitly.
>>
> Make senses. Then my changes to the libjingle.gyp is needed.
>
>>
>>
>>>
>>> >
>>> > BTW it's good idea to always use clang and component build by default -
>>> > you'll
>>> > get shorter build time and better error reporting.
>>> >
>>> >
>>> >
>>> >> For the chromeos issue, only guess is the "use_openssl" is defined so
>>> that
>>> >> the
>>> >> nss_ssl_util.cc is removed from the build:
>>> >>
>>> >>
>>>
http://code.google.com/searchframe#OAMlx_jo-ck/src/net/net.gyp&type=cs&l=942
>>> >> However if that's the case, we shouldn't even have called
>>> >> EnsureNSSSSLInit.
>>> >
>>> >
>>> >
>>> > https://codereview.chromium.org/11093061/
>>>
>>
>>
>

[Ronghua Wu (Left Chromium), 2012-10-15 00:32:21]

Thanks a lot Ryan. I was doing pretty much same as what you suggested in the
libjingle.gyp. And the NET_EXPORT solves the issue in content. At least my local
linux_clang looks good now.

This is ready for another look.

https://codereview.chromium.org/11093061/diff/20001/third_party/libjingle/lib...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/20001/third_party/libjingle/lib...
third_party/libjingle/libjingle.gyp:134: ['use_openssl!=1', {
Changed from "'OS!="android"". Make sense?

[Ryan Sleevi, 2012-10-15 17:58:39]

LGTM, but one requested restructuring of the GYP file below.

https://codereview.chromium.org/11093061/diff/20001/third_party/libjingle/lib...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/20001/third_party/libjingle/lib...
third_party/libjingle/libjingle.gyp:134: ['use_openssl!=1', {
On 2012/10/15 00:32:22, Ronghua Wu wrote:
> Changed from "'OS!="android"". Make sense?

Change SGTM, but it makes me think that you should double-nest the conditions

That is, lines 139-144 should only be evaluated inside the "'use_openssl != 1'"
condition

[Ronghua Wu (Left Chromium), 2012-10-15 18:06:24]

https://codereview.chromium.org/11093061/diff/20001/third_party/libjingle/lib...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/20001/third_party/libjingle/lib...
third_party/libjingle/libjingle.gyp:134: ['use_openssl!=1', {
On 2012/10/15 17:58:39, Ryan Sleevi wrote:
> On 2012/10/15 00:32:22, Ronghua Wu wrote:
> > Changed from "'OS!="android"". Make sense?
> 
> Change SGTM, but it makes me think that you should double-nest the conditions
> 
> That is, lines 139-144 should only be evaluated inside the "'use_openssl !=
1'"
> condition

Good point. Done

[wjia(left Chromium), 2012-10-15 18:43:14]

lgtm on media.

[Ronghua Wu (Left Chromium), 2012-10-15 18:43:37]

+piman for content_renderer.gypi change.

[Ronghua Wu (Left Chromium), 2012-10-15 21:55:20]

+creis for content_renderer.gypi change

[Charlie Reis, 2012-10-15 22:33:45]

Rubber stamp LGTM on content_renderer.gypi.

[Sergey Ulanov, 2012-10-15 23:09:13]

libjingle.gyp - lgtm

https://codereview.chromium.org/11093061/diff/20006/third_party/libjingle/lib...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/20006/third_party/libjingle/lib...
third_party/libjingle/libjingle.gyp:139: [ 'os_posix == 1 and OS != "mac" and OS
!= "ios" and OS != "android"', {
nit: 80-char lines

[Ronghua Wu (Left Chromium), 2012-10-15 23:40:39]

https://codereview.chromium.org/11093061/diff/20006/third_party/libjingle/lib...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/11093061/diff/20006/third_party/libjingle/lib...
third_party/libjingle/libjingle.gyp:139: [ 'os_posix == 1 and OS != "mac" and OS
!= "ios" and OS != "android"', {
On 2012/10/15 23:09:13, sergeyu wrote:
> nit: 80-char lines

Done.

[commit-bot: I haz the power, 2012-10-16 00:47:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/11093061/29008

[commit-bot: I haz the power, 2012-10-16 02:53:57]

Change committed as 162055