The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9

commit f6d2732a2b81694b3d7ee47affaa6a22ee405cf9
Author: bauerb <bauerb@chromium.org>
Date: Wed Mar 30 17:02:48 2016

DCHECK that jstring to C++ string conversions don't pass in null.

Also, update existing callers to avoid the call if they would otherwise trip the DCHECK now.

BUG=597564

Review URL: https://codereview.chromium.org/1828193002

Cr-Commit-Position: refs/heads/master@{#384010}

[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/base/android/content_uri_utils.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/base/android/jni_string.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/prerender/external_prerender_handler_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/ui/android/context_menu_helper.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/ui/android/context_menu_helper.h
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/components/gcm_driver/gcm_driver_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9

commit f6d2732a2b81694b3d7ee47affaa6a22ee405cf9
Author: bauerb <bauerb@chromium.org>
Date: Wed Mar 30 17:02:48 2016

DCHECK that jstring to C++ string conversions don't pass in null.

Also, update existing callers to avoid the call if they would otherwise trip the DCHECK now.

BUG=597564

Review URL: https://codereview.chromium.org/1828193002

Cr-Commit-Position: refs/heads/master@{#384010}

[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/base/android/content_uri_utils.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/base/android/jni_string.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/prerender/external_prerender_handler_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/ui/android/context_menu_helper.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/chrome/browser/ui/android/context_menu_helper.h
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/components/gcm_driver/gcm_driver_android.cc
[modify] https://crrev.com/f6d2732a2b81694b3d7ee47affaa6a22ee405cf9/ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java

Can you elaborate on the motivation for this change?  Removing a null-check from a popular utility function seems akin to running with scissors.  This change has crashed all the Cronet tests, see  Issue 599331 .

This is *adding* a null check, instead of silently (well, actually noisily, but nobody ever reads those log messages) assuming that converting null to "" will be fine for the recipient.

The cronet bot should be in the tryjob set if it's at risk of being broken by base changes.

Changing a near-silent conversion of NULL into a SIGSEGV on release builds seems like it's going to introduce latent crashes into our products.  Why not add another function that DCHECKs on NULL?

Crashes are the only way for developers to learn about something going wrong (as evidenced by the fact that no one cared about the logspam e.g. for cronet).

The contract for ConvertJavaStringToUTF{8,16} already was before my CL that passing in null was not allowed, it just happened not to enforce that.

Having a utility function convert NULL into an empty string doesn't strike me as "something going wrong".  This utility function is used for JNI, where I think passing null is 8000+ clock cycles faster than passing an empty string (JNI string conversion takes 4.3us on a 2.26GHz Nexus 5, while passing null I think is virtually free, see https://docs.google.com/a/google.com/spreadsheets/d/1ulggKOZ7VQ8IytP1hKr-1wvBu70qG2qxKT4vRbHDIGw/edit?usp=sharing ).

Note that we don't actually pass in an empty string to ConvertJavaStringToUTF{8,16}; in the places I've modified in the CL above we try to skip the call if the string is null (which is actually the same thing we did before, but now we do it at the call site, which is the better place to decide how to handle a null string).

I'm all for adding a new utility function that DCHECKs on NULL, as this gives us a better understanding of what expectations callers have.  I'm just quite scared to take a very popular utility function and make it start SEGVing on input that it used to accept (I'm also not opposed to removing the log statement that everyone was ignoring).  I don't think we have nearly enough test coverage to know if that is safe.

The latent crashers I was talking about are starting to come in:

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27base%3A%3Aandroid%3A%3AConvertJavaStringToUTF8%27%20OMIT%20RECORD%20IF%20SUM(CrashedStackTrace.StackFrame.SourceFileName%3D%27%2Fb%2Fbuild%2Fslave%2Fofficial-arm_64%2Fbuild%2Fsrc%2Fout%2FRelease%2F..%2F..%2Fthird_party%2Fandroid_tools%2Fndk%2Fplatforms%2Fandroid-21%2Farch-arm64%2Fusr%2Finclude%2Fjni.h%27)%20%3D%200%20OR%20SUM(CrashedStackTrace.StackFrame.SourceLine%3D864)%20%3D%200&ignore_case=false&enable_rewrite=false&omit_field_name=&omit_field_value=&omit_field_opt=#samplereports

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27base%3A%3Aandroid%3A%3AConvertJavaStringToUTF16%27%20OMIT%20RECORD%20IF%20SUM(CrashedStackTrace.StackFrame.SourceFileName%3D%27%2Fb%2Fbuild%2Fslave%2Fofficial-arm_64%2Fbuild%2Fsrc%2Fout%2FRelease%2F..%2F..%2Fthird_party%2Fandroid_tools%2Fndk%2Fplatforms%2Fandroid-21%2Farch-arm64%2Fusr%2Finclude%2Fjni.h%27)%20%3D%200%20OR%20SUM(CrashedStackTrace.StackFrame.SourceLine%3D864)%20%3D%200&ignore_case=false&enable_rewrite=false&omit_field_name=&omit_field_value=&omit_field_opt=&stbtiq=&reportid=&index=0

Thanks! All of these crashes are already fixed on trunk (https://codereview.chromium.org/1860933002/, https://codereview.chromium.org/1842393002/ and https://crrev.com/1856753002), except for a single crash with a new stack trace, for which I've filed  issue 600753 .

There's too many cases of this in WebView right now, unfortunately (webview is too trusting of application API calls, and lets apps pass null in a way that propagates all the way down to native and crashes here). We can't afford this in M51 right now, sorry. I'm going to partially undo your change here: keep the DCHECK, but still return empty string in release builds. I'll merge this to M51, the webview team will try to clean up the webview code, and we'll have another go at making it fatal for real later. :/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/075418c032dd093e52aef9e56b8a3eb55eb7dc58

commit 075418c032dd093e52aef9e56b8a3eb55eb7dc58
Author: torne <torne@chromium.org>
Date: Fri Apr 22 18:13:35 2016

Allow null jstring to C++ string conversions in release.

WebView has too many cases where apps can pass in a null java string and
have it reach native causing a crash. For now, put back the code that
returned empty string, but keep the DCHECK so that we still hit it on
bots that run with DCHECKs enabled. We'll fix WebView and have another
go at this in a later release.

This is a partial revert of https://codereview.chromium.org/1828193002

BUG=597564

Review URL: https://codereview.chromium.org/1913183002

Cr-Commit-Position: refs/heads/master@{#389166}

[modify] https://crrev.com/075418c032dd093e52aef9e56b8a3eb55eb7dc58/base/android/jni_string.cc

Requesting merge of commit from #14 to M51, so we can safely ship webview.

Your change meets the bar and is auto-approved for M51 (branch: 2704)

Please merge your change to M51 branch 2704 before 5:00 PM PST Monday (04/25/16) so we can take it for next week M51 Beta candidate cut. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35f63706d80bd84b2f5ccd5e5f251243a7805731

commit 35f63706d80bd84b2f5ccd5e5f251243a7805731
Author: Torne (Richard Coles) <torne@chromium.org>
Date: Mon Apr 25 10:10:09 2016

Allow null jstring to C++ string conversions in release.

WebView has too many cases where apps can pass in a null java string and
have it reach native causing a crash. For now, put back the code that
returned empty string, but keep the DCHECK so that we still hit it on
bots that run with DCHECKs enabled. We'll fix WebView and have another
go at this in a later release.

This is a partial revert of https://codereview.chromium.org/1828193002

BUG=597564

Review URL: https://codereview.chromium.org/1913183002

Cr-Commit-Position: refs/heads/master@{#389166}
(cherry picked from commit 075418c032dd093e52aef9e56b8a3eb55eb7dc58)

Review URL: https://codereview.chromium.org/1919753002 .

Cr-Commit-Position: refs/branch-heads/2704@{#211}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/35f63706d80bd84b2f5ccd5e5f251243a7805731/base/android/jni_string.cc

OK, problem solved for M51. I'll keep this for now to track fixing up webview to not rely on null conversions.

You started fixing this bug over two years ago. Are you still working on it? 

We haven't fixed all the cases yet. :(