Investigate high volume of VIEW intents not dispatched to custom tabs |
||||
Issue descriptionThis is a follow-up to issue 780619 to investigate why number of VIEW intents not dispatched from onNewIntent() is so high. The UMA to look at is Android.MainActivity.ExplicitMainViewIntentDispatched.OnNewIntent, and for 63.0.3239.60 we have the following: Not dispatched 22,658 22.58% Dispatched 77,699 77.42% Splitting by OS we get: Android_Lollipop: Not dispatched 6,001 44.63% Dispatched 7,446 55.37% Android_Marshmallow: Not dispatched 6,222 28.78% Dispatched 15,395 71.22% Android_N: Not dispatched 9,719 20.99% Dispatched 36,585 79.01% On KitKat and below that code is not active (.Main points to ChromeLauncherActivity, not ChromeTabbedActivity), so numbers are zero.
,
Nov 29 2017
,
Nov 29 2017
Analysis of how different branches in LaunchIntentDispatcher.dispatch() handle VIEW intents:
private @Action int dispatch() {
if (url == null && tabId == Tab.INVALID_TAB_ID && !incognito
&& intentHandler.handleWebSearchIntent(mIntent)) {
return Action.FINISH_ACTIVITY;
}
=> Only handles Intent.ACTION_SEARCH and MediaStore.INTENT_ACTION_MEDIA_SEARCH
actions
if (WebappLauncherActivity.bringWebappToFront(tabId)) {
return Action.FINISH_ACTIVITY_REMOVE_TASK;
}
=> Can handle VIEW intents, since it only cares about BRING_TAB_TO_FRONT extra
if (mIntent.hasCategory(Notification.INTENT_CATEGORY_NOTIFICATION_PREFERENCES)) {
NotificationPlatformBridge.launchNotificationPreferences(mActivity, mIntent);
return Action.FINISH_ACTIVITY;
}
=> Android calls activity that has this in the manifest, for Chrome it's
.IntentDispatcher alias to ChromeLauncherActivity
if (InstantAppsHandler.getInstance().handleIncomingIntent(
mActivity, mIntent, mIsCustomTabIntent && !mIsHerbIntent, false)) {
return Action.FINISH_ACTIVITY;
}
=> Can handle VIEW intents since if seems to care only about extras
if (FirstRunFlowSequencer.launch(mActivity, mIntent, false /* requiresBroadcast */,
false /* preferLightweightFre */)) {
return Action.FINISH_ACTIVITY;
}
=> Irrelevant, since CTA is already running
if (!mIsCustomTabIntent && !FeatureUtilities.isDocumentMode(mActivity)) {
return dispatchToTabbedActivity();
}
=> Dispatches to CTA itself (i.e. ends up calling onNewIntent() anyway)
if (mIsCustomTabIntent) {
if (!mIntent.getBooleanExtra(
TrustedWebUtils.EXTRA_LAUNCH_AS_TRUSTED_WEB_ACTIVITY, false)
|| !launchTrustedWebActivity()) {
launchCustomTabActivity();
}
return Action.FINISH_ACTIVITY;
}
=> CTA.onNewIntent() handles this case already
if (DocumentModeAssassin.getInstance().isMigrationNecessary()) {
Log.d(TAG, "Diverting to UpgradeActivity via " + mActivity.getClass().getName());
UpgradeActivity.launchInstance(mActivity, mIntent);
return Action.FINISH_ACTIVITY_REMOVE_TASK;
}
=> Irrelevant
So we have two cases that are not handled by CTA.maybeDispatchExplicitMainViewIntent():
1. WebappLauncherActivity.bringWebappToFront()
2. InstantAppsHandler.handleIncomingIntent()
,
Dec 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b90f227f35b89c8057138ab6d203aa5e27092cdf commit b90f227f35b89c8057138ab6d203aa5e27092cdf Author: Dmitry Skiba <dskiba@chromium.org> Date: Sun Dec 03 03:28:24 2017 Categorize undispatched VIEW intents sent to .Main alias. Bug: 789732 Change-Id: I660cad683bcc96772ddb5fc25f1a94ae8a988bb6 Reviewed-on: https://chromium-review.googlesource.com/803617 Commit-Queue: Dmitry Skiba <dskiba@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Cr-Commit-Position: refs/heads/master@{#521224} [modify] https://crrev.com/b90f227f35b89c8057138ab6d203aa5e27092cdf/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/b90f227f35b89c8057138ab6d203aa5e27092cdf/chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java [modify] https://crrev.com/b90f227f35b89c8057138ab6d203aa5e27092cdf/tools/metrics/histograms/enums.xml [modify] https://crrev.com/b90f227f35b89c8057138ab6d203aa5e27092cdf/tools/metrics/histograms/histograms.xml
,
Dec 6 2017
And the first results are in: Other 10 01.51% Chrome 654 98.49% I suspect something in Chrome copies original CTA intent and then changes action to VIEW (among other things).
,
Dec 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/67f6261f18e00169fd532e45ab682bf531376a30 commit 67f6261f18e00169fd532e45ab682bf531376a30 Author: Dmitry Skiba <dskiba@chromium.org> Date: Wed Dec 13 20:50:11 2017 Crash debug builds on undispatched VIEW/.Main intents. See the bug - we're somehow getting nontrivial number of non-CCT VIEW intents sent directly to .Main activity alias. We reported intent sources to a histogram and found that majority of the intents are from Chrome itself. Static analysis didn't find places where we construct a VIEW intent with .Main activity alias and EXTRA_APPLICATION_ID set to Chrome. This CL adds code to crash debug builds when we detect non-CCT VIEW intent sent to .Main activity alias. There is also a command line flag to disable crashing. This is a temporary change that will be reverted once we find the cause. Bug: 789732 Change-Id: Ic0ed3067e5cccbc48f913a02acf9b54925e3b93c Reviewed-on: https://chromium-review.googlesource.com/817625 Commit-Queue: Dmitry Skiba <dskiba@chromium.org> Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org> Cr-Commit-Position: refs/heads/master@{#523868} [modify] https://crrev.com/67f6261f18e00169fd532e45ab682bf531376a30/chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java [modify] https://crrev.com/67f6261f18e00169fd532e45ab682bf531376a30/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
,
Jan 19 2018
Running into this locally: java.lang.IllegalStateException: VIEW intent sent to .Main activity alias was not dispatched. PLEASE report the following info to crbug.com/789732: "Intent { act=android.intent.action.VIEW dat=chrome-native://bookmarks/folder/3 flg=0x10000000 pkg=com.google.android.apps.chrome cmp=com.google.android.apps.chrome/.Main (has extras) }, extras.keySet = [trusted_application_code_extra, com.android.browser.application_id, com.google.chrome.transition_type, org.chromium.chrome.browser.timestamp]". Use --dont-crash-on-view-main-intents flag to disable this check. Build: ToT Android Build Number: 7.1.2 Device: Nexus 9 This was using Monochrome.apk with is_debug = true.
,
Jan 19 2018
Woohoo, thanks for reporting! I'll take a look.
,
Jan 19 2018
Turned out we have places where we construct VIEW intents with component name of the current activity. One of such places is responsible for the intent in #7: ... [7] org.chromium.chrome.browser.IntentHandler.startActivityForTrustedIntentInternal (IntentHandler.java:196) [8] org.chromium.chrome.browser.IntentHandler.startActivityForTrustedIntent (IntentHandler.java:185) [9] org.chromium.chrome.browser.bookmarks.BookmarkUtils.openUrl (BookmarkUtils.java:52) [10] org.chromium.chrome.browser.bookmarks.BookmarkUtils.showBookmarkManager (BookmarkUtils.java:16) [11] org.chromium.chrome.browser.ChromeTabbedActivity$$Lambda$7.run (unknown) [12] org.chromium.chrome.browser.compositor.CompositorViewHolder.hideKeyboard (CompositorViewHolder.java:315) [13] org.chromium.chrome.browser.ChromeTabbedActivity.onMenuOrKeyboardAction (ChromeTabbedActivity.java:716) [14] org.chromium.chrome.browser.ChromeActivity.onOptionsItemSelected (ChromeActivity.java:770) [15] org.chromium.chrome.browser.appmenu.AppMenu.onItemClick (AppMenu.java:169) [16] org.chromium.chrome.browser.appmenu.AppMenuAdapter$$Lambda$3.onClick (unknown) [17] android.view.View.performClick (View.java:6,294) [18] android.view.View$PerformClick.run (View.java:24,770) [19] android.os.Handler.handleCallback (Handler.java:790) [20] android.os.Handler.dispatchMessage (Handler.java:99) showBookmarkManager() calls openUrl() with activity.getComponentName(), and for CTA started via .Main alias it's com.google.android.apps.chrome/.Main.
,
Jan 19 2018
Current Android.MainActivity.UndispatchedExplicitMainViewIntentSource data: Other 430 04.45% Chrome 9,212 95.43% Gsa 11 00.11%
,
Apr 26 2018
I'm hitting this on current ToT clicking items from the History activity.
,
May 30 2018
,
May 30 2018
The history activity crash from #11 is from here: [1] org.chromium.chrome.browser.IntentHandler.startActivityForTrustedIntentInternal (IntentHandler.java:672) [2] org.chromium.chrome.browser.IntentHandler.startActivityForTrustedIntent (IntentHandler.java:651) [3] org.chromium.chrome.browser.history.HistoryManager.openUrl (HistoryManager.java:281) [4] org.chromium.chrome.browser.history.HistoryItem.open (HistoryItem.java:103) [5] org.chromium.chrome.browser.history.HistoryItemView.onClick (HistoryItemView.java:149) [6] org.chromium.chrome.browser.widget.selection.SelectableItemView.onClick (SelectableItemView.java:146) [7] android.view.View.performClick (View.java:5,637) [8] android.view.View$PerformClick.run (View.java:22,429) [9] android.os.Handler.handleCallback (Handler.java:751) [10] android.os.Handler.dispatchMessage (Handler.java:95) [11] android.os.Looper.loop (Looper.java:154) [12] android.app.ActivityThread.main (ActivityThread.java:6,121) [13] java.lang.reflect.Method.invoke (native method) [14] com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run (ZygoteInit.java:889) [15] com.android.internal.os.ZygoteInit.main (ZygoteInit.java:779) HistoryManager.openUrl() calls HistoryManager.getOpenUrlIntent(), which creates VIEW intent with mActivity.getComponentName().
,
May 30 2018
Ted, so here are two places which end up sending VIEW intents to activity.getComponentName(), which can be .Main activity. I feel we should fix these two cases to use ChromeLauncherActivity instead? Or just remove the check and let them be?
,
Jun 1 2018
If we want the nuclear option, could we just override startActivity on AsyncInitializationActivity, ChromeApplication, and SynchronousInitializationActivity to change the component if we see it as .Main? If it is .Main, I think changing to ChromeLauncherActivity is correct, but we also wouldn't want to change it always if it were ChromeTabbedActivity2 for example.
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e484d3ec0ec66c07b8f18a3e5f8e2d1c19822d39 commit e484d3ec0ec66c07b8f18a3e5f8e2d1c19822d39 Author: Ted Choc <tedchoc@google.com> Date: Fri Jun 15 20:52:17 2018 Do not dispatch intents from within Chrome to the .Main alias. The .Main alias is meant for the launcher activity triggering only, so we want to ensure we don't pollute that with intents from within Chrome. The two sources found thus far are bookmarks and history. BUG=789732 Change-Id: I90dc10019a2d07076439f80f3f280d4635f2e02e Reviewed-on: https://chromium-review.googlesource.com/1102573 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Ted Choc <tedchoc@chromium.org> Cr-Commit-Position: refs/heads/master@{#567781} [modify] https://crrev.com/e484d3ec0ec66c07b8f18a3e5f8e2d1c19822d39/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/e484d3ec0ec66c07b8f18a3e5f8e2d1c19822d39/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java [modify] https://crrev.com/e484d3ec0ec66c07b8f18a3e5f8e2d1c19822d39/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
,
Jun 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/62d16d51eb1c7e0dcc460c65e9432c2b3a3c9111 commit 62d16d51eb1c7e0dcc460c65e9432c2b3a3c9111 Author: Ted Choc <tedchoc@google.com> Date: Thu Jun 28 00:12:29 2018 Prevent offline pages/downloads from sending intents with the .Main alias. Downloaded an offline page, clicked the infobar, and it crahsed using this code path. BUG=789732 Change-Id: I55e18d8b3ab5abe17743f25fe0b090140cb0530f Reviewed-on: https://chromium-review.googlesource.com/1117213 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Ted Choc <tedchoc@chromium.org> Cr-Commit-Position: refs/heads/master@{#570959} [modify] https://crrev.com/62d16d51eb1c7e0dcc460c65e9432c2b3a3c9111/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java [modify] https://crrev.com/62d16d51eb1c7e0dcc460c65e9432c2b3a3c9111/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java [modify] https://crrev.com/62d16d51eb1c7e0dcc460c65e9432c2b3a3c9111/chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java [modify] https://crrev.com/62d16d51eb1c7e0dcc460c65e9432c2b3a3c9111/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java |
||||
►
Sign in to add a comment |
||||
Comment 1 by dskiba@chromium.org
, Nov 29 2017