From a68cace7d150660f98991e2e4340be4eacc22a3c Mon Sep 17 00:00:00 2001 From: Kurt Partridge Date: Sun, 21 Apr 2013 16:41:03 -0700 Subject: [PATCH] Avoid NPE by fixing ResearchLogger initialization Previously, mMainResearchLog and mMainLogBuffer were set up when the user moved to a new TextView, and set to null when the user left the TextView. This change causes mMainResearchLog, mMainLogBuffer, mFeedbackLog, and mFeedbackLogBuffer to be non-null forever after init() is called. start() no longer sets up these fields; instead they are cleared and reset every time stop() is called. Checks for null values are now removed. The earlier code just didn't initialize these variables if the user disabled logging, but since the new version invariantly keeps these variables valid, we add a check for whether the user has enabled logging in publishLogUnits(). Change-Id: Ifde3517f1cf924cfa33cda95fec24529b52b3c08 --- .../inputmethod/research/ResearchLogger.java | 155 +++++++----------- 1 file changed, 62 insertions(+), 93 deletions(-) diff --git a/java/src/com/android/inputmethod/research/ResearchLogger.java b/java/src/com/android/inputmethod/research/ResearchLogger.java index 1f6845c8b..b45134edc 100644 --- a/java/src/com/android/inputmethod/research/ResearchLogger.java +++ b/java/src/com/android/inputmethod/research/ResearchLogger.java @@ -150,18 +150,18 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang private static final ResearchLogger sInstance = new ResearchLogger(); private static String sAccountType = null; private static String sAllowedAccountDomain = null; - /* package */ ResearchLog mMainResearchLog; + private ResearchLog mMainResearchLog; // always non-null after init() is called // mFeedbackLog records all events for the session, private or not (excepting // passwords). It is written to permanent storage only if the user explicitly commands // the system to do so. // LogUnits are queued in the LogBuffers and published to the ResearchLogs when words are // complete. - /* package */ MainLogBuffer mMainLogBuffer; + /* package for test */ MainLogBuffer mMainLogBuffer; // always non-null after init() is called // TODO: Remove the feedback log. The feedback log continuously captured user data in case the // user wanted to submit it. We now use the mUserRecordingLogBuffer to allow the user to // explicitly reproduce a problem. - /* package */ ResearchLog mFeedbackLog; - /* package */ LogBuffer mFeedbackLogBuffer; + private ResearchLog mFeedbackLog; + private LogBuffer mFeedbackLogBuffer; /* package */ ResearchLog mUserRecordingLog; /* package */ LogBuffer mUserRecordingLogBuffer; private File mUserRecordingFile = null; @@ -241,6 +241,9 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang mResearchLogDirectory = new ResearchLogDirectory(mLatinIME); cleanLogDirectoryIfNeeded(mResearchLogDirectory, System.currentTimeMillis()); + // Initialize log buffers + resetLogBuffers(); + // Initialize external services mUploadIntent = new Intent(mLatinIME, UploaderService.class); mUploadNowIntent = new Intent(mLatinIME, UploaderService.class); @@ -252,6 +255,39 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang mReplayer.setKeyboardSwitcher(keyboardSwitcher); } + private void resetLogBuffers() { + mMainResearchLog = new ResearchLog(mResearchLogDirectory.getLogFilePath( + System.currentTimeMillis(), System.nanoTime()), mLatinIME); + final int numWordsToIgnore = new Random().nextInt(NUMBER_OF_WORDS_BETWEEN_SAMPLES + 1); + mMainLogBuffer = new MainLogBuffer(NUMBER_OF_WORDS_BETWEEN_SAMPLES, numWordsToIgnore, + mSuggest) { + @Override + protected void publish(final ArrayList logUnits, + boolean canIncludePrivateData) { + canIncludePrivateData |= IS_LOGGING_EVERYTHING; + for (final LogUnit logUnit : logUnits) { + if (DEBUG) { + final String wordsString = logUnit.getWordsAsString(); + Log.d(TAG, "onPublish: '" + wordsString + + "', hc: " + logUnit.containsCorrection() + + ", cipd: " + canIncludePrivateData); + } + for (final String word : logUnit.getWordsAsStringArray()) { + final Dictionary dictionary = getDictionary(); + mStatistics.recordWordEntered( + dictionary != null && dictionary.isValidWord(word), + logUnit.containsCorrection()); + } + } + publishLogUnits(logUnits, mMainResearchLog, canIncludePrivateData); + } + }; + + mFeedbackLog = new ResearchLog(mResearchLogDirectory.getLogFilePath( + System.currentTimeMillis(), System.nanoTime()), mLatinIME); + mFeedbackLogBuffer = new FixedLogBuffer(FEEDBACK_WORD_BUFFER_SIZE); + } + private void cleanLogDirectoryIfNeeded(final ResearchLogDirectory researchLogDirectory, final long now) { final long lastCleanupTime = ResearchSettings.readResearchLastDirCleanupTime(mPrefs); @@ -380,49 +416,6 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang requestIndicatorRedraw(); mStatistics.reset(); checkForEmptyEditor(); - if (mFeedbackLogBuffer == null) { - resetFeedbackLogging(); - } - if (!isAllowedToLog()) { - // Log.w(TAG, "not in usability mode; not logging"); - return; - } - if (mMainLogBuffer == null) { - mMainResearchLog = new ResearchLog(mResearchLogDirectory.getLogFilePath( - System.currentTimeMillis(), System.nanoTime()), mLatinIME); - final int numWordsToIgnore = new Random().nextInt(NUMBER_OF_WORDS_BETWEEN_SAMPLES + 1); - mMainLogBuffer = new MainLogBuffer(NUMBER_OF_WORDS_BETWEEN_SAMPLES, numWordsToIgnore, - mSuggest) { - @Override - protected void publish(final ArrayList logUnits, - boolean canIncludePrivateData) { - canIncludePrivateData |= IS_LOGGING_EVERYTHING; - for (final LogUnit logUnit : logUnits) { - if (DEBUG) { - final String wordsString = logUnit.getWordsAsString(); - Log.d(TAG, "onPublish: '" + wordsString - + "', hc: " + logUnit.containsCorrection() - + ", cipd: " + canIncludePrivateData); - } - for (final String word : logUnit.getWordsAsStringArray()) { - final Dictionary dictionary = getDictionary(); - mStatistics.recordWordEntered( - dictionary != null && dictionary.isValidWord(word), - logUnit.containsCorrection()); - } - } - if (mMainResearchLog != null) { - publishLogUnits(logUnits, mMainResearchLog, canIncludePrivateData); - } - } - }; - } - } - - private void resetFeedbackLogging() { - mFeedbackLog = new ResearchLog(mResearchLogDirectory.getLogFilePath( - System.currentTimeMillis(), System.nanoTime()), mLatinIME); - mFeedbackLogBuffer = new FixedLogBuffer(FEEDBACK_WORD_BUFFER_SIZE); } /* package */ void stop() { @@ -432,35 +425,27 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang // Commit mCurrentLogUnit before closing. commitCurrentLogUnit(); - if (mMainLogBuffer != null) { - mMainLogBuffer.shiftAndPublishAll(); - logStatistics(); - commitCurrentLogUnit(); - mMainLogBuffer.setIsStopping(); - mMainLogBuffer.shiftAndPublishAll(); - mMainResearchLog.blockingClose(RESEARCHLOG_CLOSE_TIMEOUT_IN_MS); - mMainLogBuffer = null; - } - if (mFeedbackLogBuffer != null) { - mFeedbackLog.blockingClose(RESEARCHLOG_CLOSE_TIMEOUT_IN_MS); - mFeedbackLogBuffer = null; - } + mMainLogBuffer.shiftAndPublishAll(); + logStatistics(); + commitCurrentLogUnit(); + mMainLogBuffer.setIsStopping(); + mMainLogBuffer.shiftAndPublishAll(); + mMainResearchLog.blockingClose(RESEARCHLOG_CLOSE_TIMEOUT_IN_MS); + mFeedbackLog.blockingClose(RESEARCHLOG_CLOSE_TIMEOUT_IN_MS); + + resetLogBuffers(); } public void abort() { if (DEBUG) { Log.d(TAG, "abort called"); } - if (mMainLogBuffer != null) { - mMainLogBuffer.clear(); - mMainResearchLog.blockingAbort(RESEARCHLOG_ABORT_TIMEOUT_IN_MS); - mMainLogBuffer = null; - } - if (mFeedbackLogBuffer != null) { - mFeedbackLogBuffer.clear(); - mFeedbackLog.blockingAbort(RESEARCHLOG_ABORT_TIMEOUT_IN_MS); - mFeedbackLogBuffer = null; - } + mMainLogBuffer.clear(); + mMainResearchLog.blockingAbort(RESEARCHLOG_ABORT_TIMEOUT_IN_MS); + mFeedbackLogBuffer.clear(); + mFeedbackLog.blockingAbort(RESEARCHLOG_ABORT_TIMEOUT_IN_MS); + + resetLogBuffers(); } private void restart() { @@ -745,8 +730,8 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang public void initSuggest(final Suggest suggest) { mSuggest = suggest; - // MainLogBuffer has out-of-date Suggest object. Need to close it down and create a new - // one. + // MainLogBuffer now has an out-of-date Suggest object. Close down MainLogBuffer and create + // a new one. if (mMainLogBuffer != null) { stop(); start(); @@ -857,9 +842,7 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang ": " + mCurrentLogUnit.getWordsAsString() : "")); } if (!mCurrentLogUnit.isEmpty()) { - if (mMainLogBuffer != null) { - mMainLogBuffer.shiftIn(mCurrentLogUnit); - } + mMainLogBuffer.shiftIn(mCurrentLogUnit); if (mFeedbackLogBuffer != null) { mFeedbackLogBuffer.shiftIn(mCurrentLogUnit); } @@ -887,9 +870,6 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang // // Note that we don't use mLastLogUnit here, because it only goes one word back and is only // needed for reverts, which only happen one back. - if (mMainLogBuffer == null) { - return; - } final LogUnit oldLogUnit = mMainLogBuffer.peekLastLogUnit(); // Check that expected word matches. @@ -943,6 +923,7 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang final ResearchLog researchLog, final boolean canIncludePrivateData) { final LogUnit openingLogUnit = new LogUnit(); if (logUnits.isEmpty()) return; + if (!isAllowedToLog()) return; // LogUnits not containing private data, such as contextual data for the log, do not require // logSegment boundary statements. if (canIncludePrivateData) { @@ -1376,11 +1357,7 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang public static void latinIME_promotePhantomSpace() { final ResearchLogger researchLogger = getInstance(); final LogUnit logUnit; - if (researchLogger.mMainLogBuffer == null) { - logUnit = researchLogger.mCurrentLogUnit; - } else { - logUnit = researchLogger.mMainLogBuffer.peekLastLogUnit(); - } + logUnit = researchLogger.mMainLogBuffer.peekLastLogUnit(); researchLogger.enqueueEvent(logUnit, LOGSTATEMENT_LATINIME_PROMOTEPHANTOMSPACE); } @@ -1397,11 +1374,7 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang final String charactersAfterSwap) { final ResearchLogger researchLogger = getInstance(); final LogUnit logUnit; - if (researchLogger.mMainLogBuffer == null) { - logUnit = null; - } else { - logUnit = researchLogger.mMainLogBuffer.peekLastLogUnit(); - } + logUnit = researchLogger.mMainLogBuffer.peekLastLogUnit(); if (logUnit != null) { researchLogger.enqueueEvent(logUnit, LOGSTATEMENT_LATINIME_SWAPSWAPPERANDSPACE, originalCharacters, charactersAfterSwap); @@ -1474,11 +1447,7 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang final ResearchLogger researchLogger = getInstance(); // TODO: Verify that mCurrentLogUnit has been restored and contains the reverted word. final LogUnit logUnit; - if (researchLogger.mMainLogBuffer == null) { - logUnit = null; - } else { - logUnit = researchLogger.mMainLogBuffer.peekLastLogUnit(); - } + logUnit = researchLogger.mMainLogBuffer.peekLastLogUnit(); if (originallyTypedWord.length() > 0 && hasLetters(originallyTypedWord)) { if (logUnit != null) { logUnit.setWords(originallyTypedWord);