Android: remove separate reload tracking from Tab.java

Removes the mNeedsReload flag in Tab.java which was just a thin
and partial wrapper around the navigation controller's needs_reload
mechanism. This duplication caused at least one race
condition (crbug.com/828400) and will also complicate future
LifecycleManager work.

All of the checking in Tab.java of mNeedsReload was duplicated by the
navigation controller's needs_reload flag, as used in
NavigationControllerImpl::LoadIfNecessary.

Bug: 829381
Change-Id: Id17c9d9f89088c718ae4a927ea5b8dbe95126e68
Reviewed-on: https://chromium-review.googlesource.com/1006958Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555414}

chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java

@@ -299,13 +299,6 @@ public class Tab
      */
     private boolean mShouldPreserve;
 
-    /**
-     * Indicates if the tab needs to be reloaded upon next display. This is set to true when the tab
-     * crashes while in background, or if a speculative restore in background gets cancelled before
-     * it completes.
-     */
-    private boolean mNeedsReload;
-
     /**
      * True while a page load is in progress.
      */
@@ -737,15 +730,6 @@ public class Tab
         if (getWebContents() != null) getWebContents().getNavigationController().loadIfNecessary();
     }
 
-    /**
-     * Requests the current navigation to be loaded upon the next call to loadIfNecessary().
-     */
-    protected void requestRestoreLoad() {
-        if (getWebContents() != null) {
-            getWebContents().getNavigationController().requestRestoreLoad();
-        }
-    }
-
     /**
      * Causes this tab to navigate to the specified URL.
      * @param params parameters describing the url load. Note that it is important to set correct
@@ -1698,9 +1682,6 @@ public class Tab
     protected void didStartPageLoad(String validatedUrl, boolean showingErrorPage) {
         updateTitle();
         removeSadTabIfPresent();
-        // A page load means that any pending reload is no longer necessary.
-        mNeedsReload = false;
-
         mDataSavedOnStartPageLoad =
                 DataReductionProxySettings.getInstance().getContentLengthSavedInHistorySummary();
 
@@ -2113,8 +2094,6 @@ public class Tab
         }
 
         mControlsOffsetHelper.clearPreviousPositions();
-
-        mNeedsReload = false;
     }
 
     /**
@@ -2268,13 +2247,7 @@ public class Tab
                 // Restore is needed for a tab that is loaded for the first time. WebContents will
                 // be restored from a saved state.
                 unfreezeContents();
-            } else if (mNeedsReload) {
-                // Restore is needed for a tab that was previously loaded, but its renderer was
-                // killed by the oom killer.
-                mNeedsReload = false;
-                requestRestoreLoad();
-            } else {
-                // No restore needed.
+            } else if (!needsReload()) {
                 return;
             }
 
@@ -2399,17 +2372,18 @@ public class Tab
     }
 
     /**
-     * @return Whether the Tab needs to be reloaded. {@see #mNeedsReload}
+     * @return Whether the Tab has requested a reload.
      */
     public boolean needsReload() {
-        return mNeedsReload;
+        return getWebContents() != null && getWebContents().getNavigationController().needsReload();
     }
 
     /**
-     * Set whether the Tab needs to be reloaded. {@see #mNeedsReload}
+     * Set whether the Tab needs to be reloaded.
      */
