Run CTS failing on 2 builders |
||||
Issue descriptionRun CTS failing on 2 builders Builders failed on: - Android WebView L (dbg): https://build.chromium.org/p/chromium.android/builders/Android%20WebView%20L%20%28dbg%29 - Android WebView M (dbg): https://build.chromium.org/p/chromium.android/builders/Android%20WebView%20M%20%28dbg%29 First failing build: https://build.chromium.org/p/chromium.android/builders/Android%20WebView%20L%20%28dbg%29/builds/5033
,
May 17 2017
umm, yeah, that will break webview :/ webview runs cookie manager things before knowing what the UI thread is. also application status listener doesn't work on webview at all. can we revert first and figure out what to do later? Native stack: 0036ff13 base::debug::BreakDebugger()+18 /b/c/b/Android_arm_Builder__dbg_/src/base/debug/debugger_posix.cc:251 00385da3 logging::LogMessage::~LogMessage()+846 /b/c/b/Android_arm_Builder__dbg_/src/base/logging.cc:783 0035e367 base::android::CheckException(_JNIEnv*)+126 /b/c/b/Android_arm_Builder__dbg_/src/base/android/jni_android.cc:243 v------> jni_generator::CheckException(_JNIEnv*) /b/c/b/Android_arm_Builder__dbg_/src/base/android/jni_generator/jni_generator_helper.h:42 v------> Java_ApplicationStatus_registerThreadSafeNativeApplicationStateListener /b/c/b/Android_arm_Builder__dbg_/src/out/Debug/gen/base/base_jni_headers/base/jni/ApplicationStatus_jni.h:86 0035a8c9 base::android::ApplicationStatusListener::ApplicationStatusListener(base::Callback<void (base::android::ApplicationState), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&)+208 /b/c/b/Android_arm_Builder__dbg_/src/base/android/application_status_listener.cc:45 02776261 net::SQLitePersistentCookieStore::Backend::InitializeDatabase()+188 /b/c/b/Android_arm_Builder__dbg_/src/net/extras/sqlite/sqlite_persistent_cookie_store.cc:645 02777131 net::SQLitePersistentCookieStore::Backend::LoadAndNotifyInBackground(base::Callback<void (std::__ndk1::vector<std::__ndk1::unique_ptr<net::CanonicalCookie, std::__ndk1::default_delete<net::CanonicalCookie> >, std::__ndk1::allocator<std::__ndk1::unique_ptr<net::CanonicalCookie, std::__ndk1::default_delete<net::CanonicalCookie> > > >), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, base::Time const&)+224 /b/c/b/Android_arm_Builder__dbg_/src/net/extras/sqlite/sqlite_persistent_cookie_store.cc:520 v------> base::Callback<void (), (base::internal::CopyMode)0, (base::internal::RepeatMode)0>::Run() && /b/c/b/Android_arm_Builder__dbg_/src/base/callback.h:91 0037091b base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)+634 /b/c/b/Android_arm_Builder__dbg_/src/base/debug/task_annotator.cc:59 0038cbff base::MessageLoop::RunTask(base::PendingTask*)+442 /b/c/b/Android_arm_Builder__dbg_/src/base/message_loop/message_loop.cc:404 0038cecb base::MessageLoop::DeferOrRunPendingTask(base::PendingTask)+24 /b/c/b/Android_arm_Builder__dbg_/src/base/message_loop/message_loop.cc:415 0038cfb5 base::MessageLoop::DoWork()+144 /b/c/b/Android_arm_Builder__dbg_/src/base/message_loop/message_loop.cc:503 0038f153 base::MessagePumpDefault::Run(base::MessagePump::Delegate*)+90 /b/c/b/Android_arm_Builder__dbg_/src/base/message_loop/message_pump_default.cc:33 0038e22b base::MessageLoop::RunHandler()+122 /b/c/b/Android_arm_Builder__dbg_/src/base/message_loop/message_loop.cc:368 003ab41b base::RunLoop::Run()+86 /b/c/b/Android_arm_Builder__dbg_/src/base/run_loop.cc:105 003cdc63 base::Thread::Run(base::RunLoop*)+126 /b/c/b/Android_arm_Builder__dbg_/src/base/threading/thread.cc:255 003ce975 base::Thread::ThreadMain()+676 /b/c/b/Android_arm_Builder__dbg_/src/base/threading/thread.cc:338 003c914d base::(anonymous namespace)::ThreadFunc(void*)+60 /b/c/b/Android_arm_Builder__dbg_/src/base/threading/platform_thread_posix.cc:71 00016baf __pthread_start(void*)+30 /system/lib/libc.so 00014af3 __start_thread+6
,
May 17 2017
Reverted (gah, forgot to update the reason): https://chromium-review.googlesource.com/c/508190 nharper@: I think you can run this test locally by building/installing system_webview_apk and running something like "android_webview/tools/run_cts.py --arch arm_64 --platform L -f android.webkit.cts.WebViewStartupTest#testCookieManagerBlockingUiThread -v"
,
May 17 2017
Please do not use ApplicationStatusListener from any code outside of src/chrome. It does not belong in base at all, and cannot safely be depended on by non-chrome code - it's just that the work to migrate it out of base hasn't been done yet. :( If you want to use this in the network stack, you need to have it be something that the content embedder tells the network stack about (and default to a safe behaviour in case the embedder is unable to provide this signal).
,
May 17 2017
I thought that the webview consumer managed cookie storage, so I'm a bit surprised this is creating a net::SQLitePersistentCookieStore at all. If the application status listener doesn't work on webview at all, I'm fine disabling it. Is there any way in net::SQLitePersistentCookieStore to check if we're running as part of webview? If that's not possible, can ApplicationStatusListener be fixed to be a no-op in webview?
,
May 17 2017
Re #4: We've been using ApplicationStatusListener (or predecessors) for over 4 years in //net - see https://codereview.chromium.org/14771009. This was the advice from dskiba and pasko on 719726.
,
May 17 2017
Yes, that kind of thing is what needs to be fixed before I can move ApplicationStatusListener out of base. :( That particular case happens not to directly break WebView since it just never gets invoked, but the reason to remove it is to avoid people relying on it without realising that.
,
May 18 2017
And no, please don't fix this by checking if you're part of WebView, or by making it a no-op in WebView - please just have the content embedder tell the network stack when it's a good time to flush pending state to disk, or some other *specific* signal that is actually instructing the network stack what to do rather than just informing it about the "state of the application", which is encoding policy into net in a way that seems undesirable.
,
May 18 2017
Is there a bug that's tracking the work to move ApplicationStatusListener out of base? I'll set crbug.com/719726 to be blocked on that bug.
,
May 18 2017
Issue 470582. Not a lot of progress has been made on this; the initial focus was removing ApplicationStatus.getApplicationContext, which is now done, but doesn't help you here. I'm not currently actively working on this I'm afraid, due to lack of time, so if you plan to work on issue 719726 soon then you should just proceed with doing it as I described in c#8 - as long as all the calls to ApplicationStatus are up in src/chrome it's fine, I can do the refactoring mechanically when the time comes to actually move it. It's only an issue if there are actual calls to it in lower layers.
,
May 18 2017
quite eye-opening about the simplecache use of ApplicationStatusListener. I agree that content embedder telling //net when to flush would be a cleaner design. I did not know that instantiation of the ApplicationStatusListener is not made with webview. Is it because it is protected by base::android::IsVMInitialized()? Sounds like working by accident then. I am worried that simplecache works suboptimally for webview, but I guess there is no way to tell because we do not have UMA for webview?
,
May 18 2017
> I did not know that instantiation of the ApplicationStatusListener is not made with webview. Is it because it is protected by base::android::IsVMInitialized()? No. ApplicationStatusListener just makes no sense for webview. It's assuming the main chrome activity (and the views associated with that activity) lives roughly as long as the application. That assumption is totally not true, and is not in webview's control at all. But in this particular case, it's crashing because application is trying to access cookies before creating a webview. Due to the CookieManager api being thread safe (and some other complicated reasons), at the point when cookie things are initialized, we don't really know which thread is the UI thread, hence the assert. > I am worried that simplecache works suboptimally for webview, but I guess there is no way to tell because we do not have UMA for webview? We are starting to get UMA now-ish, from a very small population.
,
May 18 2017
Yeah; WebView has *some* information about the visibility/foregroundness/lifetime of the parts of the app it's associated with, but it's not the same information that Chrome has, it's a different level of information in different cases (depending how the app uses webview) and it doesn't map directly to ApplicationStatus. This is why I want lower-level components to expose interfaces that express what actual thing they will do (e.g. "flush data to disk") rather than observe status directly and make their own policy decisions - because it's possible that in some cases WebView may well be able to determine "okay, now is a good time to flush data to disk", and if there's an API that does that, we can maybe call it sometimes - but we can't implement ApplicationStatus, because there's other times it won't make sense. If simplecache is relying on ApplicationStatus signals then yes, it's quite possible it works suboptimally for WebView. I'm not sure how it's used by simplecache, but generally what you would want to do to make sure you work correctly for WebView is to be pessimistic by default, and only use information from ApplicationStatus (or more generally from the kind of more-specific higher-level interface I'm proposing using instead) to *relax* behaviour.
,
May 19 2017
Another thing.. why wasn't this caught by an instrumentation test (which runs on cq)? I can't quite tell if we actually have an instrumentation test for the cookie manager start up path.
,
May 19 2017
I don't think we do. It has to run in a fresh process that hasn't loaded the code, so the setup requirements for a proper test are awkward :/
,
May 19 2017
should be possible, each instrumentation test is an entirely new process, and we just need to make sure test and activity doesn't create AwContents and whatnot before the test even runs
,
May 19 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by estevenson@chromium.org
, May 17 2017Owner: nhar...@chromium.org