Looks like we have null check for url in AwContents.loadUrl(String) [1] but don't have one for AwContents.loadUrl(String, Map<String, String>).

ConvertJavaStringToUTF8() [2] looks a little weird to me, we have a DCHECK(str) then immediately we check |if (!str)| for logging purpose.

torne@, shall we add a null check to loadUrl(String, Map<String, String>) as the same for loadUrl(String)?

[1] https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwContents.java?rcl=d48e2e1661083cf905857bb985bea02609bd5a93&l=1604
[2] https://cs.chromium.org/chromium/src/base/android/jni_string.cc?l=27&rcl=1071f3681c3f98422d8561b888e27a25a843992f

> ConvertJavaStringToUTF8() [2] looks a little weird to me, we have a DCHECK(str) then immediately we check |if (!str)| for logging purpose.

I believe this is a violation of the style guide [1]:

> Attempting to handle a DCHECK() failure is a statement that the DCHECK() can fail, which contradicts the point of writing the DCHECK()

Skimming the CL, it sounds like this was intended to be a temporary fix, which has unfortunately been not-so-temporary. Can we add a better fix in the AW-layer?

[1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#check_dcheck_and-notreached

So we've had issue 597564 open for a long time about ConvertJavaStringToUTF8 - we should fix WebView to stop passing null to it, so that we can eventually remove the null check and just get segfaults as per the style guide :)

There are many places in WebView where we trust the caller not to pass in null and we should not - we should check it in Java and reject it in an appropriate way for the particular API call (if nothing else, just throwing NullPointerException), before we get to the point of JNI.

If someone wants to take over that issue and the related blockers from me, they're welcome to, I haven't had a chance to look at this for a long time and had forgotten about it entirely :(

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

commit 941617afc5fd790da4079f537b88733ae8f25675
Author: Hazem Ashmawy <hazems@chromium.org>
Date: Wed Oct 17 10:21:05 2018

[aw] Early return in loadUrl

loadUrl(String) has an early return if url == null, this is just adding the same early return to loadUrl(String, Map<String,String>). This should prevent sending null values to AwContents::SetExtraHeadersForUrl and then to ConvertJavaStringToUTF8.

Bug:  864708 
Change-Id: If169dbc89dfdf745e7fa27921fa7624abed40a84
Reviewed-on: https://chromium-review.googlesource.com/c/1283449
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Hazem Ashmawy <hazems@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600338}
[modify] https://crrev.com/941617afc5fd790da4079f537b88733ae8f25675/android_webview/java/src/org/chromium/android_webview/AwContents.java

Maybe we can condition the null-handling on a base::Feature, and experiment via Finch to see if this fix was good enough? paulmiller@ do we have infra to associate a spike in a particular crash with a culprit Finch experiment?

Currently, no, because we don't log finch experiments in crash dumps. This will only suppress an existing crash, and we should be able to find those crashes in crash/ already. I don't think this is worthy of a finch experiment.

However, we're now turning a crash into an no-op, and so we should be looking for a way to alert developers to the fact that null is being passed.

> I don't think this is worthy of a finch experiment.

Fair enough. But is this enough to try removing the null-check-after-dcheck (as per comment #2)? I would support removing that as soon as possible.

Also, maybe we should add @NonNull to the loadUrl definitions in frameworks?

No, I don't think we can remove the null-after-dcheck. The string conversion method is reached by a lot of paths, and I think enumerating them is going to be a long process.

Generally, adding @NonNull/@Nullable to the framework APIs where it's possible to do so would be a worthwhile project.

> Generally, adding @NonNull/@Nullable to the framework APIs where it's possible to do so would be a worthwhile project.

I tackled some low hanging fruit last year. Unfortunately, I don't think the annotations show up in Studio (although, maybe they would someday). It does auto-add javadoc which says "don't pass null", which is already a good thing.

> I don't think we can remove the null-after-dcheck.

Then maybe we should give up and remove the DCHECK. Or, we could turn this into a CHECK() (temporarily on betas) to get call stacks.

Do you mean the DCHECK in ConvertJavaStringToUTF8? That just needs someone to fix all the cases, which were mostly in WebView; the DCHECK will be preventing at least some proportion of potential regressions here. Making it a CHECK on dev or beta might help find remaining cases, yeah. If someone wants to take issue 597564 from me to fix that, they should feel free, but this bug should be fine to close since it's just about loadUrl.

> but this bug should be fine to close since it's just about loadUrl.

SGTM. I'll leave this for hazems@ to close, in case there's something else planned for this bug.

Well, when tobiasjs@ assigned the bug to me, he suggested ignoring null headers and/or values in LoadUrlParams but I think it's more related to issue 597564 and 605903 so I'm going to close the bug now.

I think we can mark this as "Verified", because I don't think there's any obvious way for our testers to manually verify (otherwise, Fixed bugs show up in their TODO queue).