-    protected void setNeedsReload(boolean needsReload) {
-        mNeedsReload = needsReload;
+    protected void setNeedsReload() {
+        assert getWebContents() != null;
+        getWebContents().getNavigationController().setNeedsReload();
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java

@@ -110,9 +110,7 @@ public class TabWebContentsObserver extends WebContentsObserver {
                 || activityState == ActivityState.STOPPED
                 || activityState == ActivityState.DESTROYED) {
             // The tab crashed in background or was killed by the OS out-of-memory killer.
-            // TODO(crbug.com/829381): why not reuse the native needsReload flag found in
-            // NavigationController?
-            mTab.setNeedsReload(true);
+            mTab.setNeedsReload();
             if (applicationRunning) {
                 rendererCrashStatus = TAB_RENDERER_CRASH_STATUS_HIDDEN_IN_FOREGROUND_APP;
             } else {

chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java

@@ -117,9 +117,13 @@ public class NavigationPopupTest {
         }
 
         @Override
-        public void requestRestoreLoad() {
+        public boolean needsReload() {
+            return false;
         }
 
+        @Override
+        public void setNeedsReload() {}
+
         @Override
         public void reload(boolean checkForRepost) {
         }

content/browser/frame_host/navigation_controller_android.cc

@@ -156,7 +156,13 @@ void NavigationControllerAndroid::ReloadBypassingCache(
   navigation_controller_->Reload(ReloadType::BYPASSING_CACHE, check_for_repost);
 }
 
-void NavigationControllerAndroid::RequestRestoreLoad(
+jboolean NavigationControllerAndroid::NeedsReload(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj) {
+  return navigation_controller_->NeedsReload();
+}
+
+void NavigationControllerAndroid::SetNeedsReload(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
   navigation_controller_->SetNeedsReload();

content/browser/frame_host/navigation_controller_android.h

@@ -55,8 +55,10 @@ class CONTENT_EXPORT NavigationControllerAndroid {
   void ReloadBypassingCache(JNIEnv* env,
                             const base::android::JavaParamRef<jobject>& obj,
                             jboolean check_for_repost);
-  void RequestRestoreLoad(JNIEnv* env,
-                          const base::android::JavaParamRef<jobject>& obj);
+  jboolean NeedsReload(JNIEnv* env,
+                       const base::android::JavaParamRef<jobject>& obj);
+  void SetNeedsReload(JNIEnv* env,
+                      const base::android::JavaParamRef<jobject>& obj);
   void CancelPendingReload(JNIEnv* env,
                            const base::android::JavaParamRef<jobject>& obj);
   void GoToNavigationIndex(JNIEnv* env,

content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java

@@ -101,10 +101,14 @@ import org.chromium.content_public.common.ResourceRequestBody;
     }
 
     @Override
-    public void requestRestoreLoad() {
-        if (mNativeNavigationControllerAndroid != 0) {
-            nativeRequestRestoreLoad(mNativeNavigationControllerAndroid);
-        }
+    public boolean needsReload() {
+        return mNativeNavigationControllerAndroid != 0
+                && nativeNeedsReload(mNativeNavigationControllerAndroid);
+    }
+
+    @Override
+    public void setNeedsReload() {
+        nativeSetNeedsReload(mNativeNavigationControllerAndroid);
     }
 
     @Override
@@ -296,7 +300,8 @@ import org.chromium.content_public.common.ResourceRequestBody;
     private native boolean nativeCanGoForward(long nativeNavigationControllerAndroid);
     private native boolean nativeIsInitialNavigation(long nativeNavigationControllerAndroid);
     private native void nativeLoadIfNecessary(long nativeNavigationControllerAndroid);
-    private native void nativeRequestRestoreLoad(long nativeNavigationControllerAndroid);
+    private native boolean nativeNeedsReload(long nativeNavigationControllerAndroid);
+    private native void nativeSetNeedsReload(long nativeNavigationControllerAndroid);
     private native boolean nativeCanGoToOffset(
             long nativeNavigationControllerAndroid, int offset);
     private native void nativeGoBack(long nativeNavigationControllerAndroid);

content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

@@ -60,10 +60,15 @@ public interface NavigationController {
      */
     public void loadIfNecessary();
 
+    /**
+     * @return Whether a reload has been requested.
+     */
+    public boolean needsReload();
+
     /**
      * Requests the current navigation to be loaded upon the next call to loadIfNecessary().
      */
-    public void requestRestoreLoad();
+    public void setNeedsReload();
 
     /**
      * Reload the current page.