Tabs being reparented must stay associated with old activity, or cause crashes |
|
Issue descriptionCurrently navigation, and many other things, require the Tab to be associated with an Activity, so we can't set the WindowAndroid for the Tab to null while re-parenting. See crbug.com/648865 for example of when navigation can be triggered during reparenting. Some locations that expect the Tab to always be associated with an Activity (this list is probably not exhaustive): InterceptNavigationDelegateImpl#shouldIgnoreNavigation InterceptNavigationDelegateImpl#onOverrideUrlLoadingAndLaunchIntent TabContextMenuItemDelegate#onOpenImageInNewTab TabContextMenuItemDelegate#onOpenInChrome TabContextMenuItemDelegate#onOpenInOtherWindow TabWebContentsDelegateAndroid#activateContents TabWebContentsDelegateAndroid#addNewContents TabWebContentsDelegateAndroid#getContentVideoViewEmbedder TabWebContentsDelegateAndroid#handleKeyboardEvent TabWebContentsDelegateAndroid#closeContents TabWebContentsDelegateAndroid#shouldResumeRequestsForCreatedWindow TabWebContentsDelegateAndroid#webContentsCreated Tab#setPendingPrint NewTabPage#destroy (getSnackbarManager()) NewTabPage#showMostVisitedItemRemovedSnackbar (getSnackbarManager()) InterceptNavigationDelegateImpl#onOverrideUrlLoadingAndLaunchIntent (getTabModelSelector()) TabContextMenuItemDelegate#onOpenInNewIncognitoTab (getTabModelSelector()) TabContextMenuItemDelegate#onOpenInNewTab (getTabModelSelector()) TabWebContentsDelegateAndroid#closeContents (getTabModelSelector()) TabWebContentsDelegateAndroid#getTabModel (getTabModelSelector()) Some locations that expect the tab to always be associated with a WindowAndroid: GoogleAuthenticatorNavigationInterceptor#GoogleAuthenticatorNavigationInterceptor ChromeApplication#openClearBrowsingData ContextualSearchTabHelper#getContextualSearchManager ChromeDownloadDelegate#closeBlankTab ChromeDownloadDelegate#requestFileAccess ChromeDownloadDelegate#shouldInterceptContextMenuDownload ExternalNavigationDelegateImpl#closeTab ExternalNavigationDelegateImpl#ExternalNavigationDelegateImpl ExternalNavigationDelegateImpl#shouldRequestFileAccess ExternalNavigationDelegateImpl#startFileIntent ExternalNavigationDelegateImpl#startIncognitoIntent AppBannerInfoBarDelegateAndroid#installOrOpenNativeApp AppBannerInfoBarDelegateAndroid#showAppDetails SimpleConfirmInfoBarBuilder#create LocationBarLayout#onClick AutoSigninSnackbarController#showSnackbar Tab#swapWebContents Tab#renderProcessGone CustomTabToolbar#onNativeLibraryReady FindResultBar#dismiss FindResultBar#FindResultBar
,
Oct 18 2016
It is hard to "not do anything" before we launch the new Activity, mostly because when you launch a new one there are no guarantees as to what will happen to the old one (this is actually the root of some of the troubles), and when an Activity gets destroyed, it by design takes everything it owns down with it :). TabModelSelector, TabModels and Tabs all get cleared out and destroyed with all the associated renderers etc. That is why we remove the tab from the model before launching the intent. We have to severe the renderers connection to the window, because if not it will react to window state changes and when the old activity goes away, will hide the contents, which we don't want, because it looks awful and flickering. So before we launch the intent, we have to do "something". I agree that it is a bit weird to be able to say, we severe all ties, but leave the WindowAndroid and Activity association intact. Nevertheless, note that we actually don't finish that activity until reparenting is completely done in attachAndFinishReparenting, so relying on that old activity is technically OK. But the uncertainties about Activities dying on their own again come back to bite us here. All in all, it would probably be hard to make everything synchronous, but I agree we can make some more of these synchronous.
,
Nov 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/410dd90a44cc3a0a7d805122df31f7e65f377aa7 commit 410dd90a44cc3a0a7d805122df31f7e65f377aa7 Author: mthiesse <mthiesse@chromium.org> Date: Tue Nov 01 18:03:35 2016 Clean up other instances where Tab WindowAndroid was being set to null. BUG=657007 Review-Url: https://codereview.chromium.org/2426083002 Cr-Commit-Position: refs/heads/master@{#429048} [modify] https://crrev.com/410dd90a44cc3a0a7d805122df31f7e65f377aa7/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/410dd90a44cc3a0a7d805122df31f7e65f377aa7/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java [modify] https://crrev.com/410dd90a44cc3a0a7d805122df31f7e65f377aa7/content/browser/android/content_view_core_impl.cc |
|
►
Sign in to add a comment |
|
Comment 1 by mthiesse@chromium.org
, Oct 18 2016