Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
536023003:
Fix crash in Cronet CreateUrlRequestAdapter. (Closed)
DescriptionFix crash in Cronet CreateUrlRequestAdapter.
- Initialize URL standard schemes on load.
- Use NULL-resistant string conversions.
BUG=410374
Committed: https://crrev.com/9fe3470397eba6ea9e23899efe76c8146811cd1f
Cr-Commit-Position: refs/heads/master@{#293220}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address review comments. #
Messages
Total messages: 15 (2 generated)
mef@chromium.org changed reviewers: + clm@google.com, mmenke@chromium.org, xunjieli@chromium.org
Please take a look. I'll appreciate suggestions on how to test this.
https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... File components/cronet/android/cronet_jni.cc (right): https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... components/cronet/android/cronet_jni.cc:63: url::Shutdown(); I don't think this actually ever gets called. http://bleaklow.com/2006/02/18/jni_onunload_mostly_useless.html
https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... File components/cronet/android/cronet_jni.cc (right): https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... components/cronet/android/cronet_jni.cc:63: url::Shutdown(); On 2014/09/03 18:28:20, Charles wrote: > I don't think this actually ever gets called. > http://bleaklow.com/2006/02/18/jni_onunload_mostly_useless.html Yeah, but if it ever does we will do the right thing. :)
lgtm
lgtm , though not sure how we can test for this. It will be good if we discover this in tests early.
https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... components/cronet/android/chromium_url_request.cc:122: base::android::ConvertJavaStringToUTF8(env, url_jstring)); optional: Think it's cleaner to just do: GURL url(base::android::....); and get rid of the string intermediary, and just use url.spec() in the VLOG. That does mean if GURL's constructor crashes, we won't log anything, or if it corrupts the input, we'll log the wrong thing, but I'm skeptical about whether that actually gets us a whole lot. Feel free to disagree, it's quite possible I undervalue VLOG statements, as I never use them for debugging. https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... File components/cronet/android/cronet_jni.cc (right): https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... components/cronet/android/cronet_jni.cc:63: url::Shutdown(); On 2014/09/03 18:32:38, mef wrote: > On 2014/09/03 18:28:20, Charles wrote: > > I don't think this actually ever gets called. > > http://bleaklow.com/2006/02/18/jni_onunload_mostly_useless.html > > Yeah, but if it ever does we will do the right thing. :) Is this the right thing? Can we be guaranteed that all network threads have been killed at this point? I'm not sure we can be, so we may still be calling into GURL functions, at least destructors. Seems safer to just leak here.
On 2014/09/03 18:42:16, xunjieli1 wrote: > lgtm , though not sure how we can test for this. It will be good if we discover > this in tests early. We could make a test where we flood requests from different threads on startup - we actually have NetLog tests that do that on startup. I'm a bit skeptical of tests that do that (For all that I wrote the aforementioned ones), but it may be worth considering, to try and uncover any threading issues.
Thanks, PTAL. https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... File components/cronet/android/chromium_url_request.cc (right): https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... components/cronet/android/chromium_url_request.cc:122: base::android::ConvertJavaStringToUTF8(env, url_jstring)); On 2014/09/03 18:43:06, mmenke wrote: > optional: Think it's cleaner to just do: > GURL url(base::android::....); and get rid of the string intermediary, and just > use url.spec() in the VLOG. > > That does mean if GURL's constructor crashes, we won't log anything, or if it > corrupts the input, we'll log the wrong thing, but I'm skeptical about whether > that actually gets us a whole lot. Feel free to disagree, it's quite possible I > undervalue VLOG statements, as I never use them for debugging. Done. I haven't really used VLOGs either so far. https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... File components/cronet/android/cronet_jni.cc (right): https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... components/cronet/android/cronet_jni.cc:63: url::Shutdown(); On 2014/09/03 18:43:06, mmenke wrote: > On 2014/09/03 18:32:38, mef wrote: > > On 2014/09/03 18:28:20, Charles wrote: > > > I don't think this actually ever gets called. > > > http://bleaklow.com/2006/02/18/jni_onunload_mostly_useless.html > > > > Yeah, but if it ever does we will do the right thing. :) > > Is this the right thing? Can we be guaranteed that all network threads have > been killed at this point? I'm not sure we can be, so we may still be calling > into GURL functions, at least destructors. > > Seems safer to just leak here. Done.
On 2014/09/03 18:56:04, mef wrote: > Thanks, PTAL. > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... > File components/cronet/android/chromium_url_request.cc (right): > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... > components/cronet/android/chromium_url_request.cc:122: > base::android::ConvertJavaStringToUTF8(env, url_jstring)); > On 2014/09/03 18:43:06, mmenke wrote: > > optional: Think it's cleaner to just do: > > GURL url(base::android::....); and get rid of the string intermediary, and > just > > use url.spec() in the VLOG. > > > > That does mean if GURL's constructor crashes, we won't log anything, or if it > > corrupts the input, we'll log the wrong thing, but I'm skeptical about whether > > that actually gets us a whole lot. Feel free to disagree, it's quite possible > I > > undervalue VLOG statements, as I never use them for debugging. > > Done. I haven't really used VLOGs either so far. > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... > File components/cronet/android/cronet_jni.cc (right): > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... > components/cronet/android/cronet_jni.cc:63: url::Shutdown(); > On 2014/09/03 18:43:06, mmenke wrote: > > On 2014/09/03 18:32:38, mef wrote: > > > On 2014/09/03 18:28:20, Charles wrote: > > > > I don't think this actually ever gets called. > > > > http://bleaklow.com/2006/02/18/jni_onunload_mostly_useless.html > > > > > > Yeah, but if it ever does we will do the right thing. :) > > > > Is this the right thing? Can we be guaranteed that all network threads have > > been killed at this point? I'm not sure we can be, so we may still be calling > > into GURL functions, at least destructors. > > > > Seems safer to just leak here. > > Done. LGTM. Hrm...If we're ever unloaded, we'd probably have to add a block on all network threads shutting down, anyways, so my shutdown concerns were probably irrelevant, regardless. :)
On 2014/09/03 19:15:39, mmenke wrote: > On 2014/09/03 18:56:04, mef wrote: > > Thanks, PTAL. > > > > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... > > File components/cronet/android/chromium_url_request.cc (right): > > > > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/ch... > > components/cronet/android/chromium_url_request.cc:122: > > base::android::ConvertJavaStringToUTF8(env, url_jstring)); > > On 2014/09/03 18:43:06, mmenke wrote: > > > optional: Think it's cleaner to just do: > > > GURL url(base::android::....); and get rid of the string intermediary, and > > just > > > use url.spec() in the VLOG. > > > > > > That does mean if GURL's constructor crashes, we won't log anything, or if > it > > > corrupts the input, we'll log the wrong thing, but I'm skeptical about > whether > > > that actually gets us a whole lot. Feel free to disagree, it's quite > possible > > I > > > undervalue VLOG statements, as I never use them for debugging. > > > > Done. I haven't really used VLOGs either so far. > > > > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... > > File components/cronet/android/cronet_jni.cc (right): > > > > > https://codereview.chromium.org/536023003/diff/1/components/cronet/android/cr... > > components/cronet/android/cronet_jni.cc:63: url::Shutdown(); > > On 2014/09/03 18:43:06, mmenke wrote: > > > On 2014/09/03 18:32:38, mef wrote: > > > > On 2014/09/03 18:28:20, Charles wrote: > > > > > I don't think this actually ever gets called. > > > > > http://bleaklow.com/2006/02/18/jni_onunload_mostly_useless.html > > > > > > > > Yeah, but if it ever does we will do the right thing. :) > > > > > > Is this the right thing? Can we be guaranteed that all network threads have > > > been killed at this point? I'm not sure we can be, so we may still be > calling > > > into GURL functions, at least destructors. > > > > > > Seems safer to just leak here. > > > > Done. > > LGTM. Hrm...If we're ever unloaded, we'd probably have to add a block on all > network threads shutting down, anyways, so my shutdown concerns were probably > irrelevant, regardless. :) Thanks! Yeah, I think unload scenario would require a few changes.
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/536023003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as 510084ce227e21dbc57e4e09ea8f59ad5def1a80
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9fe3470397eba6ea9e23899efe76c8146811cd1f Cr-Commit-Position: refs/heads/master@{#293220} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
