Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1448583003:
[Cronet] Add QUIC experimental params (Closed)
Description[Cronet] Add QUIC experimental params
This CL plumbs four QUIC experimental params (
quic_store_server_configs_in_properties,
quic_delay_tcp_race,
quic_max_number_of_lossy_connections,
quic_packet_loss_threshold) from Cronet's
setExperimentalOptions API to net::HttpNetworkSession.
This CL also adds a unittests target to run the unittests.
A followup CL will enable the unittests on the cronet bots.
BUG=545118
Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c
Cr-Commit-Position: refs/heads/master@{#360454}
Committed: https://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253
Cr-Commit-Position: refs/heads/master@{#360875}
Committed: https://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5
Cr-Commit-Position: refs/heads/master@{#361138}
Patch Set 1 #Patch Set 2 : Add test and quic_delay_tcp_race #Patch Set 3 : Rebased #Patch Set 4 : Self review #Patch Set 5 : #
Total comments: 8
Patch Set 6 : Add two more params and address comments #
Total comments: 4
Patch Set 7 : Address comments #Patch Set 8 : Initialize variables #Patch Set 9 : Rebased #Patch Set 10 : Fix build #Messages
Total messages: 56 (21 generated)
Description was changed from ========== [Cronet] Add QUIC experimental params BUG= ========== to ========== [Cronet] Add QUIC experimental params This CL plumbs two QUIC experimental params ( quic_store_server_configs_in_properties & ?) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. BUG=545118 ==========
xunjieli@chromium.org changed reviewers: + rtenneti@google.com
Raman, you mentioned that there are two QUIC parameters that you'd like to add, what is the other param that you need? I only remember "quic_store_server_configs_in_properties."
rtenneti@chromium.org changed reviewers: + rtenneti@chromium.org
lgtm LGTM. Would be awesome if we could write an unittest.
Description was changed from ========== [Cronet] Add QUIC experimental params This CL plumbs two QUIC experimental params ( quic_store_server_configs_in_properties & ?) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. BUG=545118 ========== to ========== [Cronet] Add QUIC experimental params This CL plumbs two QUIC experimental params ( quic_store_server_configs_in_properties & quic_delay_tcp_race) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 ==========
Patchset #2 (id:20001) has been deleted
xunjieli@chromium.org changed reviewers: + mef@chromium.org
On 2015/11/14 00:43:35, ramant wrote: > lgtm > > LGTM. Would be awesome if we could write an unittest. Thanks! I have added a unittest. I added quic_delay_tcp_race, do you need quic_packet_loss_threshold and quic_max_number_of_lossy_connections in this CL? Misha, PTAL.
On 2015/11/18 20:09:04, xunjieli wrote:
> On 2015/11/14 00:43:35, ramant wrote:
> > lgtm
> >
> > LGTM. Would be awesome if we could write an unittest.
>
> Thanks! I have added a unittest. I added quic_delay_tcp_race, do you need
> quic_packet_loss_threshold and quic_max_number_of_lossy_connections in this
CL?
>
> Misha, PTAL.
If it is no problem, would be good to add bad packet loss experiment also to
this CL which accepts the following two parameters. cronet users can specify
(and/or try different values) these values in their experiments.
{
"name": "EnabledBadPacketLoss",
"params": {
"max_number_of_lossy_connections": "4",
"packet_loss_threshold": "0.5"
}
}
thanks much for doing this
raman
Patch Set 5: LGTM https://codereview.chromium.org/1448583003/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config_unittest.cc (right): https://codereview.chromium.org/1448583003/diff/100001/components/cronet/url_... components/cronet/url_request_context_config_unittest.cc:36: // Set a ProxyConfigService to advoid DCHECK failure when building. nit: advoid -> avoid.
It would be nice to add more edge tests, but that should probably wait for Paul to land his CL to get rid of JSON. https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi... components/cronet.gypi:449: 'target_name': 'cronet_unittests', FWIW cronet_unittests are already included into component tests: https://code.google.com/p/chromium/codesearch#chromium/src/components/compone... I'm not sure whether they are executed though. https://codereview.chromium.org/1448583003/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config_unittest.cc (right): https://codereview.chromium.org/1448583003/diff/100001/components/cronet/url_... components/cronet/url_request_context_config_unittest.cc:49: // Check store_server_configs_in_properties nit: End comment with period, also below.
Thanks! I have addressed comments and added two additional params. https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi... components/cronet.gypi:449: 'target_name': 'cronet_unittests', On 2015/11/18 20:33:25, mef wrote: > FWIW cronet_unittests are already included into component tests: > > https://code.google.com/p/chromium/codesearch#chromium/src/components/compone... > > I'm not sure whether they are executed though. They are probably executed, but not on Cronet bots. So it is hard to know. Should we watch these on our bots? I do see components who have their own test targets though. Not sure which is better. Which one do you prefer? https://codereview.chromium.org/1448583003/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config_unittest.cc (right): https://codereview.chromium.org/1448583003/diff/100001/components/cronet/url_... components/cronet/url_request_context_config_unittest.cc:36: // Set a ProxyConfigService to advoid DCHECK failure when building. On 2015/11/18 20:22:08, ramant wrote: > nit: advoid -> avoid. Done. https://codereview.chromium.org/1448583003/diff/100001/components/cronet/url_... components/cronet/url_request_context_config_unittest.cc:49: // Check store_server_configs_in_properties On 2015/11/18 20:33:25, mef wrote: > nit: End comment with period, also below. Done.
Patch Set 6: LGTM https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... components/cronet/url_request_context_config.cc:50: VLOG(1) << "Experimental Options:" << experimental_options; nit: is this intentional? Should we do DVLOG(1)?
https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi... components/cronet.gypi:449: 'target_name': 'cronet_unittests', On 2015/11/18 20:58:44, xunjieli wrote: > On 2015/11/18 20:33:25, mef wrote: > > FWIW cronet_unittests are already included into component tests: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/compone... > > > > I'm not sure whether they are executed though. > > They are probably executed, but not on Cronet bots. So it is hard to know. > Should we watch these on our bots? > > I do see components who have their own test targets though. > > Not sure which is better. Which one do you prefer? I don't have particular preference, I just don't think we should do both if we can avoid that. https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... File components/cronet/url_request_context_config_unittest.cc (right): https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... components/cronet/url_request_context_config_unittest.cc:59: EXPECT_EQ(0.5f, params->quic_packet_loss_threshold); You may want to use EXPECT_DOUBLE_EQ due to rounding https://code.google.com/p/googletest/wiki/AdvancedGuide#Floating-Point_Compar...
Thanks! PTAL https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/1448583003/diff/100001/components/cronet.gypi... components/cronet.gypi:449: 'target_name': 'cronet_unittests', On 2015/11/18 21:35:33, mef wrote: > On 2015/11/18 20:58:44, xunjieli wrote: > > On 2015/11/18 20:33:25, mef wrote: > > > FWIW cronet_unittests are already included into component tests: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/compone... > > > > > > I'm not sure whether they are executed though. > > > > They are probably executed, but not on Cronet bots. So it is hard to know. > > Should we watch these on our bots? > > > > I do see components who have their own test targets though. > > > > Not sure which is better. Which one do you prefer? > > I don't have particular preference, I just don't think we should do both if we > can avoid that. Definitely not both. I have moved the other one over. I think it's more convenient this way and easier to know where test files go and where tests are run. Compiling components_unittests takes a substantially longer time too. https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... components/cronet/url_request_context_config.cc:50: VLOG(1) << "Experimental Options:" << experimental_options; On 2015/11/18 21:22:08, ramant wrote: > nit: is this intentional? Should we do DVLOG(1)? Done. https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... File components/cronet/url_request_context_config_unittest.cc (right): https://codereview.chromium.org/1448583003/diff/120001/components/cronet/url_... components/cronet/url_request_context_config_unittest.cc:59: EXPECT_EQ(0.5f, params->quic_packet_loss_threshold); On 2015/11/18 21:35:33, mef wrote: > You may want to use EXPECT_DOUBLE_EQ due to rounding > https://code.google.com/p/googletest/wiki/AdvancedGuide#Floating-Point_Compar... Done. Used EXPECT_FLOAT_EQ since the quic_packet_loss_threshold is a float.
Description was changed from ========== [Cronet] Add QUIC experimental params This CL plumbs two QUIC experimental params ( quic_store_server_configs_in_properties & quic_delay_tcp_race) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 ========== to ========== [Cronet] Add QUIC experimental params This CL plumbs four QUIC experimental params ( quic_store_server_configs_in_properties & quic_delay_tcp_race, quic_max_number_of_lossy_connections, quic_packet_loss_threshold) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 ==========
Description was changed from ========== [Cronet] Add QUIC experimental params This CL plumbs four QUIC experimental params ( quic_store_server_configs_in_properties & quic_delay_tcp_race, quic_max_number_of_lossy_connections, quic_packet_loss_threshold) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 ========== to ========== [Cronet] Add QUIC experimental params This CL plumbs four QUIC experimental params ( quic_store_server_configs_in_properties, quic_delay_tcp_race, quic_max_number_of_lossy_connections, quic_packet_loss_threshold) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 ==========
lgtm
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtenneti@chromium.org Link to the patchset: https://codereview.chromium.org/1448583003/#ps140001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1448583003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1448583003/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c Cr-Commit-Position: refs/heads/master@{#360454}
Message was sent while issue was closed.
I believe that this is the culprit for this uninitialized read: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests...
Message was sent while issue was closed.
On 2015/11/19 02:58:49, Derek Bruening wrote: > I believe that this is the culprit for this uninitialized read: > > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests... And this one (the msan tool seems to be having symbol issues, but the URLRequestContextBuilder is visible): http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests... Given that it affects multiple tests it seems best to revert.
Message was sent while issue was closed.
Plus errors on the Valgrind bots: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v... UninitCondition Conditional jump or move depends on uninitialised value(s) net::QuicStreamFactory::QuicStreamFactory(net::HostResolver*, net::ClientSocketFactory*, base::WeakPtr<net::HttpServerProperties>, net::CertVerifier*, net::CertPolicyEnforcer*, net::ChannelIDService*, net::TransportSecurityState*, net::SocketPerformanceWatcherFactory*, net::QuicCryptoClientStreamFactory*, net::QuicRandom*, net::QuicClock*, unsigned long, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<net::QuicVersion, std::allocator<net::QuicVersion> > const&, bool, bool, bool, float, bool, bool, bool, bool, int, float, int, int, int, int, bool, bool, bool, std::vector<unsigned int, std::allocator<unsigned int> > const&) (net/quic/quic_stream_factory.cc:649) net::HttpNetworkSession::HttpNetworkSession(net::HttpNetworkSession::Params const&) (net/http/http_network_session.cc:140) net::URLRequestContextBuilder::Build() (net/url_request/url_request_context_builder.cc:404) extensions::PrivetV3ContextGetter::InitOnNetThread() (chrome/browser/extensions/api/gcd_private/privet_v3_context_getter.cc:114) extensions::PrivetV3ContextGetter::AddPairedHostOnNetThread(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, net::SHA256HashValue const&) (chrome/browser/extensions/api/gcd_private/privet_v3_context_getter.cc:132)
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:140001) has been created in https://codereview.chromium.org/1454333002/ by bruening@chromium.org. The reason for reverting is: [MemSheriff] Reverting CL that contains uninitialized reads that show up in multiple tests on MSan and Valgrind MFYI bots, e.g.: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests... .
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Add QUIC experimental params This CL plumbs four QUIC experimental params ( quic_store_server_configs_in_properties, quic_delay_tcp_race, quic_max_number_of_lossy_connections, quic_packet_loss_threshold) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c Cr-Commit-Position: refs/heads/master@{#360454} ========== to ========== [Cronet] Add QUIC experimental params This CL plumbs four QUIC experimental params ( quic_store_server_configs_in_properties, quic_delay_tcp_race, quic_max_number_of_lossy_connections, quic_packet_loss_threshold) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c Cr-Commit-Position: refs/heads/master@{#360454} ==========
Raman, could you take a look to see if the initial values are correct?
Patch Set 8 LGTM (initial values are correct).
The CQ bit was checked by rtenneti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org Link to the patchset: https://codereview.chromium.org/1448583003/#ps160001 (title: "Initialize variables")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1448583003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1448583003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtenneti@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/1448583003/#ps180001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1448583003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1448583003/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253 Cr-Commit-Position: refs/heads/master@{#360875}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:180001) has been created in https://codereview.chromium.org/1464903002/ by xunjieli@chromium.org. The reason for reverting is: gyp dependency is missing. This broke one internal bot..
Message was sent while issue was closed.
Hi Misha, could you take a look at the gyp file changes?
The failure output from the internal bot is pasted below.
Is this because shared library linking clashes with that of android on chrome? I
couldn't get the error to reproduce locally. I made cronet_unittests depend on
cronet_static instead of cronet_static_small. Is this the right way?
Thanks! PTAL
FAILED: if [ ! -e lib/libcronet_unittests.cr.so -o ! -e
lib/libcronet_unittests.cr.so.TOC ]; then /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC
-B/b/build/slave/clang-clankium-tot-builder/build/src/third_party/binutils/Linux_x64/Release/bin
-Wl,-z,relro -Wl,-z,now -fuse-ld=gold -fsanitize=address -Wl,--build-id=sha1
-Wl,--no-undefined
--sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm
-nostdlib
-L../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libs/armeabi-v7a
-Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a
-Wl,--exclude-libs=libcommon_audio.a -Wl,--exclude-libs=libcommon_audio_neon.a
-Wl,--exclude-libs=libcommon_audio_sse2.a -Wl,--exclude-libs=libiSACFix.a
-Wl,--exclude-libs=libisac_neon.a -Wl,--exclude-libs=libopus.a
-Wl,--exclude-libs=libvpx.a -target arm-linux-androideabi -Wl,--icf=all
-Wl,-shared,-Bsymbolic
../../third_party/android_tools/ndk//platforms/android-16/arch-arm/usr/lib/crtbegin_so.o
-Wl,--warn-shared-textrel -Wl,-O1 -o lib/libcronet_unittests.cr.so
-Wl,-soname=libcronet_unittests.cr.so @lib/libcronet_unittests.cr.so.rsp && {
readelf -d lib/libcronet_unittests.cr.so | grep SONAME ; nm -gD -f p
lib/libcronet_unittests.cr.so | cut -f1-2 -d' '; } >
lib/libcronet_unittests.cr.so.TOC; else /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC
-B/b/build/slave/clang-clankium-tot-builder/build/src/third_party/binutils/Linux_x64/Release/bin
-Wl,-z,relro -Wl,-z,now -fuse-ld=gold -fsanitize=address -Wl,--build-id=sha1
-Wl,--no-undefined
--sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm
-nostdlib
-L../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libs/armeabi-v7a
-Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a
-Wl,--exclude-libs=libcommon_audio.a -Wl,--exclude-libs=libcommon_audio_neon.a
-Wl,--exclude-libs=libcommon_audio_sse2.a -Wl,--exclude-libs=libiSACFix.a
-Wl,--exclude-libs=libisac_neon.a -Wl,--exclude-libs=libopus.a
-Wl,--exclude-libs=libvpx.a -target arm-linux-androideabi -Wl,--icf=all
-Wl,-shared,-Bsymbolic
../../third_party/android_tools/ndk//platforms/android-16/arch-arm/usr/lib/crtbegin_so.o
-Wl,--warn-shared-textrel -Wl,-O1 -o lib/libcronet_unittests.cr.so
-Wl,-soname=libcronet_unittests.cr.so @lib/libcronet_unittests.cr.so.rsp && {
readelf -d lib/libcronet_unittests.cr.so | grep SONAME ; nm -gD -f p
lib/libcronet_unittests.cr.so | cut -f1-2 -d' '; } >
lib/libcronet_unittests.cr.so.tmp && if ! cmp -s
lib/libcronet_unittests.cr.so.tmp lib/libcronet_unittests.cr.so.TOC; then mv
lib/libcronet_unittests.cr.so.tmp lib/libcronet_unittests.cr.so.TOC ; fi; fi
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
undefined reference to 'GURL::GURL(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&)'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
undefined reference to 'GURL::GURL(GURL const&)'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
undefined reference to 'GURL::~GURL()'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
undefined reference to 'GURL::~GURL()'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_library_loader.o):../../components/cronet/android/cronet_library_loader.cc:function
cronet::(anonymous namespace)::Init(): error: undefined reference to
'url::Initialize()'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_library_loader.o):../../components/cronet/android/cronet_library_loader.cc:cronet::(anonymous
namespace)::kCronetRegisteredMethods: error: undefined reference to
'url::android::RegisterJni(_JNIEnv*)'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
Java_org_chromium_net_CronetUrlRequest_nativeCreateRequestAdapter: error:
undefined reference to 'GURL::GURL(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&)'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
Java_org_chromium_net_CronetUrlRequest_nativeCreateRequestAdapter: error:
undefined reference to 'GURL::~GURL()'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
cronet::CronetURLRequestAdapter::CronetURLRequestAdapter(cronet::CronetURLRequestContextAdapter*,
_JNIEnv*, _jobject*, GURL const&, net::RequestPriority): error: undefined
reference to 'GURL::GURL(GURL const&)'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
cronet::CronetURLRequestAdapter::~CronetURLRequestAdapter(): error: undefined
reference to 'GURL::~GURL()'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
cronet::CronetURLRequestAdapter::OnReceivedRedirect(net::URLRequest*,
net::RedirectInfo const&, bool*): error: undefined reference to 'GURL::spec()
const'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.url_request_adapter.o):../../components/cronet/android/url_request_adapter.cc:function
cronet::URLRequestAdapter::URLRequestAdapter(cronet::URLRequestContextAdapter*,
cronet::URLRequestAdapter::URLRequestAdapterDelegate*, GURL,
net::RequestPriority): error: undefined reference to 'GURL::GURL()'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.url_request_adapter.o):../../components/cronet/android/url_request_adapter.cc:function
cronet::URLRequestAdapter::URLRequestAdapter(cronet::URLRequestContextAdapter*,
cronet::URLRequestAdapter::URLRequestAdapterDelegate*, GURL,
net::RequestPriority): error: undefined reference to 'GURL::GURL(GURL const&)'
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.url_request_adapter.o):../../components/cronet/android/url_request_adapter.cc:function
cronet::URLRequestAdapter::URLRequestAdapter(cronet::URLRequestContextAdapter*,
cronet::URLRequestAdapter::URLRequestAdapterDelegate*, GURL,
net::RequestPriority): error: undefined reference to 'GURL::operator=(GURL)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Add QUIC experimental params This CL plumbs four QUIC experimental params ( quic_store_server_configs_in_properties, quic_delay_tcp_race, quic_max_number_of_lossy_connections, quic_packet_loss_threshold) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c Cr-Commit-Position: refs/heads/master@{#360454} Committed: https://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253 Cr-Commit-Position: refs/heads/master@{#360875} ========== to ========== [Cronet] Add QUIC experimental params This CL plumbs four QUIC experimental params ( quic_store_server_configs_in_properties, quic_delay_tcp_race, quic_max_number_of_lossy_connections, quic_packet_loss_threshold) from Cronet's setExperimentalOptions API to net::HttpNetworkSession. This CL also adds a unittests target to run the unittests. A followup CL will enable the unittests on the cronet bots. BUG=545118 Committed: https://crrev.com/fde0b72c603cd111c36ca4cc416d82a7395bcf6c Cr-Commit-Position: refs/heads/master@{#360454} Committed: https://crrev.com/8ece3aa6845350c1971a3e824bf148f3e8de3253 Cr-Commit-Position: refs/heads/master@{#360875} ==========
On 2015/11/20 20:34:56, xunjieli wrote:
> Hi Misha, could you take a look at the gyp file changes?
>
> The failure output from the internal bot is pasted below.
> Is this because shared library linking clashes with that of android on chrome?
I
> couldn't get the error to reproduce locally. I made cronet_unittests depend on
> cronet_static instead of cronet_static_small. Is this the right way?
>
> Thanks! PTAL
>
> FAILED: if [ ! -e lib/libcronet_unittests.cr.so -o ! -e
> lib/libcronet_unittests.cr.so.TOC ]; then /b/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
> -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC
>
-B/b/build/slave/clang-clankium-tot-builder/build/src/third_party/binutils/Linux_x64/Release/bin
> -Wl,-z,relro -Wl,-z,now -fuse-ld=gold -fsanitize=address -Wl,--build-id=sha1
> -Wl,--no-undefined
> --sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm
> -nostdlib
>
-L../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libs/armeabi-v7a
> -Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a
> -Wl,--exclude-libs=libcommon_audio.a -Wl,--exclude-libs=libcommon_audio_neon.a
> -Wl,--exclude-libs=libcommon_audio_sse2.a -Wl,--exclude-libs=libiSACFix.a
> -Wl,--exclude-libs=libisac_neon.a -Wl,--exclude-libs=libopus.a
> -Wl,--exclude-libs=libvpx.a -target arm-linux-androideabi -Wl,--icf=all
> -Wl,-shared,-Bsymbolic
>
../../third_party/android_tools/ndk//platforms/android-16/arch-arm/usr/lib/crtbegin_so.o
> -Wl,--warn-shared-textrel -Wl,-O1 -o lib/libcronet_unittests.cr.so
> -Wl,-soname=libcronet_unittests.cr.so @lib/libcronet_unittests.cr.so.rsp && {
> readelf -d lib/libcronet_unittests.cr.so | grep SONAME ; nm -gD -f p
> lib/libcronet_unittests.cr.so | cut -f1-2 -d' '; } >
> lib/libcronet_unittests.cr.so.TOC; else /b/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
> -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC
>
-B/b/build/slave/clang-clankium-tot-builder/build/src/third_party/binutils/Linux_x64/Release/bin
> -Wl,-z,relro -Wl,-z,now -fuse-ld=gold -fsanitize=address -Wl,--build-id=sha1
> -Wl,--no-undefined
> --sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm
> -nostdlib
>
-L../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libs/armeabi-v7a
> -Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a
> -Wl,--exclude-libs=libcommon_audio.a -Wl,--exclude-libs=libcommon_audio_neon.a
> -Wl,--exclude-libs=libcommon_audio_sse2.a -Wl,--exclude-libs=libiSACFix.a
> -Wl,--exclude-libs=libisac_neon.a -Wl,--exclude-libs=libopus.a
> -Wl,--exclude-libs=libvpx.a -target arm-linux-androideabi -Wl,--icf=all
> -Wl,-shared,-Bsymbolic
>
../../third_party/android_tools/ndk//platforms/android-16/arch-arm/usr/lib/crtbegin_so.o
> -Wl,--warn-shared-textrel -Wl,-O1 -o lib/libcronet_unittests.cr.so
> -Wl,-soname=libcronet_unittests.cr.so @lib/libcronet_unittests.cr.so.rsp && {
> readelf -d lib/libcronet_unittests.cr.so | grep SONAME ; nm -gD -f p
> lib/libcronet_unittests.cr.so | cut -f1-2 -d' '; } >
> lib/libcronet_unittests.cr.so.tmp && if ! cmp -s
> lib/libcronet_unittests.cr.so.tmp lib/libcronet_unittests.cr.so.TOC; then mv
> lib/libcronet_unittests.cr.so.tmp lib/libcronet_unittests.cr.so.TOC ; fi; fi
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
> Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
> undefined reference to 'GURL::GURL(std::__1::basic_string<char,
> std::__1::char_traits<char>, std::__1::allocator<char> > const&)'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
> Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
> undefined reference to 'GURL::GURL(GURL const&)'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
> Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
> undefined reference to 'GURL::~GURL()'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.chromium_url_request.o):../../components/cronet/android/chromium_url_request.cc:function
> Java_org_chromium_net_ChromiumUrlRequest_nativeCreateRequestAdapter: error:
> undefined reference to 'GURL::~GURL()'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_library_loader.o):../../components/cronet/android/cronet_library_loader.cc:function
> cronet::(anonymous namespace)::Init(): error: undefined reference to
> 'url::Initialize()'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_library_loader.o):../../components/cronet/android/cronet_library_loader.cc:cronet::(anonymous
> namespace)::kCronetRegisteredMethods: error: undefined reference to
> 'url::android::RegisterJni(_JNIEnv*)'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
> Java_org_chromium_net_CronetUrlRequest_nativeCreateRequestAdapter: error:
> undefined reference to 'GURL::GURL(std::__1::basic_string<char,
> std::__1::char_traits<char>, std::__1::allocator<char> > const&)'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
> Java_org_chromium_net_CronetUrlRequest_nativeCreateRequestAdapter: error:
> undefined reference to 'GURL::~GURL()'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
>
cronet::CronetURLRequestAdapter::CronetURLRequestAdapter(cronet::CronetURLRequestContextAdapter*,
> _JNIEnv*, _jobject*, GURL const&, net::RequestPriority): error: undefined
> reference to 'GURL::GURL(GURL const&)'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
> cronet::CronetURLRequestAdapter::~CronetURLRequestAdapter(): error: undefined
> reference to 'GURL::~GURL()'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.cronet_url_request_adapter.o):../../components/cronet/android/cronet_url_request_adapter.cc:function
> cronet::CronetURLRequestAdapter::OnReceivedRedirect(net::URLRequest*,
> net::RedirectInfo const&, bool*): error: undefined reference to 'GURL::spec()
> const'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.url_request_adapter.o):../../components/cronet/android/url_request_adapter.cc:function
>
cronet::URLRequestAdapter::URLRequestAdapter(cronet::URLRequestContextAdapter*,
> cronet::URLRequestAdapter::URLRequestAdapterDelegate*, GURL,
> net::RequestPriority): error: undefined reference to 'GURL::GURL()'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.url_request_adapter.o):../../components/cronet/android/url_request_adapter.cc:function
>
cronet::URLRequestAdapter::URLRequestAdapter(cronet::URLRequestContextAdapter*,
> cronet::URLRequestAdapter::URLRequestAdapterDelegate*, GURL,
> net::RequestPriority): error: undefined reference to 'GURL::GURL(GURL const&)'
>
obj/components/libcronet_static_small.a(obj/components/cronet/android/cronet_static_small.url_request_adapter.o):../../components/cronet/android/url_request_adapter.cc:function
>
cronet::URLRequestAdapter::URLRequestAdapter(cronet::URLRequestContextAdapter*,
> cronet::URLRequestAdapter::URLRequestAdapterDelegate*, GURL,
> net::RequestPriority): error: undefined reference to 'GURL::operator=(GURL)'
> clang: error: linker command failed with exit code 1 (use -v to see
invocation)
> ninja: build stopped: subcommand failed.
Reopened the issue. PTAL at Patch #10. Thanks!
Patchset #10 (id:200001) has been deleted
Patchset #10 (id:220001) has been deleted
Hi Misha, the CL got reverted because it broke an internal bot. PTAL at new gyp files changes (Patch #10). Thanks!
On 2015/11/23 16:20:11, xunjieli wrote: > Hi Misha, the CL got reverted because it broke an internal bot. PTAL at new gyp > files changes (Patch #10). Thanks! Looks fine to me. Did you try to build all those targets locally? I've considered adding '../url/url.gyp:url_lib', to new target, but I think your change looks cleaner.
On 2015/11/23 16:27:22, mef wrote: > On 2015/11/23 16:20:11, xunjieli wrote: > > Hi Misha, the CL got reverted because it broke an internal bot. PTAL at new > gyp > > files changes (Patch #10). Thanks! > > Looks fine to me. Did you try to build all those targets locally? Yes, building these targets(cronet_static, cronet_static_small, libcronet, cronet_unittests) work locally. > I've considered adding '../url/url.gyp:url_lib', to new target, but I think your > change looks cleaner.
lgtm
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtenneti@chromium.org Link to the patchset: https://codereview.chromium.org/1448583003/#ps240001 (title: "Fix build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1448583003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1448583003/240001
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f24ee5f5afeb59d2a9134a7b986efb6483629fe5 Cr-Commit-Position: refs/heads/master@{#361138} |
