Thank you for the bug report!

https://crrev.com/c/979334 adds a call to AddGlobalChangeListener in the CookieManager mojo interface. The implementation uses AddCallbackForAllChanges(), so it was technically always broken on Android. This was used in chrome/browser/extensions/extension_cookie_notifier.cc which is desktop-only, and my CL added a call in src/content/browser/cookie_store/cookie_store_manager.cc which is used both on Desktop and Android.

Marking this as blocking the Async Cookies API.

ntfschr@: How would I run the integration tests?

http://go/clank-webview/testing-webview. See "instrumentation tests." Make sure DCHECKs are enabled.

---

As a general question (not directed specifically at pwnall@): How did this even land on trunk? Do we not build with DCHECKs for webview instrumentation on trunk? That seems ridiculous...

Thank you very much for the pointer to the integration tests!

"ridiculous" might be perceived as hostile, I think it's better to phrase things in terms of opportunities for improvement :)

My apologies--hostility was not my intention.

I think our product's stability will be greatly improved if we can have DCHECKs enabled during CQ--this seems ideal to prevent other layers from accidentally violating DCHECK assertions in the android_webview/ layer. I was under the (mistaken) impression this was already the case.

cq bots that run tests are all release builds with dcheck_always_on=true. debug builds are just too slow. I'm not sure what happens with NOTIMPLEMENTED in that configuration though, have to look up the implementation.

TL;DR I confused NOTIMPLEMENTED (which is a log message) with NOTREACHED (which crashes when DCHECKs are enabled).

NOTIMPLEMENTED is unrelated to DCHECKs. This is just LOG(ERROR) [1], and doesn't crash. Documentation is ambiguous, but I interpret NOTIMPLEMENTED as, "this method isn't actually implemented yet, but it's OK if we call it on accident, and the caller should behave reasonably if it calls this." This is my first time seeing NOTIMPLEMENTED, so correct me if I'm wrong.

Based on this, it seems like there was no bug for CQ to catch, so CQ is working as intended.

When I ran the tests locally, I thought I needed to comment out the NOTIMPLEMENTED to avoid a crash. I must have been mistaken, or something else must have caused the crash.

So, I'll lower the priority since this was my misunderstanding. pwnall@ can re-adjust priority as appropriate.

[1] https://cs.chromium.org/chromium/src/base/logging.h?l=1169&rcl=10bcc4d1db5c4b506aff7fa9f5e9e0d8226c84c6

Hm, I was not able to reproduce the crashes. The bug makes sense to me and I think I can fix it without the repro, but it'd be nice to have that and confirm that crashes are gone.

My gn configuration:

is_component_build = true
is_debug = true
target_os = "android"
use_goma = true

The tests I've tried:

$ out/Android/bin/run_webview_instrumentation_test_apk -f "AwContentsClientFullScreenTest.*"
C  148.672s Main  ********************************************************************************
C  148.672s Main  Summary
C  148.672s Main  ********************************************************************************
C  148.672s Main  [==========] 16 tests ran.
C  148.672s Main  [  PASSED  ] 16 tests.
C  148.673s Main  ********************************************************************************

$ out/Android/bin/run_webview_instrumentation_test_apk -f "SafeBrowsingTest.*"
C  510.898s Main  ********************************************************************************
C  510.899s Main  Summary
C  510.899s Main  ********************************************************************************
C  510.899s Main  [==========] 80 tests ran.
C  510.899s Main  [  PASSED  ] 80 tests.
C  510.899s Main  ********************************************************************************

I'm using a Pixel C on OPM4. 

> Hm, I was not able to reproduce the crashes.

See #c7. I think this is only logspam, not a crash (not sure where my crash was coming from). You can check the log with:

$ adb logcat | grep 'Not implemented reached in'

ntfschr@: Hm, I think I added NOTIMPLEMENTED because I saw it in other cookie change dispatch code, or because of review feedback. I'll admit that at the moment I thought it's equivalent to NOTREACHED, so I thought that switching from CHECK to NOTIMPLEMENTED was a no-op.


boliu@: Do we run layout tests on WebView?

> boliu@: Do we run layout tests on WebView?

I believe we run layout tests on android using content shell, which doesn't have any code from chrome/ or android_webview/