Commit b3f92034 authored by Bo Liu's avatar Bo Liu Committed by Commit Bot

weblayer: Support setRetainInstance

Need to ensure that when switching between Activities, nothing holds
onto the old Activity (ie Context) or Views from the old Activity.
Otherwise it could cause the old Activity to leak. This required fixes
in content as well.

Bug: 1033926
Change-Id: I731cd25b35171a572956d0846eabd259c19ae554
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1980476Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: default avatarShimi Zhang <ctzsm@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728805}
parent 34e57118
......@@ -312,6 +312,8 @@ class WebContentsAccessibilityAndroid::Connector
WebContentsAccessibilityAndroid* accessibility);
~Connector() override;
void DeleteEarly();
// RenderWidgetHostConnector:
void UpdateRenderProcessConnection(
RenderWidgetHostViewAndroid* old_rwhva,
......@@ -337,6 +339,10 @@ WebContentsAccessibilityAndroid::Connector::~Connector() {
manager->set_web_contents_accessibility(nullptr);
}
void WebContentsAccessibilityAndroid::Connector::DeleteEarly() {
RenderWidgetHostConnector::DestroyEarly();
}
void WebContentsAccessibilityAndroid::Connector::UpdateRenderProcessConnection(
RenderWidgetHostViewAndroid* old_rwhva,
RenderWidgetHostViewAndroid* new_rwhva) {
......@@ -371,6 +377,10 @@ WebContentsAccessibilityAndroid::~WebContentsAccessibilityAndroid() {
Java_WebContentsAccessibilityImpl_onNativeObjectDestroyed(env, obj);
}
void WebContentsAccessibilityAndroid::DeleteEarly(JNIEnv* env) {
connector_->DeleteEarly();
}
jboolean WebContentsAccessibilityAndroid::IsEnabled(
JNIEnv* env,
const JavaParamRef<jobject>& obj) {
......
......@@ -42,6 +42,8 @@ class CONTENT_EXPORT WebContentsAccessibilityAndroid
// Methods called from Java via JNI
// --------------------------------------------------------------------------
void DeleteEarly(JNIEnv* env);
// Global methods.
jboolean IsEnabled(JNIEnv* env,
const base::android::JavaParamRef<jobject>& obj);
......
......@@ -35,11 +35,14 @@ class RenderWidgetHostConnector::Observer
void RenderWidgetHostViewDestroyed(
RenderWidgetHostViewAndroid* rwhva) override;
void DestroyEarly();
void UpdateRenderWidgetHostView(RenderWidgetHostViewAndroid* new_rwhva);
RenderWidgetHostViewAndroid* GetRenderWidgetHostViewAndroid() const;
RenderWidgetHostViewAndroid* active_rwhva() const { return active_rwhva_; }
private:
void DoDestroy(WebContentsAndroid* web_contents_android);
RenderWidgetHostConnector* const connector_;
// Active RenderWidgetHostView connected to this instance. Can also point to
......@@ -94,6 +97,16 @@ void RenderWidgetHostConnector::Observer::DidDetachInterstitialPage() {
void RenderWidgetHostConnector::Observer::WebContentsAndroidDestroyed(
WebContentsAndroid* web_contents_android) {
DoDestroy(web_contents_android);
}
void RenderWidgetHostConnector::Observer::DestroyEarly() {
DoDestroy(
static_cast<WebContentsImpl*>(web_contents())->GetWebContentsAndroid());
}
void RenderWidgetHostConnector::Observer::DoDestroy(
WebContentsAndroid* web_contents_android) {
web_contents_android->RemoveDestructionObserver(this);
DCHECK_EQ(active_rwhva_, GetRenderWidgetHostViewAndroid());
UpdateRenderWidgetHostView(nullptr);
......@@ -153,6 +166,10 @@ RenderWidgetHostViewAndroid* RenderWidgetHostConnector::GetRWHVAForTesting()
return render_widget_observer_->active_rwhva();
}
void RenderWidgetHostConnector::DestroyEarly() {
render_widget_observer_->DestroyEarly();
}
WebContents* RenderWidgetHostConnector::web_contents() const {
return render_widget_observer_->web_contents();
}
......
......@@ -5,6 +5,7 @@
#ifndef CONTENT_BROWSER_ANDROID_RENDER_WIDGET_HOST_CONNECTOR_H_
#define CONTENT_BROWSER_ANDROID_RENDER_WIDGET_HOST_CONNECTOR_H_
#include "base/gtest_prod_util.h"
#include "base/memory/weak_ptr.h"
#include "content/browser/renderer_host/render_widget_host_view_android.h"
#include "content/public/browser/web_contents_observer.h"
......@@ -16,7 +17,9 @@ namespace content {
// override |UpdateRenderProcessConnection| to set itself to the RWHVA
// brought up foreground, and null out its reference in the RWHVA going
// away so it won't access the object any more.
// This class owns itself and gets deleted when the Java WebContents is deleted.
// This class owns itself and gets deleted when the WebContents is deleted.
// Can also delete this object early (before the WebContents is destroyed)
// by calling Destroy directly.
class RenderWidgetHostConnector {
public:
explicit RenderWidgetHostConnector(WebContents* web_contents);
......@@ -39,8 +42,14 @@ class RenderWidgetHostConnector {
RenderWidgetHostViewAndroid* GetRWHVAForTesting() const;
protected:
FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostConnectorTest, DestroyEarly);
WebContents* web_contents() const;
// Deletes this now. Note this is usually not required as this is
// deleted when the corresponding WebContents is destroyed.
void DestroyEarly();
private:
class Observer;
std::unique_ptr<Observer> render_widget_observer_;
......
......@@ -119,4 +119,16 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostConnectorTest,
EXPECT_EQ(nullptr, connector->GetRWHVAForTesting());
}
IN_PROC_BROWSER_TEST_F(RenderWidgetHostConnectorTest, DestroyEarly) {
EXPECT_TRUE(
NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html")));
RenderWidgetHostViewAndroid* main_rwhva = render_widget_host_view_android();
RenderWidgetHostConnector* connector = render_widget_host_connector();
EXPECT_EQ(main_rwhva, connector->GetRWHVAForTesting());
connector->DestroyEarly();
EXPECT_EQ(nullptr, render_widget_host_connector());
}
} // namespace content
......@@ -43,6 +43,7 @@ import org.chromium.content_public.browser.AccessibilitySnapshotCallback;
import org.chromium.content_public.browser.AccessibilitySnapshotNode;
import org.chromium.content_public.browser.WebContents;
import org.chromium.content_public.browser.WebContentsAccessibility;
import org.chromium.ui.base.WindowAndroid;
import java.util.ArrayList;
import java.util.List;
......@@ -215,6 +216,17 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
mCaptioningController.startListening();
}
@Override
public void onWindowAndroidChanged(WindowAndroid windowAndroid) {
// Delete this object when switching between WindowAndroids/Activities.
WindowEventObserverManager.from(mWebContents).removeObserver(this);
mWebContents.removeUserData(WebContentsAccessibilityImpl.class);
if (mNativeObj != 0) {
WebContentsAccessibilityImplJni.get().deleteEarly(mNativeObj);
assert mNativeObj == 0;
}
}
/**
* Refresh a11y state with that of {@link AccessibilityManager}.
*/
......@@ -1533,6 +1545,7 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
@NativeMethods
interface Natives {
long init(WebContentsAccessibilityImpl caller, WebContents webContents);
void deleteEarly(long nativeWebContentsAccessibilityAndroid);
void onAutofillPopupDisplayed(
long nativeWebContentsAccessibilityAndroid, WebContentsAccessibilityImpl caller);
void onAutofillPopupDismissed(
......
......@@ -17,6 +17,7 @@ import android.content.pm.ResolveInfo;
import android.content.res.Resources;
import android.graphics.Rect;
import android.os.Build;
import android.os.Handler;
import android.provider.Browser;
import android.text.Spanned;
import android.text.TextUtils;
......@@ -110,6 +111,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
SelectionPopupControllerImpl::new;
}
private final Handler mHandler;
private Context mContext;
private WindowAndroid mWindowAndroid;
private WebContentsImpl mWebContents;
......@@ -128,6 +130,8 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
// required because ActionMode only exposes a temporary hide routine.
private Runnable mRepeatingHideRunnable;
// Can be null temporarily when switching between WindowAndroid.
@Nullable
private View mView;
private ActionMode mActionMode;
......@@ -236,6 +240,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
private SelectionPopupControllerImpl(
WebContents webContents, PopupController popupController, boolean initializeNative) {
mHandler = new Handler();
mWebContents = (WebContentsImpl) webContents;
mPopupController = popupController;
mContext = mWebContents.getContext();
......@@ -254,7 +259,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
assert mHidden;
final long hideDuration = getDefaultHideDuration();
// Ensure the next hide call occurs before the ActionMode reappears.
mView.postDelayed(mRepeatingHideRunnable, hideDuration - 1);
mHandler.postDelayed(mRepeatingHideRunnable, hideDuration - 1);
hideActionModeTemporarily(hideDuration);
}
};
......@@ -287,14 +292,12 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
@Override
public void onUpdateContainerView(ViewGroup view) {
assert view != null;
// Cleans up action mode before switching to a new container view.
if (isActionModeValid()) finishActionMode();
mUnselectAllOnDismiss = true;
destroyPastePopup();
view.setClickable(true);
if (view != null) view.setClickable(true);
mView = view;
initHandleObserver();
}
......@@ -411,7 +414,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
* <p> If the action mode cannot be created the selection is cleared.
*/
public void showActionModeOrClearOnFailure() {
if (!isActionModeSupported() || !hasSelection()) return;
if (!isActionModeSupported() || !hasSelection() || mView == null) return;
// Just refresh non-floating action mode if it already exists to avoid blinking.
if (isActionModeValid() && !isFloatingActionMode()) {
......@@ -442,6 +445,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
}
private ActionMode startFloatingActionMode() {
assert mView != null;
assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.M;
ActionMode actionMode = ContentApiHelperForM.startActionMode(mView, this, mCallback);
return actionMode;
......@@ -460,7 +464,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
}
private void createAndShowPastePopup() {
if (mView.getParent() == null || mView.getVisibility() != View.VISIBLE) {
if (mView == null || mView.getParent() == null || mView.getVisibility() != View.VISIBLE) {
return;
}
......@@ -595,6 +599,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
@Override
public void onWindowAndroidChanged(WindowAndroid newWindowAndroid) {
mWindowAndroid = newWindowAndroid;
mContext = mWebContents.getContext();
initHandleObserver();
destroyPastePopup();
}
......@@ -653,7 +658,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
if (mHidden) {
mRepeatingHideRunnable.run();
} else {
mView.removeCallbacks(mRepeatingHideRunnable);
mHandler.removeCallbacks(mRepeatingHideRunnable);
// To show the action mode that is being hidden call hide() again with a short delay.
hideActionModeTemporarily(SHOW_DELAY_MS);
}
......@@ -880,6 +885,8 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
@Override
public boolean onActionItemClicked(ActionMode mode, MenuItem item) {
// Actions should only happen when there is a WindowAndroid so mView should not be null.
assert mView != null;
if (!isActionModeValid()) return true;
int id = item.getItemId();
......@@ -1001,6 +1008,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
*/
@VisibleForTesting
void doAssistAction() {
assert mView != null;
if (mClassificationResult == null || !mClassificationResult.hasNamedAction()) return;
assert mClassificationResult.onClickListener != null
......@@ -1379,7 +1387,7 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
@VisibleForTesting
/* package */ void performHapticFeedback() {
if (BuildInfo.isAtLeastQ()) {
if (BuildInfo.isAtLeastQ() && mView != null) {
mView.performHapticFeedback(HapticFeedbackConstants.TEXT_HANDLE_MOVE);
}
}
......
......@@ -922,6 +922,12 @@ public class WebContentsImpl implements WebContents, RenderFrameHostDelegate, Wi
return key.cast(data);
}
public <T extends UserData> void removeUserData(Class<T> key) {
UserDataHost userDataHost = getUserDataHost();
if (userDataHost == null) return;
userDataHost.removeUserData(key);
}
/**
* @return {@code UserDataHost} that contains internal user data. {@code null} if
* it is already gc'ed.
......
......@@ -12,6 +12,7 @@ import android.content.Intent;
import android.os.Bundle;
import android.support.test.InstrumentationRegistry;
import android.support.test.rule.ActivityTestRule;
import android.support.v4.app.Fragment;
import android.text.TextUtils;
import org.json.JSONException;
......@@ -251,4 +252,12 @@ public class InstrumentationActivityTestRule extends ActivityTestRule<Instrument
public String getTestDataURL(String path) {
return getTestServer().getURL("/weblayer/test/data/" + path);
}
public void setRetainInstance(boolean retain) {
TestThreadUtils.runOnUiThreadBlocking(() -> getActivity().setRetainInstance(retain));
}
public Fragment getFragment() {
return TestThreadUtils.runOnUiThreadBlockingNoException(() -> getActivity().getFragment());
}
}
......@@ -5,6 +5,7 @@
package org.chromium.weblayer.test;
import android.support.test.filters.SmallTest;
import android.support.v4.app.Fragment;
import org.junit.Assert;
import org.junit.Rule;
......@@ -86,4 +87,38 @@ public class SmokeTest {
}
});
}
@Test
@SmallTest
public void testSetRetainInstance() {
ReferenceQueue<InstrumentationActivity> referenceQueue = new ReferenceQueue<>();
PhantomReference<InstrumentationActivity> reference;
{
InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl("about:blank");
mActivityTestRule.setRetainInstance(true);
Fragment firstFragment = mActivityTestRule.getFragment();
mActivityTestRule.recreateActivity();
Fragment secondFragment = mActivityTestRule.getFragment();
Assert.assertEquals(firstFragment, secondFragment);
boolean destroyed =
TestThreadUtils.runOnUiThreadBlockingNoException(() -> activity.isDestroyed());
Assert.assertTrue(destroyed);
reference = new PhantomReference<>(activity, referenceQueue);
}
CriteriaHelper.pollInstrumentationThread(new Criteria() {
@Override
public boolean isSatisfied() {
Reference enqueuedReference = referenceQueue.poll();
if (enqueuedReference == null) {
Runtime.getRuntime().gc();
return false;
}
Assert.assertEquals(reference, enqueuedReference);
return true;
}
});
}
}
......@@ -33,6 +33,7 @@ public class BrowserImpl extends IBrowser.Stub {
private BrowserViewController mViewController;
private FragmentWindowAndroid mWindowAndroid;
private ArrayList<TabImpl> mTabs = new ArrayList<TabImpl>();
private TabImpl mActiveTab;
private IBrowserClient mClient;
private LocaleChangedBroadcastReceiver mLocaleReceiver;
......@@ -46,22 +47,31 @@ public class BrowserImpl extends IBrowser.Stub {
}
public ViewGroup getViewAndroidDelegateContainerView() {
if (mViewController == null) return null;
return mViewController.getContentView();
}
public void onFragmentAttached(Context context, FragmentWindowAndroid windowAndroid) {
assert mWindowAndroid == null;
assert mViewController == null;
mWindowAndroid = windowAndroid;
mViewController = new BrowserViewController(context, windowAndroid);
mLocaleReceiver = new LocaleChangedBroadcastReceiver(context);
if (mTabs.isEmpty()) {
TabImpl tab = new TabImpl(mProfile, windowAndroid);
addTab(tab);
boolean set_active_result = setActiveTab(tab);
assert set_active_result;
mLocaleReceiver = new LocaleChangedBroadcastReceiver(context);
} else {
updateAllTabs();
mViewController.setActiveTab(mActiveTab);
}
}
public void onFragmentDetached() {
destroy(); // For now we don't retain anything between detach and attach.
destroyAttachmentState();
updateAllTabs();
}
public void onActivityResult(int requestCode, int resultCode, Intent data) {
......@@ -144,6 +154,7 @@ public class BrowserImpl extends IBrowser.Stub {
public boolean setActiveTab(ITab controller) {
StrictModeWorkaround.apply();
TabImpl tab = (TabImpl) controller;
mActiveTab = tab;
if (tab != null && tab.getBrowser() != this) return false;
mViewController.setActiveTab(tab);
try {
......@@ -157,7 +168,7 @@ public class BrowserImpl extends IBrowser.Stub {
}
public TabImpl getActiveTab() {
return mViewController.getTab();
return mActiveTab;
}
@Override
......@@ -190,15 +201,21 @@ public class BrowserImpl extends IBrowser.Stub {
}
public void destroy() {
setActiveTab(null);
for (TabImpl tab : mTabs) {
tab.destroy();
}
mTabs.clear();
destroyAttachmentState();
}
private void destroyAttachmentState() {
if (mLocaleReceiver != null) {
mLocaleReceiver.destroy();
mLocaleReceiver = null;
}
if (mViewController != null) {
mViewController.destroy();
for (TabImpl tab : mTabs) {
tab.destroy();
}
mViewController = null;
}
if (mWindowAndroid != null) {
......@@ -206,4 +223,10 @@ public class BrowserImpl extends IBrowser.Stub {
mWindowAndroid = null;
}
}
private void updateAllTabs() {
for (TabImpl tab : mTabs) {
tab.updateFromBrowser();
}
}
}
......@@ -115,12 +115,16 @@ public final class TabImpl extends ITab.Stub {
*/
public void attachToBrowser(BrowserImpl browser) {
mBrowser = browser;
mWebContents.setTopLevelNativeWindow(browser.getWindowAndroid());
mViewAndroidDelegate.setContainerView(browser.getViewAndroidDelegateContainerView());
updateFromBrowser();
SelectionPopupController.fromWebContents(mWebContents)
.setActionModeCallback(new ActionModeCallback(mWebContents));
}
public void updateFromBrowser() {
mWebContents.setTopLevelNativeWindow(mBrowser.getWindowAndroid());
mViewAndroidDelegate.setContainerView(mBrowser.getViewAndroidDelegateContainerView());
}
public BrowserImpl getBrowser() {
return mBrowser;
}
......
......@@ -35,6 +35,7 @@ import java.util.List;
*/
public class InstrumentationActivity extends FragmentActivity {
private static final String TAG = "WLInstrumentation";
private static final String KEY_MAIN_VIEW_ID = "mainViewId";
public static final String EXTRA_PROFILE_NAME = "EXTRA_PROFILE_NAME";
......@@ -43,6 +44,7 @@ public class InstrumentationActivity extends FragmentActivity {
public static final String EXTRA_CREATE_WEBLAYER = "EXTRA_CREATE_WEBLAYER";
private Profile mProfile;
private Fragment mFragment;
private Browser mBrowser;
private Tab mTab;
private EditText mUrlView;
......@@ -51,11 +53,16 @@ public class InstrumentationActivity extends FragmentActivity {
private ViewGroup mTopContentsContainer;
private IntentInterceptor mIntentInterceptor;
private Bundle mSavedInstanceState;
private TabCallback mTabCallback;
public Tab getTab() {
return mTab;
}
public Fragment getFragment() {
return mFragment;
}
public Browser getBrowser() {
return mBrowser;
}
......@@ -88,7 +95,11 @@ public class InstrumentationActivity extends FragmentActivity {
super.onCreate(savedInstanceState);
mSavedInstanceState = savedInstanceState;
LinearLayout mainView = new LinearLayout(this);
if (savedInstanceState == null) {
mMainViewId = View.generateViewId();
} else {
mMainViewId = savedInstanceState.getInt(KEY_MAIN_VIEW_ID);
}
mainView.setId(mMainViewId);
mMainView = mainView;
setContentView(mainView);
......@@ -117,6 +128,23 @@ public class InstrumentationActivity extends FragmentActivity {
}
}
@Override
public void onSaveInstanceState(Bundle outState) {
super.onSaveInstanceState(outState);
// When restoring Fragments, FragmentManager tries to put them in the containers with same
// ids as before.
outState.putInt(KEY_MAIN_VIEW_ID, mMainViewId);
}
@Override
protected void onDestroy() {
super.onDestroy();
if (mTabCallback != null) {
mTab.unregisterTabCallback(mTabCallback);
mTabCallback = null;
}
}
private void createWebLayerAsync() {
try {
WebLayer.loadAsync(getApplicationContext(), webLayer -> onWebLayerReady());
......@@ -138,19 +166,20 @@ public class InstrumentationActivity extends FragmentActivity {
private void onWebLayerReady() {
if (mBrowser != null || isFinishing() || isDestroyed()) return;
Fragment fragment = getOrCreateBrowserFragment();
mBrowser = Browser.fromFragment(fragment);
mFragment = getOrCreateBrowserFragment();
mBrowser = Browser.fromFragment(mFragment);
mProfile = mBrowser.getProfile();
mBrowser.setTopView(mTopContentsContainer);
mTab = mBrowser.getActiveTab();
mTab.registerTabCallback(new TabCallback() {
mTabCallback = new TabCallback() {
@Override
public void onVisibleUriChanged(Uri uri) {
mUrlView.setText(uri.toString());
}
});
};
mTab.registerTabCallback(mTabCallback);
}
private Fragment getOrCreateBrowserFragment() {
......@@ -189,6 +218,10 @@ public class InstrumentationActivity extends FragmentActivity {
mUrlView.clearFocus();
}
public void setRetainInstance(boolean retain) {
mFragment.setRetainInstance(retain);
}
private static String getUrlFromIntent(Intent intent) {
return intent != null ? intent.getDataString() : null;
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment