Bug 1190627 - Part 3 - If an error occurs parsing the regular session store data file on startup, attempt to read the tabs from the backup copy instead. r=sebastian draft
authorJan Henning <jh+bugzilla@buttercookie.de>
Sun, 29 May 2016 15:46:20 +0200
changeset 401233 1eac3223c9c285fa327e084aa5e0ada566c539c9
parent 401232 a3ebc48970c2cd62da4c8dd34600491f58a81cb8
child 401234 dd7f34b4ea5c7fedb8c636ca600f954c1c1104f7
push id26408
push usermozilla@buttercookie.de
push dateTue, 16 Aug 2016 18:08:19 +0000
reviewerssebastian
bugs1190627
milestone51.0a1
Bug 1190627 - Part 3 - If an error occurs parsing the regular session store data file on startup, attempt to read the tabs from the backup copy instead. r=sebastian Since a SessionRestoreException also happens if we deliberately skipped all tabs (because they didn't contain anything interesting apart from about:home) or the session file was valid but didn't contain any tabs (because the user cleared history), we need to ensure that we don't switch to the backup copy in that case. To that extent, we extend the SessionParser's parse function to return whether any JSON exceptions occured while parsing the session file, which would signify a corrupt session file. MozReview-Commit-ID: Kdh5d69irqF
mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
mobile/android/base/java/org/mozilla/gecko/SessionParser.java
mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoProfile.java
--- a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ -1372,40 +1372,49 @@ public abstract class GeckoApp
         }
 
         ThreadUtils.postToBackgroundThread(new Runnable() {
             @Override
             public void run() {
                 // If we are doing a restore, read the session data so we can send it to Gecko later.
                 String restoreMessage = null;
                 if (!mIsRestoringActivity && mShouldRestore) {
+                    final boolean isExternalURL = invokedWithExternalURL(getIntentURI(new SafeIntent(getIntent())));
                     try {
                         // restoreSessionTabs() will create simple tab stubs with the
                         // URL and title for each page, but we also need to restore
                         // session history. restoreSessionTabs() will inject the IDs
                         // of the tab stubs into the JSON data (which holds the session
                         // history). This JSON data is then sent to Gecko so session
                         // history can be restored for each tab.
-                        final SafeIntent intent = new SafeIntent(getIntent());
-                        restoreMessage = restoreSessionTabs(invokedWithExternalURL(getIntentURI(intent)));
+                        restoreMessage = restoreSessionTabs(isExternalURL, false);
                     } catch (SessionRestoreException e) {
-                        // If restore failed, do a normal startup
-                        Log.e(LOGTAG, "An error occurred during restore", e);
-                        // If mShouldRestore was already set to false in restoreSessionTabs(),
-                        // this means that we intentionally skipped all tabs read from the
-                        // session file, so we don't have to report this exception in telemetry
-                        // and can ignore the following bit.
-                        if (mShouldRestore && getProfile().sessionFileExistsAndNotEmptyWindow()) {
-                            // If we got a SessionRestoreException even though the file exists and its
-                            // length doesn't match the known length of an intentionally empty file,
-                            // it's very likely we've encountered a damaged/corrupt session store file.
-                            Log.d(LOGTAG, "Suspecting a damaged session store file.");
-                            Telemetry.addToHistogram("FENNEC_SESSIONSTORE_DAMAGED_SESSION_FILE", 1);
+                        // If mShouldRestore was set to false in restoreSessionTabs(), this means
+                        // either that we intentionally skipped all tabs read from the session file,
+                        // or else that the file was syntactically valid, but didn't contain any
+                        // tabs (e.g. because the user cleared history), therefore we don't need
+                        // to switch to the backup copy.
+                        if (mShouldRestore) {
+                            Log.e(LOGTAG, "An error occurred during restore, switching to backup file", e);
+                            // To be on the safe side, we will always attempt to restore from the backup
+                            // copy if we end up here.
+                            // Since we will also hit this situation regularly during first run though,
+                            // we'll only report it in telemetry if we failed to restore despite the
+                            // file existing, which means it's very probably damaged.
+                            if (getProfile().sessionFileExists()) {
+                                Telemetry.addToHistogram("FENNEC_SESSIONSTORE_DAMAGED_SESSION_FILE", 1);
+                            }
+                            try {
+                                restoreMessage = restoreSessionTabs(isExternalURL, true);
+                            } catch (SessionRestoreException ex) {
+                                // If this fails, too, do a normal startup.
+                                Log.e(LOGTAG, "An error occurred during restore", ex);
+                                mShouldRestore = false;
+                            }
                         }
-                        mShouldRestore = false;
                     }
                 }
 
                 synchronized (GeckoApp.this) {
                     mSessionRestoreParsingFinished = true;
                     GeckoApp.this.notifyAll();
                 }
 
@@ -1774,46 +1783,49 @@ public abstract class GeckoApp
                 } else {
                     openTabsRunnable.run();
                 }
             }
         });
     }
 
     @WorkerThread
