From ef63947f7d10bcbb3237661ad480b430d4208833 Mon Sep 17 00:00:00 2001 From: Keisuke Kuroyanagi Date: Fri, 27 Sep 2013 22:28:54 +0900 Subject: [PATCH 1/3] Add boundary checking for PtNode Array reading. DO NOT MERGE cheery-pick of Iea5e19d98d2fc26f137046dd5f8e339239672351 Bug: 10957075 Change-Id: I3f1806c32fe480f2e7422d3a945932b9cc0cd98b --- .../dynamic_patricia_trie_node_reader.cpp | 3 ++- .../dynamic_patricia_trie_reading_helper.cpp | 18 ++++++++++++++++++ .../dynamic_patricia_trie_reading_helper.h | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_node_reader.cpp b/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_node_reader.cpp index 456352c17..2fa3111d3 100644 --- a/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_node_reader.cpp +++ b/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_node_reader.cpp @@ -26,7 +26,8 @@ namespace latinime { void DynamicPatriciaTrieNodeReader::fetchPtNodeInfoFromBufferAndProcessMovedPtNode( const int ptNodePos, const int maxCodePointCount, int *const outCodePoints) { if (ptNodePos < 0 || ptNodePos >= mBuffer->getTailPosition()) { - AKLOGE("Fetching PtNode info form invalid dictionary position: %d, dictionary size: %d", + // Reading invalid position because of bug or broken dictionary. + AKLOGE("Fetching PtNode info from invalid dictionary position: %d, dictionary size: %d", ptNodePos, mBuffer->getTailPosition()); ASSERT(false); invalidatePtNodeInfo(); diff --git a/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.cpp b/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.cpp index f4a2ef389..601ee663b 100644 --- a/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.cpp +++ b/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.cpp @@ -155,6 +155,15 @@ bool DynamicPatriciaTrieReadingHelper::traverseAllPtNodesInPtNodeArrayLevelPreor // Read node array size and process empty node arrays. Nodes and arrays are counted up in this // method to avoid an infinite loop. void DynamicPatriciaTrieReadingHelper::nextPtNodeArray() { + if (mReadingState.mPos < 0 || mReadingState.mPos >= mBuffer->getTailPosition()) { + // Reading invalid position because of a bug or a broken dictionary. + AKLOGE("Reading PtNode array info from invalid dictionary position: %d, dict size: %d", + mReadingState.mPos, mBuffer->getTailPosition()); + ASSERT(false); + mIsError = true; + mReadingState.mPos = NOT_A_DICT_POS; + return; + } mReadingState.mPosOfLastPtNodeArrayHead = mReadingState.mPos; const bool usesAdditionalBuffer = mBuffer->isInAdditionalBuffer(mReadingState.mPos); const uint8_t *const dictBuf = mBuffer->getBuffer(usesAdditionalBuffer); @@ -191,6 +200,15 @@ void DynamicPatriciaTrieReadingHelper::nextPtNodeArray() { // Follow the forward link and read the next node array if exists. void DynamicPatriciaTrieReadingHelper::followForwardLink() { + if (mReadingState.mPos < 0 || mReadingState.mPos >= mBuffer->getTailPosition()) { + // Reading invalid position because of bug or broken dictionary. + AKLOGE("Reading forward link from invalid dictionary position: %d, dict size: %d", + mReadingState.mPos, mBuffer->getTailPosition()); + ASSERT(false); + mIsError = true; + mReadingState.mPos = NOT_A_DICT_POS; + return; + } const bool usesAdditionalBuffer = mBuffer->isInAdditionalBuffer(mReadingState.mPos); const uint8_t *const dictBuf = mBuffer->getBuffer(usesAdditionalBuffer); if (usesAdditionalBuffer) { diff --git a/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.h b/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.h index b033eee05..154590fbd 100644 --- a/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.h +++ b/native/jni/src/suggest/policyimpl/dictionary/dynamic_patricia_trie_reading_helper.h @@ -240,6 +240,7 @@ class DynamicPatriciaTrieReadingHelper { static const int MAX_NODE_ARRAY_COUNT_TO_AVOID_INFINITE_LOOP; static const size_t MAX_READING_STATE_STACK_SIZE; + // TODO: Introduce error code to track what caused the error. bool mIsError; ReadingState mReadingState; const BufferWithExtendableBuffer *const mBuffer; From ceb364c0411dbcb38f2bd94f1d2b54eef3b985c1 Mon Sep 17 00:00:00 2001 From: Yuichiro Hanada Date: Fri, 27 Sep 2013 11:13:22 +0900 Subject: [PATCH 2/3] Fix PrioritizedSerialExecutor. It was possible that fetchNextTasks() would be called by multiple threads concurrently. If it happens, some tasks in the task queues might be ignored. cherrypick of Idc81c43c45e382da3850cc55b9a42c281548d2a8 bug: 10957075 Change-Id: I41bf90dda15306ee879350f96a791d940ea31da1 --- .../latin/ExpandableBinaryDictionary.java | 2 +- ...ynamicPersonalizationDictionaryWriter.java | 8 +-- .../utils/PrioritizedSerialExecutor.java | 70 ++++++++++--------- .../UserHistoryDictionaryTests.java | 13 ++-- 4 files changed, 46 insertions(+), 47 deletions(-) diff --git a/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java b/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java index 183f12ad9..712520212 100644 --- a/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java +++ b/java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java @@ -727,7 +727,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary { holder.set(mBinaryDictionary.isValidWord(word)); } else { holder.set(((DynamicPersonalizationDictionaryWriter) mDictionaryWriter) - .isInDictionaryForTests(word)); + .isInBigramListForTests(word)); } } } diff --git a/java/src/com/android/inputmethod/latin/personalization/DynamicPersonalizationDictionaryWriter.java b/java/src/com/android/inputmethod/latin/personalization/DynamicPersonalizationDictionaryWriter.java index 0af028a9e..305088536 100644 --- a/java/src/com/android/inputmethod/latin/personalization/DynamicPersonalizationDictionaryWriter.java +++ b/java/src/com/android/inputmethod/latin/personalization/DynamicPersonalizationDictionaryWriter.java @@ -79,7 +79,7 @@ public class DynamicPersonalizationDictionaryWriter extends AbstractDictionaryWr public void addUnigramWord(final String word, final String shortcutTarget, final int frequency, final boolean isNotAWord) { if (mBigramList.size() > mMaxHistoryBigrams * 2) { - // Too many entries: just stop adding new vocabrary and wait next refresh. + // Too many entries: just stop adding new vocabulary and wait next refresh. return; } mExpandableDictionary.addWord(word, shortcutTarget, frequency); @@ -90,7 +90,7 @@ public class DynamicPersonalizationDictionaryWriter extends AbstractDictionaryWr public void addBigramWords(final String word0, final String word1, final int frequency, final boolean isValid, final long lastModifiedTime) { if (mBigramList.size() > mMaxHistoryBigrams * 2) { - // Too many entries: just stop adding new vocabrary and wait next refresh. + // Too many entries: just stop adding new vocabulary and wait next refresh. return; } if (lastModifiedTime > 0) { @@ -176,8 +176,8 @@ public class DynamicPersonalizationDictionaryWriter extends AbstractDictionaryWr } @UsedForTesting - public boolean isInDictionaryForTests(final String word) { + public boolean isInBigramListForTests(final String word) { // TODO: Use native method to determine whether the word is in dictionary or not - return mBigramList.containsKey(word); + return mBigramList.containsKey(word) || mBigramList.getBigrams(null).containsKey(word); } } diff --git a/java/src/com/android/inputmethod/latin/utils/PrioritizedSerialExecutor.java b/java/src/com/android/inputmethod/latin/utils/PrioritizedSerialExecutor.java index 5dc0b5893..201a70d42 100644 --- a/java/src/com/android/inputmethod/latin/utils/PrioritizedSerialExecutor.java +++ b/java/src/com/android/inputmethod/latin/utils/PrioritizedSerialExecutor.java @@ -16,8 +16,11 @@ package com.android.inputmethod.latin.utils; -import java.util.ArrayDeque; import java.util.Queue; +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; /** * An object that executes submitted tasks using a thread. @@ -27,19 +30,20 @@ public class PrioritizedSerialExecutor { private final Object mLock = new Object(); - // The default value of capacities of task queues. - private static final int TASK_QUEUE_CAPACITY = 1000; private final Queue mTasks; private final Queue mPrioritizedTasks; private boolean mIsShutdown; + private final ThreadPoolExecutor mThreadPoolExecutor; // The task which is running now. private Runnable mActive; public PrioritizedSerialExecutor() { - mTasks = new ArrayDeque(TASK_QUEUE_CAPACITY); - mPrioritizedTasks = new ArrayDeque(TASK_QUEUE_CAPACITY); + mTasks = new ConcurrentLinkedQueue(); + mPrioritizedTasks = new ConcurrentLinkedQueue(); mIsShutdown = false; + mThreadPoolExecutor = new ThreadPoolExecutor(1 /* corePoolSize */, 1 /* maximumPoolSize */, + 0 /* keepAliveTime */, TimeUnit.MILLISECONDS, new ArrayBlockingQueue(1)); } /** @@ -59,7 +63,16 @@ public class PrioritizedSerialExecutor { public void execute(final Runnable r) { synchronized(mLock) { if (!mIsShutdown) { - mTasks.offer(r); + mTasks.offer(new Runnable() { + @Override + public void run() { + try { + r.run(); + } finally { + scheduleNext(); + } + } + }); if (mActive == null) { scheduleNext(); } @@ -74,45 +87,36 @@ public class PrioritizedSerialExecutor { public void executePrioritized(final Runnable r) { synchronized(mLock) { if (!mIsShutdown) { - mPrioritizedTasks.offer(r); - if (mActive == null) { + mPrioritizedTasks.offer(new Runnable() { + @Override + public void run() { + try { + r.run(); + } finally { + scheduleNext(); + } + } + }); + if (mActive == null) { scheduleNext(); } } } } - private boolean fetchNextTasks() { - synchronized(mLock) { - mActive = mPrioritizedTasks.poll(); - if (mActive == null) { - mActive = mTasks.poll(); - } - return mActive != null; + private boolean fetchNextTasksLocked() { + mActive = mPrioritizedTasks.poll(); + if (mActive == null) { + mActive = mTasks.poll(); } + return mActive != null; } private void scheduleNext() { synchronized(mLock) { - if (!fetchNextTasks()) { - return; + if (fetchNextTasksLocked()) { + mThreadPoolExecutor.execute(mActive); } - new Thread(new Runnable() { - @Override - public void run() { - try { - do { - synchronized(mLock) { - if (mActive != null) { - mActive.run(); - } - } - } while (fetchNextTasks()); - } finally { - scheduleNext(); - } - } - }).start(); } } diff --git a/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java b/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java index d605cdb84..06c427193 100644 --- a/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java +++ b/tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java @@ -84,29 +84,24 @@ public class UserHistoryDictionaryTests extends AndroidTestCase { } /** - * @param checksContents if true, checks whether written words are actually in the dictionary + * @param checkContents if true, checks whether written words are actually in the dictionary * or not. */ private void addAndWriteRandomWords(final String testFilenameSuffix, final int numberOfWords, - final Random random, final boolean checksContents) { + final Random random, final boolean checkContents) { final List words = generateWords(numberOfWords, random); final UserHistoryPredictionDictionary dict = PersonalizationHelper.getUserHistoryPredictionDictionary(getContext(), testFilenameSuffix /* locale */, mPrefs); // Add random words to the user history dictionary. addToDict(dict, words); - if (checksContents) { + if (checkContents) { try { Thread.sleep(TimeUnit.MILLISECONDS.convert(5L, TimeUnit.SECONDS)); } catch (InterruptedException e) { } - // Limit word count to check when using a Java on memory dictionary. - final int wordCountToCheck = - ExpandableBinaryDictionary.ENABLE_BINARY_DICTIONARY_DYNAMIC_UPDATE ? - numberOfWords : 10; - for (int i = 0; i < wordCountToCheck; ++i) { + for (int i = 0; i < numberOfWords; ++i) { final String word = words.get(i); - // This may fail as long as we use tryLock on inserting the bigram words assertTrue(dict.isInDictionaryForTests(word)); } } From e628cd3c325d97243d4ef1c991f07c084d0cbaac Mon Sep 17 00:00:00 2001 From: Keisuke Kuroyanagi Date: Mon, 30 Sep 2013 18:16:29 +0900 Subject: [PATCH 3/3] Use reentrant lock for main dictionaries. DO NOT MERGE cherrypick of Iaa9b79fc770d8ae2ec9d7c362c90c28bc9f65ea8 Bug: 10964805 Change-Id: Id5e67b00bf9594be0591c87407a78146297e0e78 --- .../inputmethod/latin/DictionaryFactory.java | 31 +++-- .../latin/ReadOnlyBinaryDictionary.java | 120 ++++++++++++++++++ 2 files changed, 135 insertions(+), 16 deletions(-) create mode 100644 java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java diff --git a/java/src/com/android/inputmethod/latin/DictionaryFactory.java b/java/src/com/android/inputmethod/latin/DictionaryFactory.java index 3721132c5..828e54f14 100644 --- a/java/src/com/android/inputmethod/latin/DictionaryFactory.java +++ b/java/src/com/android/inputmethod/latin/DictionaryFactory.java @@ -51,7 +51,7 @@ public final class DictionaryFactory { if (null == locale) { Log.e(TAG, "No locale defined for dictionary"); return new DictionaryCollection(Dictionary.TYPE_MAIN, - createBinaryDictionary(context, locale)); + createReadOnlyBinaryDictionary(context, locale)); } final LinkedList dictList = CollectionUtils.newLinkedList(); @@ -59,11 +59,11 @@ public final class DictionaryFactory { BinaryDictionaryGetter.getDictionaryFiles(locale, context); if (null != assetFileList) { for (final AssetFileAddress f : assetFileList) { - final BinaryDictionary binaryDictionary = new BinaryDictionary(f.mFilename, - f.mOffset, f.mLength, useFullEditDistance, locale, Dictionary.TYPE_MAIN, - false /* isUpdatable */); - if (binaryDictionary.isValidDictionary()) { - dictList.add(binaryDictionary); + final ReadOnlyBinaryDictionary readOnlyBinaryDictionary = + new ReadOnlyBinaryDictionary(f.mFilename, f.mOffset, f.mLength, + useFullEditDistance, locale, Dictionary.TYPE_MAIN); + if (readOnlyBinaryDictionary.isValidDictionary()) { + dictList.add(readOnlyBinaryDictionary); } } } @@ -89,12 +89,12 @@ public final class DictionaryFactory { } /** - * Initializes a dictionary from a raw resource file + * Initializes a read-only binary dictionary from a raw resource file * @param context application context for reading resources * @param locale the locale to use for the resource - * @return an initialized instance of BinaryDictionary + * @return an initialized instance of ReadOnlyBinaryDictionary */ - protected static BinaryDictionary createBinaryDictionary(final Context context, + protected static ReadOnlyBinaryDictionary createReadOnlyBinaryDictionary(final Context context, final Locale locale) { AssetFileDescriptor afd = null; try { @@ -113,9 +113,8 @@ public final class DictionaryFactory { Log.e(TAG, "sourceDir is not a file: " + sourceDir); return null; } - return new BinaryDictionary(sourceDir, afd.getStartOffset(), afd.getLength(), - false /* useFullEditDistance */, locale, Dictionary.TYPE_MAIN, - false /* isUpdatable */); + return new ReadOnlyBinaryDictionary(sourceDir, afd.getStartOffset(), afd.getLength(), + false /* useFullEditDistance */, locale, Dictionary.TYPE_MAIN); } catch (android.content.res.Resources.NotFoundException e) { Log.e(TAG, "Could not find the resource"); return null; @@ -142,10 +141,10 @@ public final class DictionaryFactory { final DictionaryCollection dictionaryCollection = new DictionaryCollection(Dictionary.TYPE_MAIN); for (final AssetFileAddress address : dictionaryList) { - final BinaryDictionary binaryDictionary = new BinaryDictionary(address.mFilename, - address.mOffset, address.mLength, useFullEditDistance, locale, - Dictionary.TYPE_MAIN, false /* isUpdatable */); - dictionaryCollection.addDictionary(binaryDictionary); + final ReadOnlyBinaryDictionary readOnlyBinaryDictionary = new ReadOnlyBinaryDictionary( + address.mFilename, address.mOffset, address.mLength, useFullEditDistance, + locale, Dictionary.TYPE_MAIN); + dictionaryCollection.addDictionary(readOnlyBinaryDictionary); } return dictionaryCollection; } diff --git a/java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java b/java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java new file mode 100644 index 000000000..68505ce38 --- /dev/null +++ b/java/src/com/android/inputmethod/latin/ReadOnlyBinaryDictionary.java @@ -0,0 +1,120 @@ +/* + * Copyright (C) 2013 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.inputmethod.latin; + +import com.android.inputmethod.keyboard.ProximityInfo; +import com.android.inputmethod.latin.SuggestedWords.SuggestedWordInfo; + +import java.util.ArrayList; +import java.util.Locale; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +/** + * This class provides binary dictionary reading operations with locking. An instance of this class + * can be used by multiple threads. Note that different session IDs must be used when multiple + * threads get suggestions using this class. + */ +public final class ReadOnlyBinaryDictionary extends Dictionary { + /** + * A lock for accessing binary dictionary. Only closing binary dictionary is the operation + * that change the state of dictionary. + */ + private final ReentrantReadWriteLock mLock = new ReentrantReadWriteLock(); + + private final BinaryDictionary mBinaryDictionary; + + public ReadOnlyBinaryDictionary(final String filename, final long offset, final long length, + final boolean useFullEditDistance, final Locale locale, final String dictType) { + super(dictType); + mBinaryDictionary = new BinaryDictionary(filename, offset, length, useFullEditDistance, + locale, dictType, false /* isUpdatable */); + } + + public boolean isValidDictionary() { + return mBinaryDictionary.isValidDictionary(); + } + + @Override + public ArrayList getSuggestions(final WordComposer composer, + final String prevWord, final ProximityInfo proximityInfo, + final boolean blockOffensiveWords, final int[] additionalFeaturesOptions) { + return getSuggestionsWithSessionId(composer, prevWord, proximityInfo, blockOffensiveWords, + additionalFeaturesOptions, 0 /* sessionId */); + } + + @Override + public ArrayList getSuggestionsWithSessionId(final WordComposer composer, + final String prevWord, final ProximityInfo proximityInfo, + final boolean blockOffensiveWords, final int[] additionalFeaturesOptions, + final int sessionId) { + if (mLock.readLock().tryLock()) { + try { + return mBinaryDictionary.getSuggestions(composer, prevWord, proximityInfo, + blockOffensiveWords, additionalFeaturesOptions); + } finally { + mLock.readLock().unlock(); + } + } + return null; + } + + @Override + public boolean isValidWord(final String word) { + if (mLock.readLock().tryLock()) { + try { + return mBinaryDictionary.isValidWord(word); + } finally { + mLock.readLock().unlock(); + } + } + return false; + } + + @Override + public boolean shouldAutoCommit(final SuggestedWordInfo candidate) { + if (mLock.readLock().tryLock()) { + try { + return mBinaryDictionary.shouldAutoCommit(candidate); + } finally { + mLock.readLock().unlock(); + } + } + return false; + } + + @Override + public int getFrequency(final String word) { + if (mLock.readLock().tryLock()) { + try { + return mBinaryDictionary.getFrequency(word); + } finally { + mLock.readLock().unlock(); + } + } + return NOT_A_PROBABILITY; + } + + @Override + public void close() { + mLock.writeLock().lock(); + try { + mBinaryDictionary.close(); + } finally { + mLock.writeLock().unlock(); + } + } +}