Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
624443003:
Setup ProxyConfigServiceAndroid in Cronet (Closed)
DescriptionSetup ProxyConfigServiceAndroid in Cronet
BUG=411377
Committed: https://crrev.com/0b21f8f3c41e0bd7e05bdd1f5c59785f8a96edb8
Cr-Commit-Position: refs/heads/master@{#300094}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added two tests #
Total comments: 2
Patch Set 3 : rebased on the other CL and created a separate nativeInitRequestContext #
Total comments: 6
Patch Set 4 : Modified tests #
Total comments: 6
Patch Set 5 : Addressed comments #Patch Set 6 : rebased #
Total comments: 10
Patch Set 7 : Addressed Misha's comments #Patch Set 8 : Rebased #Patch Set 9 : fix an indentation #
Total comments: 26
Patch Set 10 : Addressed Matt's comments #
Total comments: 10
Patch Set 11 : Addressed comments #Patch Set 12 : Initialize boolean to false #
Total comments: 1
Messages
Total messages: 45 (5 generated)
xunjieli@chromium.org changed reviewers: + clm@google.com, mef@chromium.org, mmenke@chromium.org
Hi Matt, Misha and Charles, Here's the CL that sets up ProxyConfigServiceAndroid. Basically I adopted Charles's suggestion to post an event to UI thread, block url context creation util we hear back. PTAL. Thanks! Helen https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:102: nativeInitProxyConfigServiceOnUIThread( nit: indent. will change in next patch.
https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.cc:137: base::MessageLoopForUI::current()->Start(); I hadn't realized it was so easy to set up a base::MessagePump on a Java thread. That's pretty nifty. https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.cc:142: proxy_config_service_.reset(service); Hrm...So we ensure that the Java URLRequestContext is still alive at this point because posting the task Java-side holds onto a reference to the object, and we ensure that the URLRequestContextAdapter is kept alive by a reference being owned by the task... A bit ugly, but it workw. https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.cc:151: scoped_ptr<URLRequestContextConfig> config) { What happens if the embedder tries to use the URLRequestContextAdapter before InitializeURLRequestContext has been called?
https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.cc:142: proxy_config_service_.reset(service); On 2014/10/02 16:17:25, mmenke wrote: > Hrm...So we ensure that the Java URLRequestContext is still alive at this point > because posting the task Java-side holds onto a reference to the object, and we > ensure that the URLRequestContextAdapter is kept alive by a reference being > owned by the task... A bit ugly, but it workw. I am open to suggestions :) There is probably a better way. https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.cc:151: scoped_ptr<URLRequestContextConfig> config) { On 2014/10/02 16:17:25, mmenke wrote: > What happens if the embedder tries to use the URLRequestContextAdapter before > InitializeURLRequestContext has been called? I guess they will have a context-not-setup error just like before?
On 2014/10/02 17:21:31, xunjieli wrote: > https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... > File components/cronet/android/url_request_context_adapter.cc (right): > > https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... > components/cronet/android/url_request_context_adapter.cc:142: > proxy_config_service_.reset(service); > On 2014/10/02 16:17:25, mmenke wrote: > > Hrm...So we ensure that the Java URLRequestContext is still alive at this > point > > because posting the task Java-side holds onto a reference to the object, and > we > > ensure that the URLRequestContextAdapter is kept alive by a reference being > > owned by the task... A bit ugly, but it workw. > > I am open to suggestions :) There is probably a better way. > > https://codereview.chromium.org/624443003/diff/1/components/cronet/android/ur... > components/cronet/android/url_request_context_adapter.cc:151: > scoped_ptr<URLRequestContextConfig> config) { > On 2014/10/02 16:17:25, mmenke wrote: > > What happens if the embedder tries to use the URLRequestContextAdapter before > > InitializeURLRequestContext has been called? > > I guess they will have a context-not-setup error just like before? Forgot we block in ChromiumUrlRequestContext currently - something we shouldn't really be doing. Currently, if we run that block on the main thread, we presumably prevent the callback from being called, so just block for 2 seconds before startup for no reason. Note that we'd have a Java-side ChromiumUrlRequest object, so ChromiumUrlRequest's constructor would not throw an error. We'd create the UrlRequestAdapter, post a task...And then crash, I think, in URLRequestAdapter::OnInitiateConnection, when we try and dereference the context. Or am I missing something?
Hi Matt, I added two test cases, and rebased on my other CL. Not sure if the test cases are good enough to address the concerns. PTAL. https://codereview.chromium.org/624443003/diff/20001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:66: Thread.sleep(4000); How important is to start the request right after the context is initialized? Does it matter to be right after or some time after? Haven't found a clean way to do that. Will try harder if it is an important case.
https://codereview.chromium.org/624443003/diff/20001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/20001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:66: Thread.sleep(4000); On 2014/10/03 19:05:12, xunjieli wrote: > How important is to start the request right after the context is initialized? > Does it matter to be right after or some time after? > Haven't found a clean way to do that. Will try harder if it is an important > case. I'm not following why you have to wait here - starting a request during factory initialization should automatically wait for initialization to complete before really starting the request, if you rebase this CL on top of your other CL. Also...why does sleeping help? Aren't we also running on the main thread here, so blocking initialization by the sleep, or are we on the main thread of another process, and there's some implicit IPC going on here, or what?
Hi Matt, I have incorporated your suggestion. It does fix the crashing. Thanks so much for helping me with debugging. Here's the new patch. PTAL. On 2014/10/06 15:30:14, mmenke wrote: > https://codereview.chromium.org/624443003/diff/20001/components/cronet/androi... > File > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java > (right): > > https://codereview.chromium.org/624443003/diff/20001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:66: > Thread.sleep(4000); > On 2014/10/03 19:05:12, xunjieli wrote: > > How important is to start the request right after the context is initialized? > > Does it matter to be right after or some time after? > > Haven't found a clean way to do that. Will try harder if it is an important > > case. > > I'm not following why you have to wait here - starting a request during factory > initialization should automatically wait for initialization to complete before > really starting the request, if you rebase this CL on top of your other CL. > I was trying to come up with two test cases so that we can test #1 start a request before factory initialization is done #2 start a request after factory initialization. Since factory/context initialization is no longer blocking, if we place the start call right after the factor initialization, we can probably fall into #1. If we delay some time, then we are confident it will go into #2? > Also...why does sleeping help? Aren't we also running on the main thread here, > so blocking initialization by the sleep, or are we on the main thread of another > process, and there's some implicit IPC going on here, or what? The test instrumentation app is running in another process with a different main thread, so sleeping does not block the main UI thread of the test app.
https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:45: assertEquals(200, listener.mHttpStatusCode); Unfortunately, this isn't guaranteed to be fast enough, if this is on a different thread from the main thread the factory posts back to (Given your response to my comment, I it's my understanding this is indeed on a different thread, though it seems weird to me that many of these Java calls are implicitly blocking cross-process IPCs). As a result, you'll have to do both initRequestFactory and request.start in the other process, synchronously. https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:46: } I'd also like to see a test where we synchronously init the context, start the request, and cancel it immediately. https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:65: // TODO(xunjieli): maybe find out a better way to do this. My suggestion: Send on request, wait for it to complete successfully, then send another, and make sure the second one works. That guarantees we wait long enough, and is robust against all possible refactors. Fixed timeouts are rarely, if ever, a good idea, though a fair number of tests do use them.
On 2014/10/06 18:08:01, mmenke wrote: > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > File > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java > (right): > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:45: > assertEquals(200, listener.mHttpStatusCode); > Unfortunately, this isn't guaranteed to be fast enough, if this is on a > different thread from the main thread the factory posts back to (Given your > response to my comment, I it's my understanding this is indeed on a different > thread, though it seems weird to me that many of these Java calls are implicitly > blocking cross-process IPCs). > > As a result, you'll have to do both initRequestFactory and request.start in the > other process, synchronously. > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:46: > } > I'd also like to see a test where we synchronously init the context, start the > request, and cancel it immediately. > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:65: > // TODO(xunjieli): maybe find out a better way to do this. > My suggestion: Send on request, wait for it to complete successfully, then send > another, and make sure the second one works. That guarantees we wait long > enough, and is robust against all possible refactors. > > Fixed timeouts are rarely, if ever, a good idea, though a fair number of tests > do use them. FYI, android unit tests run in the same process as the code under test. Also, are we sure that it's safe to remove the block() call? Won't the network stack now return in a partially-initialized state?
On 2014/10/06 18:13:07, Charles wrote: > On 2014/10/06 18:08:01, mmenke wrote: > > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > > File > > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java > > (right): > > > > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:45: > > assertEquals(200, listener.mHttpStatusCode); > > Unfortunately, this isn't guaranteed to be fast enough, if this is on a > > different thread from the main thread the factory posts back to (Given your > > response to my comment, I it's my understanding this is indeed on a different > > thread, though it seems weird to me that many of these Java calls are > implicitly > > blocking cross-process IPCs). > > > > As a result, you'll have to do both initRequestFactory and request.start in > the > > other process, synchronously. > > > > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:46: > > } > > I'd also like to see a test where we synchronously init the context, start the > > request, and cancel it immediately. > > > > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:65: > > // TODO(xunjieli): maybe find out a better way to do this. > > My suggestion: Send on request, wait for it to complete successfully, then > send > > another, and make sure the second one works. That guarantees we wait long > > enough, and is robust against all possible refactors. > > > > Fixed timeouts are rarely, if ever, a good idea, though a fair number of tests > > do use them. > > FYI, android unit tests run in the same process as the code under test. > > Also, are we sure that it's safe to remove the block() call? Won't the network > stack now return in a partially-initialized state? Yes, but once https://codereview.chromium.org/617393005/ lands, we'll secretly delay all requests behind the scenes until initialization is complete.
Thanks Matt for replying to Charles's comment. I have uploaded a patch which modifies the tests. PTAL. Thanks! https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:45: assertEquals(200, listener.mHttpStatusCode); On 2014/10/06 18:08:00, mmenke wrote: > Unfortunately, this isn't guaranteed to be fast enough, if this is on a > different thread from the main thread the factory posts back to (Given your > response to my comment, I it's my understanding this is indeed on a different > thread, though it seems weird to me that many of these Java calls are implicitly > blocking cross-process IPCs). > > As a result, you'll have to do both initRequestFactory and request.start in the > other process, synchronously. Done. https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:46: } On 2014/10/06 18:08:00, mmenke wrote: > I'd also like to see a test where we synchronously init the context, start the > request, and cancel it immediately. Done. https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:65: // TODO(xunjieli): maybe find out a better way to do this. On 2014/10/06 18:08:00, mmenke wrote: > My suggestion: Send on request, wait for it to complete successfully, then send > another, and make sure the second one works. That guarantees we wait long > enough, and is robust against all possible refactors. > > Fixed timeouts are rarely, if ever, a good idea, though a fair number of tests > do use them. Done. Great suggestion! thanks!
On 2014/10/06 19:40:45, xunjieli wrote: > Thanks Matt for replying to Charles's comment. I have uploaded a patch which > modifies the tests. PTAL. Thanks! > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > File > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java > (right): > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:45: > assertEquals(200, listener.mHttpStatusCode); > On 2014/10/06 18:08:00, mmenke wrote: > > Unfortunately, this isn't guaranteed to be fast enough, if this is on a > > different thread from the main thread the factory posts back to (Given your > > response to my comment, I it's my understanding this is indeed on a different > > thread, though it seems weird to me that many of these Java calls are > implicitly > > blocking cross-process IPCs). > > > > As a result, you'll have to do both initRequestFactory and request.start in > the > > other process, synchronously. > > Done. > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:46: > } > On 2014/10/06 18:08:00, mmenke wrote: > > I'd also like to see a test where we synchronously init the context, start the > > request, and cancel it immediately. > > Done. > > https://codereview.chromium.org/624443003/diff/40001/components/cronet/androi... > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:65: > // TODO(xunjieli): maybe find out a better way to do this. > On 2014/10/06 18:08:00, mmenke wrote: > > My suggestion: Send on request, wait for it to complete successfully, then > send > > another, and make sure the second one works. That guarantees we wait long > > enough, and is robust against all possible refactors. > > > > Fixed timeouts are rarely, if ever, a good idea, though a fair number of tests > > do use them. > > Done. Great suggestion! thanks! Just a heads up that there's a good chance I won't get back to this until next week - 3 days of the network stack meeting, and interview + heading out early on Friday (Parents in town).
Got it! I will work on NCN in the meanwhile. On Mon, Oct 6, 2014 at 3:48 PM, <mmenke@chromium.org> wrote: > On 2014/10/06 19:40:45, xunjieli wrote: > >> Thanks Matt for replying to Charles's comment. I have uploaded a patch >> which >> modifies the tests. PTAL. Thanks! >> > > > https://codereview.chromium.org/624443003/diff/40001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java > >> File >> > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java > >> (right): >> > > > https://codereview.chromium.org/624443003/diff/40001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java#newcode45 > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java:45: > >> assertEquals(200, listener.mHttpStatusCode); >> On 2014/10/06 18:08:00, mmenke wrote: >> > Unfortunately, this isn't guaranteed to be fast enough, if this is on a >> > different thread from the main thread the factory posts back to (Given >> your >> > response to my comment, I it's my understanding this is indeed on a >> > different > >> > thread, though it seems weird to me that many of these Java calls are >> implicitly >> > blocking cross-process IPCs). >> > >> > As a result, you'll have to do both initRequestFactory and >> request.start in >> the >> > other process, synchronously. >> > > Done. >> > > > https://codereview.chromium.org/624443003/diff/40001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java#newcode46 > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java:46: > >> } >> On 2014/10/06 18:08:00, mmenke wrote: >> > I'd also like to see a test where we synchronously init the context, >> start >> > the > >> > request, and cancel it immediately. >> > > Done. >> > > > https://codereview.chromium.org/624443003/diff/40001/ > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java#newcode65 > > components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ > ContextInitTest.java:65: > >> // TODO(xunjieli): maybe find out a better way to do this. >> On 2014/10/06 18:08:00, mmenke wrote: >> > My suggestion: Send on request, wait for it to complete successfully, >> then >> send >> > another, and make sure the second one works. That guarantees we wait >> long >> > enough, and is robust against all possible refactors. >> > >> > Fixed timeouts are rarely, if ever, a good idea, though a fair number of >> > tests > >> > do use them. >> > > Done. Great suggestion! thanks! >> > > Just a heads up that there's a good chance I won't get back to this until > next > week - 3 days of the network stack meeting, and interview + heading out > early on > Friday (Parents in town). > > https://codereview.chromium.org/624443003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:17: private static final String URL = "http://127.0.0.1:8000"; Per Android java style guide, only public static final should use all caps.... Looks like that rule is pretty universally ignored, though, so I guess this is the way to go. https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:22: CronetTestActivity activity = skipFactoryInitInOnCreate(); I'm sorry, I should have chimed in after Charles's comment: Since the test fixture is running inside the same process as the code being tested, the old test approach was fine (create activity with the factory here and then run requests here as well), and it was a bit cleaner, too. https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... File components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java (right): https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:64: class BlockingHttpUrlRequestListener implements HttpUrlRequestListener { Instead of this, can we just move MockHttpUrlRequestListener into its own file (And maybe rename it to TestHttpUrlRequestListener), and have it shared between different sets of tests?
Patchset #5 (id:90001) has been deleted
PTAL. Thanks! https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:17: private static final String URL = "http://127.0.0.1:8000"; On 2014/10/10 14:52:09, mmenke wrote: > Per Android java style guide, only public static final should use all caps.... > Looks like that rule is pretty universally ignored, though, so I guess this is > the way to go. I tried to changing it to lower case. But presubmit lint says "Static final field names must either be all caps (e.g. int HEIGHT_PX) for true constants, or start with s (e.g. AtomicInteger sNextId or Runnable sSuspendTask) for fields with mutable state or that dont feel like constants.", so I guess we probably should still use all caps, even though this field is not public. https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:22: CronetTestActivity activity = skipFactoryInitInOnCreate(); On 2014/10/10 14:52:10, mmenke wrote: > I'm sorry, I should have chimed in after Charles's comment: Since the test > fixture is running inside the same process as the code being tested, the old > test approach was fine (create activity with the factory here and then run > requests here as well), and it was a bit cleaner, too. Done. Thanks! I thought it was in a different process. After reading the android testing docs, it seems that it is indeed one process. https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... File components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java (right): https://codereview.chromium.org/624443003/diff/60001/components/cronet/androi... components/cronet/android/test/src/org/chromium/cronet_test_apk/CronetTestActivity.java:64: class BlockingHttpUrlRequestListener implements HttpUrlRequestListener { On 2014/10/10 14:52:10, mmenke wrote: > Instead of this, can we just move MockHttpUrlRequestListener into its own file > (And maybe rename it to TestHttpUrlRequestListener), and have it shared between > different sets of tests? Done. I have moved it to a separate file. I will refactor other tests to use that class in a follow-up CL.
Don't think I'm going to have time to get to this today. If you want to land early next week, feel free to do so, once you get Misha's signoff.
On 2014/10/10 19:18:09, mmenke wrote: > Don't think I'm going to have time to get to this today. If you want to land > early next week, feel free to do so, once you get Misha's signoff. Understood! I also have just made the UI message loop as a global, and rebased. I will work on NCN in the meanwhile.
https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/chromium_url_request_context.cc (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/chromium_url_request_context.cc:151: static void InitRequestContext( I would call this InitRequestContextOnUIThread for explicity and consistency. https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:141: private native void nativeInitRequestContext(long chromiumUrlRequestContextAdapter); nit: I'd put argument to the next line. https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:148: base::Bind(&URLRequestContextAdapter::InitializeURLRequestContext, I'd rename this InitRequestContextOnNetworkThread to clearly distinguish from InitRequestContextOnUIThread. https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:199: while (!tasks_waiting_for_context_.empty()) { this seems to be inside of 'if (config->enable_quic)', which is wrong. https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:83: // Called by main java UI thread to initialize URLRequestContext. Called _on_ main...
https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/chromium_url_request_context.cc (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/chromium_url_request_context.cc:151: static void InitRequestContext( On 2014/10/10 21:20:37, mef wrote: > I would call this InitRequestContextOnUIThread for explicity and consistency. Done. https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:141: private native void nativeInitRequestContext(long chromiumUrlRequestContextAdapter); On 2014/10/10 21:20:37, mef wrote: > nit: I'd put argument to the next line. Done. https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:148: base::Bind(&URLRequestContextAdapter::InitializeURLRequestContext, On 2014/10/10 21:20:37, mef wrote: > I'd rename this InitRequestContextOnNetworkThread to clearly distinguish from > InitRequestContextOnUIThread. Done. Good idea! Thanks! https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:199: while (!tasks_waiting_for_context_.empty()) { On 2014/10/10 21:20:37, mef wrote: > this seems to be inside of 'if (config->enable_quic)', which is wrong. Done. This is very bad. thanks for catching it! https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/624443003/diff/270001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:83: // Called by main java UI thread to initialize URLRequestContext. On 2014/10/10 21:20:37, mef wrote: > Called _on_ main... Done.
Pinging Matt and Misha for review. Thanks! Planning to wrap this up this week.
Looks good! Think we're about ready to land. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:44: nativeInitRequestContextOnUIThread( nit: +2 indent. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:48: new Handler(Looper.getMainLooper()).post(task); In the future, may want to just run synchronously if we're on the main thread (Just thinking about wrapping this with a blocking API for a URLConnection wrapper, and still allowing requests on the main thread, though, admittedly, no one should be doing that, anyways) Anyhow, fine for now. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:82: assertEquals(404, statusCodes[1]); nit: Fix indent. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:112: // Android specific UI MessageLoop. Maybe "UI MessageLoop" -> "MessageLoop on main thread, which is where objects that receive Java notifications generally live." https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:113: base::MessageLoop* g_main_message_loop = nullptr; optional: Suggest putting this at the top of the anonymous namespace (My general feeling is that globals/static variables/constants should go before class declarations in anonymous namespaces, unless they're strongly associated with one of those classes, in which case is makes sense for them to be declared just above/below that class). https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:135: void URLRequestContextAdapter::InitRequestContextOnUIThread() { DCHECK(config_)? https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:136: if (!base::MessageLoop::current()) { We have no tests where this check is false, I believe, as it requires multiple contexts/factories. Maybe add two simple tests for this case? One where we make a second context at the same time as the main one, and send requests to both contexts at once. One where we send a request on the main context, wait for it to complete, and then make and send a request on a second context. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:138: g_main_message_loop = new base::MessageLoopForUI; optional nit: Seems more common to use "()" with 0-argument constructors. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:140: } DCHECK_EQ(g_main_message_loop, base::MessageLoop::current());? https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:142: net::ProxyConfigService* service = Should generally avoid using raw pointers, except when passing around stuff you don't own. Should stick this directly in a scoped_ptr. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:150: Passed(&config_))); I don't think we get anything from passing the config here, as we muck with proxy_config_service_ here as well as in InitRequestContextOnNetworkThread. Should either pass the ProxyConfigService here as well, also using Passed, or get rid of the config_ argument and delete it in InitRequestContextOnNetworkThread. I slightly prefer the former, as it stays more in line with the whole "only muck with the Context on the network thread" thing, but can live with either. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:84: // Called on main java UI thread to initialize URLRequestContext. nit: Java https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:85: void InitRequestContextOnUIThread(); Should this be Main thread? I don't see the main thread being called the UI thread a whole lot, except in the context of Chromium.
Patchset #10 (id:420001) has been deleted
Thanks for the reviews! I have uploaded a new patch to address the comments. PTAL. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:44: nativeInitRequestContextOnUIThread( On 2014/10/15 17:44:17, mmenke wrote: > nit: +2 indent. Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:48: new Handler(Looper.getMainLooper()).post(task); On 2014/10/15 17:44:17, mmenke wrote: > In the future, may want to just run synchronously if we're on the main thread > (Just thinking about wrapping this with a blocking API for a URLConnection > wrapper, and still allowing requests on the main thread, though, admittedly, no > one should be doing that, anyways) > > Anyhow, fine for now. Acknowledged. I have added your suggestion in the comment. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:82: assertEquals(404, statusCodes[1]); On 2014/10/15 17:44:17, mmenke wrote: > nit: Fix indent. Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:112: // Android specific UI MessageLoop. On 2014/10/15 17:44:17, mmenke wrote: > Maybe "UI MessageLoop" -> "MessageLoop on main thread, which is where objects > that receive Java notifications generally live." Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:113: base::MessageLoop* g_main_message_loop = nullptr; On 2014/10/15 17:44:17, mmenke wrote: > optional: Suggest putting this at the top of the anonymous namespace (My > general feeling is that globals/static variables/constants should go before > class declarations in anonymous namespaces, unless they're strongly associated > with one of those classes, in which case is makes sense for them to be declared > just above/below that class). Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:135: void URLRequestContextAdapter::InitRequestContextOnUIThread() { On 2014/10/15 17:44:18, mmenke wrote: > DCHECK(config_)? Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:136: if (!base::MessageLoop::current()) { On 2014/10/15 17:44:17, mmenke wrote: > We have no tests where this check is false, I believe, as it requires multiple > contexts/factories. Maybe add two simple tests for this case? > > One where we make a second context at the same time as the main one, and send > requests to both contexts at once. > > One where we send a request on the main context, wait for it to complete, and > then make and send a request on a second context. Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:138: g_main_message_loop = new base::MessageLoopForUI; On 2014/10/15 17:44:17, mmenke wrote: > optional nit: Seems more common to use "()" with 0-argument constructors. Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:140: } On 2014/10/15 17:44:17, mmenke wrote: > DCHECK_EQ(g_main_message_loop, base::MessageLoop::current());? Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:142: net::ProxyConfigService* service = On 2014/10/15 17:44:17, mmenke wrote: > Should generally avoid using raw pointers, except when passing around stuff you > don't own. Should stick this directly in a scoped_ptr. Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:150: Passed(&config_))); On 2014/10/15 17:44:17, mmenke wrote: > I don't think we get anything from passing the config here, as we muck with > proxy_config_service_ here as well as in InitRequestContextOnNetworkThread. > > Should either pass the ProxyConfigService here as well, also using Passed, or > get rid of the config_ argument and delete it in > InitRequestContextOnNetworkThread. > > I slightly prefer the former, as it stays more in line with the whole "only muck > with the Context on the network thread" thing, but can live with either. Done. Sounds good. I adopted the second approach. I can't get the first working, three tests out of 31 tests failed with a seg fault when I tried Passed(&proxy_config_service_) to InitRequestContextOnNetworkThread. Not sure why.. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:84: // Called on main java UI thread to initialize URLRequestContext. On 2014/10/15 17:44:18, mmenke wrote: > nit: Java Done. https://codereview.chromium.org/624443003/diff/400001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:85: void InitRequestContextOnUIThread(); On 2014/10/15 17:44:18, mmenke wrote: > Should this be Main thread? I don't see the main thread being called the UI > thread a whole lot, except in the context of Chromium. Done.
lgtm with nits. https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/chromium_url_request_context.cc (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/chromium_url_request_context.cc:153: jlong urlRequestContextAdapter) { nit: may as well start using c++ argument naming convention here. https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:142: DCHECK_EQ(g_main_message_loop, base::MessageLoop::current()); what if base::MessageLoop::current() is NOT null on line 137? In this case g_main_message_loop will be NULL forever... https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:20: #include "net/proxy/proxy_config_service.h" I think you should be able to forward-declare net::ProxyConfigService.
LGTM https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:107: assertEquals(200, listener.mHttpStatusCode); For both of these tests, suggest using the 404 URL for the second request. Doesn't really get us much, but like the extra check that we aren't crossing the streams somehow. https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:150: } nit: +linebreak
https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/chromium_url_request_context.cc (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/chromium_url_request_context.cc:153: jlong urlRequestContextAdapter) { On 2014/10/15 20:04:55, mef wrote: > nit: may as well start using c++ argument naming convention here. Done. Good catch thanks! https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:107: assertEquals(200, listener.mHttpStatusCode); On 2014/10/15 20:15:16, mmenke wrote: > For both of these tests, suggest using the 404 URL for the second request. > Doesn't really get us much, but like the extra check that we aren't crossing the > streams somehow. Done. Good idea! thanks! https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/ContextInitTest.java:150: } On 2014/10/15 20:15:16, mmenke wrote: > nit: +linebreak Done. https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:142: DCHECK_EQ(g_main_message_loop, base::MessageLoop::current()); On 2014/10/15 20:04:55, mef wrote: > what if base::MessageLoop::current() is NOT null on line 137? In this case > g_main_message_loop will be NULL forever... I guess we implicitly assume that Java main thread won't have a chromium-specific messageloop set up in a place other than here. https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/624443003/diff/440001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:20: #include "net/proxy/proxy_config_service.h" On 2014/10/15 20:04:55, mef wrote: > I think you should be able to forward-declare net::ProxyConfigService. Done.
There appears to be a race in my code :( Tests fail on my nexus 5, even though all pass on my nexus 7. It seems that |is_context_initialized_| is evaluated to true, while our |context_| is null. I am going to figure out why this is happening tomorrow.
On 2014/10/15 21:27:21, xunjieli wrote: > There appears to be a race in my code :( Tests fail on my nexus 5, even though > all pass on my nexus 7. It seems that |is_context_initialized_| is evaluated to > true, while our |context_| is null. I am going to figure out why this is > happening tomorrow. I don't see |is_context_initialized_| being initialized :) to false in constructor.
On 2014/10/15 21:47:27, mef wrote: > On 2014/10/15 21:27:21, xunjieli wrote: > > There appears to be a race in my code :( Tests fail on my nexus 5, even though > > all pass on my nexus 7. It seems that |is_context_initialized_| is evaluated > to > > true, while our |context_| is null. I am going to figure out why this is > > happening tomorrow. > > I don't see |is_context_initialized_| being initialized :) to false in > constructor. Thanks Misha! Saved me tons of time in debugging :) I didn't know C++ does not assign a default value. That's very interesting. Thanks again!
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/624443003/480001
Message was sent while issue was closed.
Committed patchset #12 (id:480001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0b21f8f3c41e0bd7e05bdd1f5c59785f8a96edb8 Cr-Commit-Position: refs/heads/master@{#300094}
Message was sent while issue was closed.
On 2014/10/17 12:34:55, xunjieli wrote: > On 2014/10/15 21:47:27, mef wrote: > > On 2014/10/15 21:27:21, xunjieli wrote: > > > There appears to be a race in my code :( Tests fail on my nexus 5, even > though > > > all pass on my nexus 7. It seems that |is_context_initialized_| is evaluated > > to > > > true, while our |context_| is null. I am going to figure out why this is > > > happening tomorrow. > > > > I don't see |is_context_initialized_| being initialized :) to false in > > constructor. > > Thanks Misha! Saved me tons of time in debugging :) I didn't know C++ does not > assign a default value. That's very interesting. Thanks again! You're welcome. Yeah, C++ doesn't assign default value to plain old data (numbers, pointers, etc). To make things more interesting debug builds often initialize it to something predefined (e.g. on MSVC++ on Windows uses 0xCD), but release builds just use whatever happened to be in memory. Sorry that I've missed it in the previous review.
I see! That's very interesting! Thanks for letting me know. It has been a very good learning opportunity :) On Fri, Oct 17, 2014 at 10:39 AM, <mef@chromium.org> wrote: > On 2014/10/17 12:34:55, xunjieli wrote: > >> On 2014/10/15 21:47:27, mef wrote: >> > On 2014/10/15 21:27:21, xunjieli wrote: >> > > There appears to be a race in my code :( Tests fail on my nexus 5, >> even >> though >> > > all pass on my nexus 7. It seems that |is_context_initialized_| is >> > evaluated > >> > to >> > > true, while our |context_| is null. I am going to figure out why this >> is >> > > happening tomorrow. >> > >> > I don't see |is_context_initialized_| being initialized :) to false in >> > constructor. >> > > Thanks Misha! Saved me tons of time in debugging :) I didn't know C++ >> does not >> assign a default value. That's very interesting. Thanks again! >> > > You're welcome. Yeah, C++ doesn't assign default value to plain old data > (numbers, pointers, etc). > To make things more interesting debug builds often initialize it to > something > predefined (e.g. on MSVC++ on Windows uses 0xCD), but release builds just > use > whatever happened to be in memory. > > Sorry that I've missed it in the previous review. > > > > https://codereview.chromium.org/624443003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
And to make things more fun...There *are* default values for POD types, but you only get them in specific cases (Using STL containers is the primary case of this I'm aware of). On Fri, Oct 17, 2014 at 10:46 AM, Helen Li <xunjieli@chromium.org> wrote: > I see! That's very interesting! Thanks for letting me know. It has been a > very good learning opportunity :) > > On Fri, Oct 17, 2014 at 10:39 AM, <mef@chromium.org> wrote: > >> On 2014/10/17 12:34:55, xunjieli wrote: >> >>> On 2014/10/15 21:47:27, mef wrote: >>> > On 2014/10/15 21:27:21, xunjieli wrote: >>> > > There appears to be a race in my code :( Tests fail on my nexus 5, >>> even >>> though >>> > > all pass on my nexus 7. It seems that |is_context_initialized_| is >>> >> evaluated >> >>> > to >>> > > true, while our |context_| is null. I am going to figure out why >>> this is >>> > > happening tomorrow. >>> > >>> > I don't see |is_context_initialized_| being initialized :) to false in >>> > constructor. >>> >> >> Thanks Misha! Saved me tons of time in debugging :) I didn't know C++ >>> does not >>> assign a default value. That's very interesting. Thanks again! >>> >> >> You're welcome. Yeah, C++ doesn't assign default value to plain old data >> (numbers, pointers, etc). >> To make things more interesting debug builds often initialize it to >> something >> predefined (e.g. on MSVC++ on Windows uses 0xCD), but release builds just >> use >> whatever happened to be in memory. >> >> Sorry that I've missed it in the previous review. >> >> >> >> https://codereview.chromium.org/624443003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hmm.. that's indeed more fun! Seems like I have speed up my reading on C++ :) Chris recommended me two books on C++ to read. Barely half way through. On Fri, Oct 17, 2014 at 10:52 AM, Matt Menke <mmenke@chromium.org> wrote: > And to make things more fun...There *are* default values for POD types, > but you only get them in specific cases (Using STL containers is the > primary case of this I'm aware of). > > On Fri, Oct 17, 2014 at 10:46 AM, Helen Li <xunjieli@chromium.org> wrote: > >> I see! That's very interesting! Thanks for letting me know. It has been a >> very good learning opportunity :) >> >> On Fri, Oct 17, 2014 at 10:39 AM, <mef@chromium.org> wrote: >> >>> On 2014/10/17 12:34:55, xunjieli wrote: >>> >>>> On 2014/10/15 21:47:27, mef wrote: >>>> > On 2014/10/15 21:27:21, xunjieli wrote: >>>> > > There appears to be a race in my code :( Tests fail on my nexus 5, >>>> even >>>> though >>>> > > all pass on my nexus 7. It seems that |is_context_initialized_| is >>>> >>> evaluated >>> >>>> > to >>>> > > true, while our |context_| is null. I am going to figure out why >>>> this is >>>> > > happening tomorrow. >>>> > >>>> > I don't see |is_context_initialized_| being initialized :) to false in >>>> > constructor. >>>> >>> >>> Thanks Misha! Saved me tons of time in debugging :) I didn't know C++ >>>> does not >>>> assign a default value. That's very interesting. Thanks again! >>>> >>> >>> You're welcome. Yeah, C++ doesn't assign default value to plain old data >>> (numbers, pointers, etc). >>> To make things more interesting debug builds often initialize it to >>> something >>> predefined (e.g. on MSVC++ on Windows uses 0xCD), but release builds >>> just use >>> whatever happened to be in memory. >>> >>> Sorry that I've missed it in the previous review. >>> >>> >>> >>> https://codereview.chromium.org/624443003/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
pauljensen@chromium.org changed reviewers: + pauljensen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:159: context_builder.set_proxy_config_service(proxy_config_service_.get()); Hey, this looks like a double-free? We keep a scoped_ptr to the ProxyConfigService...and so does the URLRequestContextBuilder (eventually passing ownership along to ProxyService::CreateUsingSystemProxyResolver() and then to ProxyService() )
Message was sent while issue was closed.
On 2015/08/18 12:04:58, pauljensen wrote: > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > File components/cronet/android/url_request_context_adapter.cc (right): > > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > components/cronet/android/url_request_context_adapter.cc:159: > context_builder.set_proxy_config_service(proxy_config_service_.get()); > Hey, this looks like a double-free? We keep a scoped_ptr to the > ProxyConfigService...and so does the URLRequestContextBuilder (eventually > passing ownership along to ProxyService::CreateUsingSystemProxyResolver() and > then to ProxyService() ) Definitely looks like a double free to me.
Message was sent while issue was closed.
On 2015/08/18 12:47:35, mmenke wrote: > On 2015/08/18 12:04:58, pauljensen wrote: > > > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > > File components/cronet/android/url_request_context_adapter.cc (right): > > > > > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > > components/cronet/android/url_request_context_adapter.cc:159: > > context_builder.set_proxy_config_service(proxy_config_service_.get()); > > Hey, this looks like a double-free? We keep a scoped_ptr to the > > ProxyConfigService...and so does the URLRequestContextBuilder (eventually > > passing ownership along to ProxyService::CreateUsingSystemProxyResolver() and > > then to ProxyService() ) > > Definitely looks like a double free to me. Yep, thanks for catching this! When I was writing this CL, I wasn't very clear on how the ownership/scope ptrs works. But I see the problem now. Will upload a fix.
Message was sent while issue was closed.
On 2015/08/18 12:47:35, mmenke wrote: > On 2015/08/18 12:04:58, pauljensen wrote: > > > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > > File components/cronet/android/url_request_context_adapter.cc (right): > > > > > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > > components/cronet/android/url_request_context_adapter.cc:159: > > context_builder.set_proxy_config_service(proxy_config_service_.get()); > > Hey, this looks like a double-free? We keep a scoped_ptr to the > > ProxyConfigService...and so does the URLRequestContextBuilder (eventually > > passing ownership along to ProxyService::CreateUsingSystemProxyResolver() and > > then to ProxyService() ) > > Definitely looks like a double free to me. I've included fix in https://codereview.chromium.org/1303493002 where I'm redoing URLRequestContextBuilder to use scoped_ptr's
Message was sent while issue was closed.
On 2015/08/18 12:53:58, pauljensen wrote: > On 2015/08/18 12:47:35, mmenke wrote: > > On 2015/08/18 12:04:58, pauljensen wrote: > > > > > > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > > > File components/cronet/android/url_request_context_adapter.cc (right): > > > > > > > > > https://codereview.chromium.org/624443003/diff/480001/components/cronet/andro... > > > components/cronet/android/url_request_context_adapter.cc:159: > > > context_builder.set_proxy_config_service(proxy_config_service_.get()); > > > Hey, this looks like a double-free? We keep a scoped_ptr to the > > > ProxyConfigService...and so does the URLRequestContextBuilder (eventually > > > passing ownership along to ProxyService::CreateUsingSystemProxyResolver() > and > > > then to ProxyService() ) > > > > Definitely looks like a double free to me. > > I've included fix in https://codereview.chromium.org/1303493002 where I'm > redoing URLRequestContextBuilder to use scoped_ptr's Awesome! thanks a lot! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