-    private String restoreSessionTabs(final boolean isExternalURL) throws SessionRestoreException {
+    private String restoreSessionTabs(final boolean isExternalURL, boolean useBackup) throws SessionRestoreException {
         try {
-            String sessionString = getProfile().readSessionFile(false);
+            String sessionString = getProfile().readSessionFile(useBackup);
             if (sessionString == null) {
                 throw new SessionRestoreException("Could not read from session file");
             }
 
             // If we are doing an OOM restore, parse the session data and
             // stub the restored tabs immediately. This allows the UI to be
             // updated before Gecko has restored.
             if (mShouldRestore) {
                 final JSONArray tabs = new JSONArray();
                 final JSONObject windowObject = new JSONObject();
+                final boolean sessionDataValid;
 
                 LastSessionParser parser = new LastSessionParser(tabs, windowObject, isExternalURL);
 
                 if (mPrivateBrowsingSession == null) {
-                    parser.parse(sessionString);
+                    sessionDataValid = parser.parse(sessionString);
                 } else {
-                    parser.parse(sessionString, mPrivateBrowsingSession);
+                    sessionDataValid = parser.parse(sessionString, mPrivateBrowsingSession);
                 }
 
                 if (tabs.length() > 0) {
                     windowObject.put("tabs", tabs);
                     sessionString = new JSONObject().put("windows", new JSONArray().put(windowObject)).toString();
                 } else {
-                    if (parser.allTabsSkipped()) {
+                    if (parser.allTabsSkipped() || sessionDataValid) {
                         // If we intentionally skipped all tabs we've read from the session file, we
                         // set mShouldRestore back to false at this point already, so the calling code
                         // can infer that the exception wasn't due to a damaged session store file.
+                        // The same applies if the session file was syntactically valid and
+                        // simply didn't contain any tabs.
                         mShouldRestore = false;
                     }
                     throw new SessionRestoreException("No tabs could be read from session file");
                 }
             }
 
             JSONObject restoreData = new JSONObject();
             restoreData.put("sessionString", sessionString);
--- a/mobile/android/base/java/org/mozilla/gecko/SessionParser.java
+++ b/mobile/android/base/java/org/mozilla/gecko/SessionParser.java
@@ -64,17 +64,23 @@ public abstract class SessionParser {
      * Placeholder method that must be overloaded to handle closedTabs while parsing session data.
      *
      * @param closedTabs, JSONArray of recently closed tab entries.
      * @throws JSONException
      */
     public void onClosedTabsRead(final JSONArray closedTabs) throws JSONException {
     }
 
-    public void parse(String... sessionStrings) {
+    /**
+     * Parses the provided session store data and calls onTabRead for each tab that has been found.
+     *
+     * @param sessionStrings One or more strings containing session store data.
+     * @return False if any of the session strings provided didn't contain valid session store data.
+     */
+    public boolean parse(String... sessionStrings) {
         final LinkedList<SessionTab> sessionTabs = new LinkedList<SessionTab>();
         int totalCount = 0;
         int selectedIndex = -1;
         try {
             for (String sessionString : sessionStrings) {
                 final JSONArray windowsArray = new JSONObject(sessionString).getJSONArray("windows");
                 if (windowsArray.length() == 0) {
                     // Session json can be empty if the user has opted out of session restore.
@@ -112,21 +118,23 @@ public abstract class SessionParser {
                         selected = true;
                         selectedIndex = totalCount;
                     }
                     sessionTabs.add(new SessionTab(title, url, selected, tab));
                 }
             }
         } catch (JSONException e) {
             Log.e(LOGTAG, "JSON error", e);
-            return;
+            return false;
         }
 
         // If no selected index was found, select the first tab.
         if (selectedIndex == -1 && sessionTabs.size() > 0) {
             sessionTabs.getFirst().mIsSelected = true;
         }
 
         for (SessionTab tab : sessionTabs) {
             onTabRead(tab);
         }
+
+        return true;
     }
 }
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoProfile.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoProfile.java
@@ -72,17 +72,16 @@ public final class GeckoProfile {
     public static final String CUSTOM_PROFILE = "";
     public static final String GUEST_PROFILE_DIR = "guest";
 
     // Session store
     private static final String SESSION_FILE = "sessionstore.js";
     private static final String SESSION_FILE_BACKUP = "sessionstore.bak";
     private static final String SESSION_FILE_PREVIOUS = "sessionstore.old";
     private static final long MAX_PREVIOUS_FILE_AGE = 1000 * 3600 * 24; // 24 hours
-    private static final int SESSION_STORE_EMPTY_JSON_LENGTH = 14; // length of {"windows":[]}
 
     private boolean mOldSessionDataProcessed = false;
 
     private static final ConcurrentHashMap<String, GeckoProfile> sProfileCache =
             new ConcurrentHashMap<String, GeckoProfile>(
                     /* capacity */ 4, /* load factor */ 0.75f, /* concurrency */ 2);
     private static String sDefaultProfileName;
 
@@ -667,26 +666,22 @@ public final class GeckoProfile {
             }
         } catch (IOException ioe) {
             Log.e(LOGTAG, "Unable to read session file", ioe);
         }
         return null;
     }
 
     /**
-     * Checks whether the session store file exists and that its length
-     * doesn't match the known length of a session store file containing
-     * only an empty window.
+     * Checks whether the session store file exists.
      */
-    public boolean sessionFileExistsAndNotEmptyWindow() {
+    public boolean sessionFileExists() {
         File sessionFile = getFile(SESSION_FILE);
 
-        return sessionFile != null &&
-               sessionFile.exists() &&
-               sessionFile.length() != SESSION_STORE_EMPTY_JSON_LENGTH;
+        return sessionFile != null && sessionFile.exists();
     }
 
     /**
      * Ensures the parent director(y|ies) of the given filename exist by making them
      * if they don't already exist..
      *
      * @param filename The path to the file whose parents should be made directories
      * @return true if the parent directory exists, false otherwise