Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1850573003:
CroNet: Allow null strings in config when creating a UrlRequestContextConfig (Closed)
|
Created:
4 years, 8 months ago by Bernhard Bauer Modified:
4 years, 8 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCroNet: Allow null strings in config when creating a UrlRequestContextConfig.
BUG=599331
Committed: https://crrev.com/1d9c5cae125a244f561276d2e8824dd2ec04dcdc
Cr-Commit-Position: refs/heads/master@{#384355}
Patch Set 1 #Patch Set 2 : review #Messages
Total messages: 18 (6 generated)
Description was changed from ========== Allow null strings in CronetUrlRequestContextAdapter. BUG=599331 ========== to ========== CroNet: Allow null strings in config when creating a UrlRequestContextConfig BUG=599331 ==========
Description was changed from ========== CroNet: Allow null strings in config when creating a UrlRequestContextConfig BUG=599331 ========== to ========== CroNet: Allow null strings in config when creating a UrlRequestContextConfig. BUG=599331 ==========
bauerb@chromium.org changed reviewers: + pauljensen@chromium.org
Please review.
This adds a lot of code to do the same thing we were doing before. How about adding a utility function that accepts nullptr? Have you analyzed every ConvertJavaStringToUTF8 call in Cronet? I see lots of other files in Cronet that do this: https://code.google.com/p/chromium/codesearch#search/&q=ConvertJavaStringToUT...
On 2016/03/31 13:25:50, pauljensen wrote: > This adds a lot of code to do the same thing we were doing before. How about > adding a utility function that accepts nullptr? Done. > Have you analyzed every ConvertJavaStringToUTF8 call in Cronet? I see lots of > other files in Cronet that do this: > https://code.google.com/p/chromium/codesearch#search/&q=ConvertJavaStringToUT... I have not; I'm assuming that the current tests have enough coverage to exercise all of these codepaths.
On 2016/03/31 14:08:55, Bernhard Bauer wrote: > On 2016/03/31 13:25:50, pauljensen wrote: > > Have you analyzed every ConvertJavaStringToUTF8 call in Cronet? I see lots of > > other files in Cronet that do this: > > > https://code.google.com/p/chromium/codesearch#search/&q=ConvertJavaStringToUT... > > I have not; I'm assuming that the current tests have enough coverage to exercise > all of these codepaths. I have little faith that even robust amounts of testing will exercise all of these codepaths. ConvertJavaStringToUTF8 isn't documented to SEGV/DCHECK on null. Can we revert the prior change and add a new API that converts and includes the DCHECK for null?
On 2016/03/31 15:58:34, pauljensen wrote: > On 2016/03/31 14:08:55, Bernhard Bauer wrote: > > On 2016/03/31 13:25:50, pauljensen wrote: > > > Have you analyzed every ConvertJavaStringToUTF8 call in Cronet? I see lots > of > > > other files in Cronet that do this: > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=ConvertJavaStringToUT... > > > > I have not; I'm assuming that the current tests have enough coverage to > exercise > > all of these codepaths. > > I have little faith that even robust amounts of testing will exercise all of > these codepaths. ConvertJavaStringToUTF8 isn't documented to SEGV/DCHECK on > null. Can we revert the prior change and add a new API that converts and > includes the DCHECK for null? That doesn't seem to make any sense as a path forward, because the logical next step would be "go through every use of ConvertJavaStringToUTF8 in the codebase and check whether it needs to be able to accept null, and if not change it to use the new API", which would end up having pretty much the same effect as the change that already landed, except with huge amounts of extra human effort and churn. :/
On 2016/03/31 16:22:30, Torne wrote: > On 2016/03/31 15:58:34, pauljensen wrote: > > On 2016/03/31 14:08:55, Bernhard Bauer wrote: > > > On 2016/03/31 13:25:50, pauljensen wrote: > > > > Have you analyzed every ConvertJavaStringToUTF8 call in Cronet? I see > lots > > of > > > > other files in Cronet that do this: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=ConvertJavaStringToUT... > > > > > > I have not; I'm assuming that the current tests have enough coverage to > > exercise > > > all of these codepaths. > > > > I have little faith that even robust amounts of testing will exercise all of > > these codepaths. ConvertJavaStringToUTF8 isn't documented to SEGV/DCHECK on > > null. Can we revert the prior change and add a new API that converts and > > includes the DCHECK for null? Also: null is not a java string (it's the absence of a java object), so why would you expect that ConvertJavaStringToUTF8 can accept null? ConvertUTF8ToJavaString can't accept null either.
On 2016/03/31 16:22:30, Torne wrote: > On 2016/03/31 15:58:34, pauljensen wrote: > > On 2016/03/31 14:08:55, Bernhard Bauer wrote: > > > On 2016/03/31 13:25:50, pauljensen wrote: > > > > Have you analyzed every ConvertJavaStringToUTF8 call in Cronet? I see > lots > > of > > > > other files in Cronet that do this: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=ConvertJavaStringToUT... > > > > > > I have not; I'm assuming that the current tests have enough coverage to > > exercise > > > all of these codepaths. > > > > I have little faith that even robust amounts of testing will exercise all of > > these codepaths. ConvertJavaStringToUTF8 isn't documented to SEGV/DCHECK on > > null. Can we revert the prior change and add a new API that converts and > > includes the DCHECK for null? > > That doesn't seem to make any sense as a path forward, because the logical next > step would be "go through every use of ConvertJavaStringToUTF8 in the codebase > and check whether it needs to be able to accept null, and if not change it to > use the new API", which would end up having pretty much the same effect as the > change that already landed, except with huge amounts of extra human effort and > churn. :/ By adding the SEGVs before doing the investigation as to which calls are safe to convert you're putting the cart before the horse, and leaving the codebase open to random crashers until the investigation is done. Furthermore, I have no guarantee you'll go through each call in Cronet and do the necessary investigation. Bernhard's response in comment #6 on this CL indicated no investigation would be done, and that he was relying on test coverage.
I'm going to commit this as I need Cronet tests to start passing again. I'm still of the opinion we should revert the original change that made this SEGV.
The CQ bit was checked by pauljensen@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1850573003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1850573003/20001
Message was sent while issue was closed.
Description was changed from ========== CroNet: Allow null strings in config when creating a UrlRequestContextConfig. BUG=599331 ========== to ========== CroNet: Allow null strings in config when creating a UrlRequestContextConfig. BUG=599331 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== CroNet: Allow null strings in config when creating a UrlRequestContextConfig. BUG=599331 ========== to ========== CroNet: Allow null strings in config when creating a UrlRequestContextConfig. BUG=599331 Committed: https://crrev.com/1d9c5cae125a244f561276d2e8824dd2ec04dcdc Cr-Commit-Position: refs/heads/master@{#384355} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1d9c5cae125a244f561276d2e8824dd2ec04dcdc Cr-Commit-Position: refs/heads/master@{#384355} |
