Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
737913005:
Cronet UrlRequest onAppendChunk now checks that request is not destroyed. (Closed)
DescriptionCronet UrlRequest onAppendChunk now checks that request is not destroyed.
BUG=434855
TEST=cr_cronet.py test -f *testAppendChunk*
Committed: https://crrev.com/8dd3bfac451d40a2888d4a7d2580561969894e85
Cr-Commit-Position: refs/heads/master@{#305022}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix findbugs warning. #
Total comments: 9
Patch Set 3 : Address review comments. #Patch Set 4 : Moar threaded test. #
Total comments: 8
Patch Set 5 : Cleanup and split appendChunk test #
Total comments: 2
Patch Set 6 : Mark tests small. #
Messages
Total messages: 16 (3 generated)
Patchset #1 (id:1) has been deleted
mef@chromium.org changed reviewers: + miloslav@google.com, mmenke@chromium.org
Awesome! Thanks a lot for the fix! Will cherry pick it. https://codereview.chromium.org/737913005/diff/20001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/737913005/diff/20001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:132: return; Would it make sense to also log something when that happens? It should be rare, so it shouldn't cause logspam, and it may potentially help us with debugging.
https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java (right): https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:265: listener.blockForComplete(); This test doesn't fail without the patch, does it? https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:274: nit: Remove extra blank line. https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:129: // Request could have completed and got destroyed on the network thread got -> been https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:130: // while appendChunk was posting task from application thread. nit: posting the task from an application thread.
Thanks, PTAL! https://codereview.chromium.org/737913005/diff/20001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/737913005/diff/20001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:132: return; On 2014/11/19 18:45:20, miloslav wrote: > Would it make sense to also log something when that happens? It should be rare, > so it shouldn't cause logspam, and it may potentially help us with debugging. Done. https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java (right): https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:265: listener.blockForComplete(); On 2014/11/19 18:50:51, mmenke wrote: > This test doesn't fail without the patch, does it? It failed inconsistently when I had appendChunk in a loop because request was getting completed on network thread while java was posting chunks on java thread. I've removed the loop to prevent slooow tests. https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:274: On 2014/11/19 18:50:51, mmenke wrote: > nit: Remove extra blank line. Done. https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:129: // Request could have completed and got destroyed on the network thread On 2014/11/19 18:50:51, mmenke wrote: > got -> been Done. https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:130: // while appendChunk was posting task from application thread. On 2014/11/19 18:50:51, mmenke wrote: > nit: posting the task from an application thread. Done.
https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java (right): https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:265: listener.blockForComplete(); On 2014/11/19 19:32:36, mef wrote: > On 2014/11/19 18:50:51, mmenke wrote: > > This test doesn't fail without the patch, does it? > It failed inconsistently when I had appendChunk in a loop because request was > getting completed on network thread while java was posting chunks on java > thread. I've removed the loop to prevent slooow tests. Why would it fail? The request only finished after we've uploaded the entire body, so the last request.appendChunk above this line will be executed before the request completes. We have a lock when ChromiumURLRequest calls into the listener and deletes the adapter, so the appendChunk below this line should always end up seeing the deleted adapter. I just can't see how this test would trigger the issue.
On 2014/11/19 19:37:25, mmenke wrote: > https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... > File > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java > (right): > > https://codereview.chromium.org/737913005/diff/40001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:265: > listener.blockForComplete(); > On 2014/11/19 19:32:36, mef wrote: > > On 2014/11/19 18:50:51, mmenke wrote: > > > This test doesn't fail without the patch, does it? > > It failed inconsistently when I had appendChunk in a loop because request was > > getting completed on network thread while java was posting chunks on java > > thread. I've removed the loop to prevent slooow tests. > > Why would it fail? The request only finished after we've uploaded the entire > body, so the last request.appendChunk above this line will be executed before > the request completes. > > We have a lock when ChromiumURLRequest calls into the listener and deletes the > adapter, so the appendChunk below this line should always end up seeing the > deleted adapter. I just can't see how this test would trigger the issue. Updated test consistently succeeds with the onAppendChunk change and consistently crashes without it with this stack: mef@mefd:~/chrome/android/src$ third_party/android_tools/ndk/ndk-stack -sym out/Debug/lib -dump c7.txt ********** Crash dump: ********** Build fingerprint: 'google/occam/mako:5.0/LRX09G/1507390:userdebug/dev-keys' pid: 1718, tid: 1739, name: ChromiumNet >>> org.chromium.cronet_test_apk <<< signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- Stack frame #00 pc 0003bbf4 /system/lib/libc.so (tgkill+12) Stack frame #01 pc 00015f51 /system/lib/libc.so (pthread_kill+52) Stack frame #02 pc 00016b63 /system/lib/libc.so (raise+10) Stack frame #03 pc 000133b5 /system/lib/libc.so (__libc_android_abort+36) Stack frame #04 pc 00011e7c /system/lib/libc.so (abort+4) Stack frame #05 pc 0009ad01 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine DebugBreak at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/debug/debugger_posix.cc:217 Stack frame #06 pc 000b68dd /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine logging::LogMessage::~LogMessage() at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/logging.cc:641 Stack frame #07 pc 00083427 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine scoped_ptr<cronet::CronetURLRequestAdapter::CronetURLRequestAdapterDelegate, base::DefaultDeleter<cronet::CronetURLRequestAdapter::CronetURLRequestAdapterDelegate> >::operator->() const at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/memory/scoped_ptr.h:385 (discriminator 14) Stack frame #08 pc 000858a5 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine cronet::URLRequestAdapter::OnAppendChunk(scoped_ptr<char [], base::DefaultDeleter<char []> >, int, bool) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../components/cronet/android/url_request_adapter.cc:136 Stack frame #09 pc 00085b4b /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::internal::RunnableAdapter<void (cronet::URLRequestAdapter::*)(scoped_ptr<char [], base::DefaultDeleter<char []> >, int, bool)>::Run(cronet::URLRequestAdapter*, scoped_ptr<char [], base::DefaultDeleter<char []> >, int const&, bool const&) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/bind_internal.h:311 (discriminator 6) Stack frame #10 pc 00087189 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::Callback<void ()>::Run() const at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/callback.h:401 (discriminator 1) Stack frame #11 pc 0009bc09 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::Callback<void ()>::Run() const at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/callback.h:401 (discriminator 1) Stack frame #12 pc 000bcb1b /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::MessageLoop::RunTask(base::PendingTask const&) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/message_loop/message_loop.cc:449 Stack frame #13 pc 000bcba5 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/message_loop/message_loop.cc:458 Stack frame #14 pc 000bdfb3 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::MessageLoop::DoWork() at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/message_loop/message_loop.cc:567 Stack frame #15 pc 00088f51 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/message_loop/message_pump_libevent.cc:232 Stack frame #16 pc 000bd95b /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::MessageLoop::RunHandler() at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/message_loop/message_loop.cc:417 (discriminator 1) Stack frame #17 pc 000cdf7d /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::RunLoop::Run() at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/run_loop.cc:55 Stack frame #18 pc 000bc379 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::MessageLoop::Run() at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/message_loop/message_loop.cc:310 Stack frame #19 pc 000dfb65 /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine base::Thread::ThreadMain() at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/threading/thread.cc:228 Stack frame #20 pc 000dc05d /data/app/org.chromium.cronet_test_apk-1/lib/arm/libcronet_tests.so: Routine ThreadFunc at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/threading/platform_thread_posix.cc:80 Stack frame #21 pc 0001573f /system/lib/libc.so (__pthread_start(void*)+30) Stack frame #22 pc 00013713 /system/lib/libc.so (__start_thread+6) mef@mefd:~/chrome/android/src$ components/cronet/tools/cr_cronet.py build
https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java (right): https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:29: // URL used for base tests. What's a base test? Suggest just inlining this - when I add chunked upload support to the in process test server, should probably switch this test over to using it, just to be consistent with the others. https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:244: @SmallTest Is this a small test? https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:251: for (int test = 0; test < 100; ++test) { Should have a comment explaining why this is needed, including a quick description of the race this fixes, and maybe a link to the bug, since this isn't exactly the most intuitive test? https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:252: nit: Remove blank line.
Thanks, PTAL. https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java (right): https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:29: // URL used for base tests. On 2014/11/19 20:45:08, mmenke wrote: > What's a base test? Suggest just inlining this - when I add chunked upload > support to the in process test server, should probably switch this test over to > using it, just to be consistent with the others. Done. https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:244: @SmallTest On 2014/11/19 20:45:08, mmenke wrote: > Is this a small test? Hrm, according to http://googletesting.blogspot.com/2010/12/test-sizes.html most of our tests are medium if they interact with localhost. https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:251: for (int test = 0; test < 100; ++test) { On 2014/11/19 20:45:08, mmenke wrote: > Should have a comment explaining why this is needed, including a quick > description of the race this fixes, and maybe a link to the bug, since this > isn't exactly the most intuitive test? Done. https://codereview.chromium.org/737913005/diff/80001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:252: On 2014/11/19 20:45:08, mmenke wrote: > nit: Remove blank line. Done.
LGTM https://codereview.chromium.org/737913005/diff/100001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java (right): https://codereview.chromium.org/737913005/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:274: @MediumTest nit: Should probably be SmallTest, for consistency with the others. If we want to relabel them all (And given how long they take, that would probably be more accurate), should do it in a followup CL.
Thanks! https://codereview.chromium.org/737913005/diff/100001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java (right): https://codereview.chromium.org/737913005/diff/100001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/UploadTest.java:274: @MediumTest On 2014/11/20 15:57:54, mmenke wrote: > nit: Should probably be SmallTest, for consistency with the others. If we want > to relabel them all (And given how long they take, that would probably be more > accurate), should do it in a followup CL. Done.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/737913005/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8dd3bfac451d40a2888d4a7d2580561969894e85 Cr-Commit-Position: refs/heads/master@{#305022} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
