VR: Clicking on most text crashes browser. |
|||||||||||
Issue description
Not sure if this is in M60, or a post-branch issue.
I think the issue is we've disabled selection, so contextual search is firing on an empty string when we click on text in VR.
Sample trace:
signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
[FATAL:jni_string.cc(78)] Check failed: str.
Stack Trace:
RELADDR FUNCTION FILE:LINE
013eccc1 <unknown> /data/app/org.chromium.chrome-1/base.apk
013cb7fb <unknown> /data/app/org.chromium.chrome-1/base.apk
013cb8b5 <unknown> /data/app/org.chromium.chrome-1/base.apk
013cb8d9 <unknown> /data/app/org.chromium.chrome-1/base.apk
065f85f9 <unknown> /data/app/org.chromium.chrome-1/base.apk
065f85b7 <unknown> /data/app/org.chromium.chrome-1/base.apk
-----------------------------------------------------
r0 00000000 r1 000029aa r2 00000006 r3 00000008
r4 eaa7958c r5 00000006 r6 eaa79534 r7 0000010c
r8 ffead4e8 r9 ffead09c sl ffead4e4 fp 0000002d
ip 0000000b sp ffead018 lr e80c05c7 pc e80c2e30
Stack Trace:
RELADDR FUNCTION FILE:LINE
00049e30 tgkill+12 /system/lib/libc.so
000475c3 pthread_kill+34 /system/lib/libc.so
0001d635 raise+10 /system/lib/libc.so
00019181 __libc_android_abort+34 /system/lib/libc.so
00017048 abort+4 /system/lib/libc.so
v------> base::debug::(anonymous namespace)::DebugBreak() src/base/debug/debugger_posix.cc:221
00093fbb base::debug::BreakDebugger() src/base/debug/debugger_posix.cc:251
000a6f37 ~LogMessage src/base/logging.cc:783
000857f9 base::android::ConvertJavaStringToUTF16(_JNIEnv*, _jstring*, std::__ndk1::basic_string<unsigned short, base::string16_char_traits, std::__ndk1::allocator<unsigned short> >*) src/base/android/jni_string.cc:78
000858b3 base::android::ConvertJavaStringToUTF16(_JNIEnv*, _jstring*) src/base/android/jni_string.cc:101
000858d7 base::android::ConvertJavaStringToUTF16(_JNIEnv*, base::android::JavaRef<_jstring*> const&) src/base/android/jni_string.cc:110
007d15f7 TemplateUrlServiceAndroid::GetUrlForContextualSearchQuery(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jstring*> const&, base::android::JavaParamRef<_jstring*> const&, unsigned char, base::android::JavaParamRef<_jstring*> const&) src/chrome/browser/search_engines/template_url_service_android.cc:276
007d15b5 Java_org_chromium_chrome_browser_search_1engines_TemplateUrlService_nativeGetUrlForContextualSearchQuery src/out/Debug/gen/chrome/browser/jni_headers/chrome/jni/TemplateUrlService_jni.h:214
00e13ee1 offset 0xd90000 /data/app/org.chromium.chrome-1/oat/arm/base.odex
,
Jun 7 2017
It happens in M60.
,
Jun 8 2017
Maybe GetUrlForContextualSearchQuery is returning null for some reason on a VR device which is causing the convert to UTF16 to fail. We can probably add a quick check for that and merge into M60.
,
Jun 8 2017
,
Jun 9 2017
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06187f572930bf54ae294079baf9c8dad8a33926 commit 06187f572930bf54ae294079baf9c8dad8a33926 Author: donnd <donnd@chromium.org> Date: Fri Jun 09 23:10:37 2017 [TTS] Robustness for VR: show with null selection. There's a crash on VR that seems to be due to a lack of robustness in the way Contextual Search builds a query with TemplateUrl. This might also happen when tapping around a lot (speculation) so improving robustness is worthwhile. Adds a check to ContextualSearchManager.showContextualSearch that the selection is valid before calling TemplateUrl. Adds a test for this case that will expose this crash (without the robustness change). Also updates the test infrastructure for Contextual Search to not mock out TemplateUrl since that does not seem to be needed anymore. BUG= 728644 Review-Url: https://codereview.chromium.org/2928053002 Cr-Commit-Position: refs/heads/master@{#478446} [modify] https://crrev.com/06187f572930bf54ae294079baf9c8dad8a33926/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java [modify] https://crrev.com/06187f572930bf54ae294079baf9c8dad8a33926/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java
,
Jun 9 2017
The robustness CL in #6 should be safe to merge into M60, so I think we should do that ASAP (It wasn't clear to me why ddorwin@ moved the milestone to M-61).
,
Jun 9 2017
,
Jun 9 2017
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 9 2017
asimjour@ and ddorwin@ I'm going to be OOO so please determine if this needs a merge (twellington@ can help with that if needed).
,
Jun 10 2017
I think that a more robust long-term solution is to explicitly disable Contextual Search when VR mode is enabled similar to how we disable contextual search when accessibility mode is enabled. VR folks, you'll probably also want to test your feature on Android O where smart selection is implemented (timav@ implemented most of that if you have questions) since it uses some of the same native code that contextual search does.
,
Jun 12 2017
My understanding is that WebVR shell is M61 now - if that's the case, is this really needed? I assume that's why ddorwin@ changed to M-61, checking with them would be good though. Rejecting for now given this, but please re-apply label if still necessary.
,
Jun 12 2017
Yes, M61 is correct. No merge is necessary.
,
Jun 13 2017
Thank you donnd@ for the CL. I can verify that it fixes the problem. We don't need to merge it back to M60. I agree with twellington@ that we need to find a better long term solution, I'll work on it and will keep you posted. |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by ddorwin@chromium.org
, Jun 7 2017