ANR from TabPersistentStore.mergeState() |
|||||||||||||
Issue descriptionWe have a recurring ANR with the following stacktrace (this trace if from 67.0.3396.87): "main" prio=5 tid=1 Waiting | group="main" sCount=1 dsCount=0 flags=1 obj=0x73004f50 self=0xee7f5000 | sysTid=6948 nice=0 cgrp=default sched=0/0 handle=0xf22e94a8 | state=S schedstat=( 0 0 0 ) utm=71 stm=18 core=2 HZ=100 | stack=0xff0a9000-0xff0ab000 stackSize=8MB | held mutexes= at java.lang.Object.wait(Object.java) - waiting on <0x0802202e> (a java.lang.Object) at java.lang.Thread.parkFor$(Thread.java:2135) - locked <0x0802202e> (a java.lang.Object) at sun.misc.Unsafe.park(Unsafe.java:358) at java.util.concurrent.locks.LockSupport.park(LockSupport.java:190) at java.util.concurrent.FutureTask.awaitDone(FutureTask.java:450) at java.util.concurrent.FutureTask.get(FutureTask.java:192) at android.os.AsyncTask.get(AsyncTask.java:542) at org.chromium.chrome.browser.tabmodel.TabPersistentStore.mergeState(TabPersistentStore.java:447) at org.chromium.chrome.browser.tabmodel.TabModelSelectorImpl.mergeState(TabModelSelectorImpl.java:285) at org.chromium.chrome.browser.ChromeTabbedActivity.maybeMergeTabs(ChromeTabbedActivity.java:2274) at org.chromium.chrome.browser.ChromeTabbedActivity.onResumeWithNative(ChromeTabbedActivity.java:666) at org.chromium.chrome.browser.init.NativeInitializationController.onResume(NativeInitializationController.java:178) at org.chromium.chrome.browser.init.AsyncInitializationActivity.onResume(AsyncInitializationActivity.java:451) at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1456) at android.app.Activity.performResume(Activity.java:7102) at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3785) at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3851) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1738) at android.os.Handler.dispatchMessage(Handler.java:105) at android.os.Looper.loop(Looper.java:164) at android.app.ActivityThread.main(ActivityThread.java:6798) at java.lang.reflect.Method.invoke(Method.java) at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767) I was able to reproduce getting to this point in code consistently with only 1 or 2 tabs. It appears that with more tabs, the tabs aren't finished loading and thus hit this check: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java?q=mergeState&sq=package:chromium&g=0&l=433 I don't think we need to have mergeState run at all on startup (save maybe some case where we ended chrome last time with 2 split windows), since it isn't run with many tabs anyways (and everything still works fine).
,
Jul 25
When I was running this, I was always getting 2 mergeFileNames - "tab_state1" and "tab_state_browser_actions".
,
Jul 27
While creating an AsyncTask and calling get() right away is not a great thing, we want to keep it on the SERIAL_EXECUTOR to prevent race conditions. In the future we hope to move away from this one SERIAL_EXECUTOR and instead have each site using SERIAL_EXECUTOR to instead have its own instance of a serial executor, so that tasks in TabPersistentStore are only blocked by other tasks in TabPersistentStore. However, the bigger issue seems to be that we merge the tabs on every cold start. After doing some testing, it appears that we can get away without doing so, thus eliminating a massive number (probably more than 99%) of these ANRs. I hope to have a code review up shortly.
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9b5fb663f63377fd384363a37906eec962536b2 commit a9b5fb663f63377fd384363a37906eec962536b2 Author: Sam Maier <smaier@chromium.org> Date: Tue Jul 31 21:28:55 2018 Android: Not calling maybeMergeTabs on startup maybeMergeTabs is called on cold starts. It's an expenisve operation, and doesn't appear to do anything except in the case when there is a process restarted, as in https://bugs.chromium.org/p/chromium/issues/detail?id=657418 Testing: followed the Testing Multi-Instance Merging testing doc. https://docs.google.com/document/d/1KDykYo9M63O0zFeDdrV9BUYtomQl4dVnbVgssCyydng Bug: 867596 Change-Id: I1a0a42fe75d2487dfc71382d7fb55bbc0636e5a7 Reviewed-on: https://chromium-review.googlesource.com/1153438 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Sam Maier <smaier@chromium.org> Cr-Commit-Position: refs/heads/master@{#579574} [modify] https://crrev.com/a9b5fb663f63377fd384363a37906eec962536b2/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
,
Aug 3
Thanks for fixing this! smaier@, what do you think about trying to merge the change in #4 to M69? It seems pretty low risk and would help reduce ANRs.
,
Aug 3
Issue 870878 has been merged into this issue.
,
Aug 7
Marking M-69 RBS for Issue 870878.
,
Aug 8
twellington@ - it's pretty low risk, I'd have no issues with it being merged into M69.
,
Aug 8
[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.
,
Aug 8
69 hasn't been promoted to beta yet so I'm supportive.
,
Aug 8
The bug is marked as P3 or Feature. It should not be merged as M69 is in beta. Please contact the approriate milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 8
well played, sheriffbot ;) in truth though beta isn't out yet according to omahaproxy
,
Aug 8
This isn't a P3 based on the RBS label, increasing to reflect the actual priority. It's typically advised to leave bugs open until all merges have been complete so that they don't fall off the eng or TPM radar.
,
Aug 8
This bug requires manual review: M69 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), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 8
Merge approved for M69 branch 3497.
,
Aug 13
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66402cabed51dffda24123681e43b13fd3afb052 commit 66402cabed51dffda24123681e43b13fd3afb052 Author: Sam Maier <smaier@chromium.org> Date: Mon Aug 13 20:15:11 2018 Android: Not calling maybeMergeTabs on startup maybeMergeTabs is called on cold starts. It's an expenisve operation, and doesn't appear to do anything except in the case when there is a process restarted, as in https://bugs.chromium.org/p/chromium/issues/detail?id=657418 Testing: followed the Testing Multi-Instance Merging testing doc. https://docs.google.com/document/d/1KDykYo9M63O0zFeDdrV9BUYtomQl4dVnbVgssCyydng Bug: 867596 Change-Id: I1a0a42fe75d2487dfc71382d7fb55bbc0636e5a7 Reviewed-on: https://chromium-review.googlesource.com/1153438 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Sam Maier <smaier@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#579574}(cherry picked from commit a9b5fb663f63377fd384363a37906eec962536b2) Reviewed-on: https://chromium-review.googlesource.com/1173091 Reviewed-by: Sam Maier <smaier@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#580} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/66402cabed51dffda24123681e43b13fd3afb052/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
,
Aug 15
,
Aug 16
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by smaier@chromium.org
, Jul 25