From dd79a6bb8c9aaaa2a41f53979cceccceb3810f78 Mon Sep 17 00:00:00 2001 From: Jan Henning Date: Tue, 5 Jul 2016 22:40:01 +0200 Subject: [PATCH] Bug 1284017 - Add telemetry for damaged session store files. r=sebastian Just watching for a SessionRestoreException during startup can introduce some false positives, because that exception is triggered in any case where we can't restore tabs, not just when the session file has been damaged, e.g.: - on first startup - on builds affected by bug 1228593, users who are (theoretically) restoring their tabs, but clearing their history on exist end up with a deleted sessionstore.js - should we implement bug 1275662, we'd hit that exception in that case, too. Therefore we only send the telemetry event if we hit that exception even though a sessionstore.js file is present. We also exclude the case where the file size of sessionstore.js is 14 bytes, because that is most likely corresponding to a file containing only {"windows":[]}, which means that the session store intentionally wanted to write a file containing no tabs. Currently this is only the case for users who are clearing their history on exit and are also *not* restoring tabs, however if bug 1275662 should get implemented, we'd probably encounter those empty files for users who have their restore setting set to "Always restore", too. Because of bug 1261008, we can also end up with no restored tabs (and a SessionRestoreException) if the session file contains only about:home tabs with no history, because we're skipping those and not restoring them. To detect that case and exclude it from telemetry, we have to include additional logic within the SessionParser instance used during startup and pass those results back to the calling site in GeckoApp. MozReview-Commit-ID: 6pAhDU3d8QA --HG-- extra : rebase_source : ebf4d902a616c17ba10c645ad8ef469ceafe8cce --- .../base/java/org/mozilla/gecko/GeckoApp.java | 159 +++++++++++------- .../java/org/mozilla/gecko/GeckoProfile.java | 14 ++ toolkit/components/telemetry/Histograms.json | 8 + 3 files changed, 121 insertions(+), 60 deletions(-) diff --git a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java b/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java index 379dce6bff3d..33f46105dfb0 100644 --- a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java +++ b/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java @@ -196,6 +196,87 @@ public abstract class GeckoApp protected boolean mShouldRestore; private boolean mSessionRestoreParsingFinished = false; + private static final class LastSessionParser extends SessionParser { + private JSONArray tabs; + private JSONObject windowObject; + private boolean isExternalURL; + + private boolean selectNextTab; + private boolean tabsWereSkipped; + private boolean tabsWereProcessed; + + public LastSessionParser(JSONArray tabs, JSONObject windowObject, boolean isExternalURL) { + this.tabs = tabs; + this.windowObject = windowObject; + this.isExternalURL = isExternalURL; + } + + public boolean allTabsSkipped() { + return tabsWereSkipped && !tabsWereProcessed; + } + + @Override + public void onTabRead(final SessionTab sessionTab) { + if (sessionTab.isAboutHomeWithoutHistory()) { + // This is a tab pointing to about:home with no history. We won't restore + // this tab. If we end up restoring no tabs then the browser will decide + // whether it needs to open about:home or a different 'homepage'. If we'd + // always restore about:home only tabs then we'd never open the homepage. + // See bug 1261008. + + if (sessionTab.isSelected()) { + // Unfortunately this tab is the selected tab. Let's just try to select + // the first tab. If we haven't restored any tabs so far then remember + // to select the next tab that gets restored. + + if (!Tabs.getInstance().selectLastTab()) { + selectNextTab = true; + } + } + + // Do not restore this tab. + tabsWereSkipped = true; + return; + } + + tabsWereProcessed = true; + + JSONObject tabObject = sessionTab.getTabObject(); + + int flags = Tabs.LOADURL_NEW_TAB; + flags |= ((isExternalURL || !sessionTab.isSelected()) ? Tabs.LOADURL_DELAY_LOAD : 0); + flags |= (tabObject.optBoolean("desktopMode") ? Tabs.LOADURL_DESKTOP : 0); + flags |= (tabObject.optBoolean("isPrivate") ? Tabs.LOADURL_PRIVATE : 0); + + final Tab tab = Tabs.getInstance().loadUrl(sessionTab.getUrl(), flags); + + if (selectNextTab) { + // We did not restore the selected tab previously. Now let's select this tab. + Tabs.getInstance().selectTab(tab.getId()); + selectNextTab = false; + } + + ThreadUtils.postToUiThread(new Runnable() { + @Override + public void run() { + tab.updateTitle(sessionTab.getTitle()); + } + }); + + try { + tabObject.put("tabId", tab.getId()); + } catch (JSONException e) { + Log.e(LOGTAG, "JSON error", e); + } + tabs.put(tabObject); + } + + @Override + public void onClosedTabsRead(final JSONArray closedTabData) throws JSONException { + windowObject.put("closedTabs", closedTabData); + } + }; + protected boolean mInitialized; protected boolean mWindowFocusInitialized; private Telemetry.Timer mJavaUiStartupTimer; @@ -1288,6 +1369,17 @@ public abstract class GeckoApp } 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); + } mShouldRestore = false; } } @@ -1680,67 +1772,8 @@ public abstract class GeckoApp if (mShouldRestore) { final JSONArray tabs = new JSONArray(); final JSONObject windowObject = new JSONObject(); - SessionParser parser = new SessionParser() { - private boolean selectNextTab; - @Override - public void onTabRead(final SessionTab sessionTab) { - if (sessionTab.isAboutHomeWithoutHistory()) { - // This is a tab pointing to about:home with no history. We won't restore - // this tab. If we end up restoring no tabs then the browser will decide - // whether it needs to open about:home or a different 'homepage'. If we'd - // always restore about:home only tabs then we'd never open the homepage. - // See bug 1261008. - - if (sessionTab.isSelected()) { - // Unfortunately this tab is the selected tab. Let's just try to select - // the first tab. If we haven't restored any tabs so far then remember - // to select the next tab that gets restored. - - if (!Tabs.getInstance().selectLastTab()) { - selectNextTab = true; - } - } - - // Do not restore this tab. - return; - } - - JSONObject tabObject = sessionTab.getTabObject(); - - int flags = Tabs.LOADURL_NEW_TAB; - flags |= ((isExternalURL || !sessionTab.isSelected()) ? Tabs.LOADURL_DELAY_LOAD : 0); - flags |= (tabObject.optBoolean("desktopMode") ? Tabs.LOADURL_DESKTOP : 0); - flags |= (tabObject.optBoolean("isPrivate") ? Tabs.LOADURL_PRIVATE : 0); - - final Tab tab = Tabs.getInstance().loadUrl(sessionTab.getUrl(), flags); - - if (selectNextTab) { - // We did not restore the selected tab previously. Now let's select this tab. - Tabs.getInstance().selectTab(tab.getId()); - selectNextTab = false; - } - - ThreadUtils.postToUiThread(new Runnable() { - @Override - public void run() { - tab.updateTitle(sessionTab.getTitle()); - } - }); - - try { - tabObject.put("tabId", tab.getId()); - } catch (JSONException e) { - Log.e(LOGTAG, "JSON error", e); - } - tabs.put(tabObject); - } - - @Override - public void onClosedTabsRead(final JSONArray closedTabData) throws JSONException { - windowObject.put("closedTabs", closedTabData); - } - }; + LastSessionParser parser = new LastSessionParser(tabs, windowObject, isExternalURL); if (mPrivateBrowsingSession == null) { parser.parse(sessionString); @@ -1752,6 +1785,12 @@ public abstract class GeckoApp windowObject.put("tabs", tabs); sessionString = new JSONObject().put("windows", new JSONArray().put(windowObject)).toString(); } else { + if (parser.allTabsSkipped()) { + // 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. + mShouldRestore = false; + } throw new SessionRestoreException("No tabs could be read from session file"); } } diff --git a/mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java b/mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java index 5dab4ea7399c..bbc48fbc00c3 100644 --- a/mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java +++ b/mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java @@ -76,6 +76,7 @@ public final class GeckoProfile { private static final String SESSION_FILE = "sessionstore.js"; private static final String SESSION_FILE_BACKUP = "sessionstore.bak"; private static final long MAX_BACKUP_FILE_AGE = 1000 * 3600 * 24; // 24 hours + private static final int SESSION_STORE_EMPTY_JSON_LENGTH = 14; // length of {"windows":[]} private boolean mOldSessionDataProcessed = false; @@ -653,6 +654,19 @@ public final class GeckoProfile { 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. + */ + public boolean sessionFileExistsAndNotEmptyWindow() { + File sessionFile = getFile(SESSION_FILE); + + return sessionFile != null && + sessionFile.exists() && + sessionFile.length() != SESSION_STORE_EMPTY_JSON_LENGTH; + } + /** * Ensures the parent director(y|ies) of the given filename exist by making them * if they don't already exist.. diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index 2f69ae7150b9..b8bf320c1c94 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -10029,5 +10029,13 @@ "bug_numbers": [1276006], "kind": "count", "description": "Tracking the total number of opened Containers." + }, + "FENNEC_SESSIONSTORE_DAMAGED_SESSION_FILE": { + "alert_emails": ["jh+bugzilla@buttercookie.de"], + "expires_in_version": "56", + "kind": "flag", + "bug_numbers": [1284017], + "description": "When restoring tabs on startup, reading from sessionstore.js failed, even though the file exists and is not containing an explicitly empty window.", + "cpp_guard": "ANDROID" } }