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
diff --git a/java/src/com/android/inputmethod/research/ResearchLogger.java b/java/src/com/android/inputmethod/research/ResearchLogger.java
index 1f6845c..b45134e 100644
--- a/java/src/com/android/inputmethod/research/ResearchLogger.java
+++ b/java/src/com/android/inputmethod/research/ResearchLogger.java
@@ -150,18 +150,18 @@
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 @@
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 @@
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<LogUnit> 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 @@
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<LogUnit> 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 @@
// 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 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 @@
": " + mCurrentLogUnit.getWordsAsString() : ""));
}
if (!mCurrentLogUnit.isEmpty()) {
- if (mMainLogBuffer != null) {
- mMainLogBuffer.shiftIn(mCurrentLogUnit);
- }
+ mMainLogBuffer.shiftIn(mCurrentLogUnit);
if (mFeedbackLogBuffer != null) {
mFeedbackLogBuffer.shiftIn(mCurrentLogUnit);
}
@@ -887,9 +870,6 @@
//
// 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 @@
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 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 @@
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 @@
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);