diff options
| author | romax <romax@chromium.org> | 2016-03-15 17:59:03 -0700 |
|---|---|---|
| committer | Commit bot <commit-bot@chromium.org> | 2016-03-16 01:00:19 +0000 |
| commit | b5841b612f8c01518fc64b81ab31210d9bdc7abb (patch) | |
| tree | eb1666e7a1fed987a7b2949670d4d3d1ca0d0fbf | |
| parent | d420eecfc4f7881986bfbc623c3598a0b72c06c9 (diff) | |
| download | chromium_src-b5841b612f8c01518fc64b81ab31210d9bdc7abb.zip chromium_src-b5841b612f8c01518fc64b81ab31210d9bdc7abb.tar.gz chromium_src-b5841b612f8c01518fc64b81ab31210d9bdc7abb.tar.bz2 | |
[Offline Pages] Fix crash when deleting offline pages.
R=nyquist@chromium.org, fgorski@chromium.org, bburns@chromium.org
BUG=593182
Adding null check to getClientIdForOfflineId(), and pass client id along with OfflinePageDeleted() callback. Also got some test for the use cases.
nyquist@ for owner review (of the BUILD.gn file).
fgorski@ for overall review.
Review URL: https://codereview.chromium.org/1776343005
Cr-Commit-Position: refs/heads/master@{#381372}
8 files changed, 153 insertions, 23 deletions
diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn index 10e4134..f7297a6 100644 --- a/chrome/android/BUILD.gn +++ b/chrome/android/BUILD.gn @@ -279,6 +279,7 @@ junit_binary("chrome_junit_tests") { "junit/src/org/chromium/chrome/browser/media/router/cast/TestUtils.java", "junit/src/org/chromium/chrome/browser/notifications/NotificationUIManagerUnitTest.java", "junit/src/org/chromium/chrome/browser/ntp/NativePageFactoryTest.java", + "junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java", "junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java", "junit/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java", "junit/src/org/chromium/chrome/browser/omaha/VersionNumberTest.java", diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java index 52c5f6a..b18caa1 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java @@ -473,8 +473,12 @@ public class OfflinePageBridge { }; } - private ClientId getClientIdForOfflineId(long offlineId) { - return nativeGetPageByOfflineId(mNativeOfflinePageBridge, offlineId).getClientId(); + protected ClientId getClientIdForOfflineId(long offlineId) { + OfflinePageItem item = nativeGetPageByOfflineId(mNativeOfflinePageBridge, offlineId); + if (item != null) { + return item.getClientId(); + } + return null; } @CalledByNative @@ -505,8 +509,7 @@ public class OfflinePageBridge { } @CalledByNative - private void offlinePageDeleted(long offlineId) { - ClientId clientId = getClientIdForOfflineId(offlineId); + protected void offlinePageDeleted(long offlineId, ClientId clientId) { for (OfflinePageModelObserver observer : mObservers) { observer.offlinePageDeleted(offlineId, clientId); } @@ -528,6 +531,11 @@ public class OfflinePageBridge { creationTime, accessCount, lastAccessTimeMs); } + @CalledByNative + private static ClientId createClientId(String clientNamespace, String id) { + return new ClientId(clientNamespace, id); + } + private static native int nativeGetFeatureMode(); private static native boolean nativeCanSavePage(String url); private static native OfflinePageBridge nativeGetOfflinePageBridgeForProfile(Profile profile); @@ -536,7 +544,7 @@ public class OfflinePageBridge { long nativeOfflinePageBridge, List<OfflinePageItem> offlinePages); protected native long[] nativeGetOfflineIdsForClientId( long nativeOfflinePageBridge, String clientNamespace, String clientId); - private native OfflinePageItem nativeGetPageByOfflineId( + protected native OfflinePageItem nativeGetPageByOfflineId( long nativeOfflinePageBridge, long offlineId); private native OfflinePageItem nativeGetPageByOnlineURL( long nativeOfflinePageBridge, String onlineURL); diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java index 5f2d216..0aa7aa8d 100644 --- a/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java +++ b/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java @@ -16,6 +16,7 @@ import static org.mockito.Mockito.verify; import org.chromium.base.BaseChromiumApplication; import org.chromium.base.test.util.Feature; +import org.chromium.chrome.browser.offlinepages.OfflinePageBridge.OfflinePageModelObserver; import org.chromium.testing.local.LocalRobolectricTestRunner; import org.junit.Before; import org.junit.Test; @@ -34,6 +35,31 @@ public class OfflinePageBridgeTest { private static final String TEST_NAMESPACE = "TEST_NAMESPACE"; private static final String TEST_ID = "TEST_ID"; + private static final String TEST_URL = "TEST_URL"; + private static final long TEST_OFFLINE_ID = 42; + private static final ClientId TEST_CLIENT_ID = new ClientId(TEST_NAMESPACE, TEST_ID); + private static final String TEST_OFFLINE_URL = "TEST_OFFLINE_URL"; + private static final long TEST_FILESIZE = 12345; + private static final long TEST_CREATIONTIMEMS = 150; + private static final int TEST_ACCESSCOUNT = 1; + private static final long TEST_LASTACCESSTIMEMS = 20160314; + + private static final OfflinePageItem TEST_OFFLINE_PAGE_ITEM = new OfflinePageItem(TEST_URL, + TEST_OFFLINE_ID, TEST_NAMESPACE, TEST_ID, TEST_OFFLINE_URL, TEST_FILESIZE, + TEST_CREATIONTIMEMS, TEST_ACCESSCOUNT, TEST_LASTACCESSTIMEMS); + + /** + * Mocks the observer. + */ + public class MockOfflinePageModelObserver extends OfflinePageModelObserver { + public long lastDeletedOfflineId; + public ClientId lastDeletedClientId; + + public void offlinePageDeleted(long offlineId, ClientId clientId) { + lastDeletedOfflineId = offlineId; + lastDeletedClientId = clientId; + } + } @Before public void setUp() throws Exception { @@ -64,6 +90,59 @@ public class OfflinePageBridgeTest { .nativeGetOfflineIdsForClientId(anyLong(), anyString(), anyString()); } + /** + * Tests getClientIdForOfflineId. + */ + @Test + @Feature({"OfflinePages"}) + public void testGetClientIdForOfflineId() { + doReturn(TEST_OFFLINE_PAGE_ITEM) + .when(mBridge) + .nativeGetPageByOfflineId(anyLong(), eq(TEST_OFFLINE_ID)); + + mBridge.offlinePageModelLoaded(); + long testOfflineId = TEST_OFFLINE_ID; + ClientId resultClientId = mBridge.getClientIdForOfflineId(testOfflineId); + assertEquals(resultClientId, TEST_CLIENT_ID); + verify(mBridge, times(1)).getClientIdForOfflineId(eq(TEST_OFFLINE_ID)); + verify(mBridge, times(1)).nativeGetPageByOfflineId(anyLong(), eq(TEST_OFFLINE_ID)); + } + + /** + * Tests getClientIdForOfflineId for null. + */ + @Test + @Feature({"OfflinePages"}) + public void testGetClientIdForOfflineIdNull() { + doReturn(null).when(mBridge).nativeGetPageByOfflineId(anyLong(), eq(TEST_OFFLINE_ID)); + mBridge.offlinePageModelLoaded(); + long testOfflineId = TEST_OFFLINE_ID; + ClientId resultClientId = mBridge.getClientIdForOfflineId(testOfflineId); + assertEquals(resultClientId, null); + verify(mBridge, times(1)).getClientIdForOfflineId(eq(TEST_OFFLINE_ID)); + verify(mBridge, times(1)).nativeGetPageByOfflineId(anyLong(), eq(TEST_OFFLINE_ID)); + } + + /** + * Tests OfflinePageBridge#OfflinePageDeleted() callback with two observers attached. + */ + @Test + @Feature({"OfflinePages"}) + public void testRemovePageByClientId() { + MockOfflinePageModelObserver observer1 = new MockOfflinePageModelObserver(); + MockOfflinePageModelObserver observer2 = new MockOfflinePageModelObserver(); + mBridge.addObserver(observer1); + mBridge.addObserver(observer2); + + ClientId testClientId = new ClientId(TEST_NAMESPACE, TEST_ID); + long testOfflineId = 123; + mBridge.offlinePageDeleted(testOfflineId, testClientId); + assertEquals(testOfflineId, observer1.lastDeletedOfflineId); + assertEquals(testClientId, observer1.lastDeletedClientId); + assertEquals(testOfflineId, observer2.lastDeletedOfflineId); + assertEquals(testClientId, observer2.lastDeletedClientId); + } + @Test(expected = AssertionError.class) @Feature({"OfflinePages"}) public void testGetPageByClientId_ModelNotLoaded() { diff --git a/chrome/browser/android/offline_pages/offline_page_bridge.cc b/chrome/browser/android/offline_pages/offline_page_bridge.cc index 7f98028..c5021b8 100644 --- a/chrome/browser/android/offline_pages/offline_page_bridge.cc +++ b/chrome/browser/android/offline_pages/offline_page_bridge.cc @@ -131,9 +131,11 @@ void OfflinePageBridge::OfflinePageModelChanged(OfflinePageModel* model) { Java_OfflinePageBridge_offlinePageModelChanged(env, java_ref_.obj()); } -void OfflinePageBridge::OfflinePageDeleted(int64_t offline_id) { +void OfflinePageBridge::OfflinePageDeleted(int64_t offline_id, + const ClientId& client_id) { JNIEnv* env = base::android::AttachCurrentThread(); - Java_OfflinePageBridge_offlinePageDeleted(env, java_ref_.obj(), offline_id); + Java_OfflinePageBridge_offlinePageDeleted( + env, java_ref_.obj(), offline_id, CreateClientId(env, client_id).obj()); } void OfflinePageBridge::GetAllPages(JNIEnv* env, @@ -321,6 +323,15 @@ ScopedJavaLocalRef<jobject> OfflinePageBridge::CreateOfflinePageItem( offline_page.access_count, offline_page.last_access_time.ToJavaTime()); } +ScopedJavaLocalRef<jobject> OfflinePageBridge::CreateClientId( + JNIEnv* env, + const ClientId& client_id) const { + return Java_OfflinePageBridge_createClientId( + env, + ConvertUTF8ToJavaString(env, client_id.name_space).obj(), + ConvertUTF8ToJavaString(env, client_id.id).obj()); +} + bool RegisterOfflinePageBridge(JNIEnv* env) { return RegisterNativesImpl(env); } diff --git a/chrome/browser/android/offline_pages/offline_page_bridge.h b/chrome/browser/android/offline_pages/offline_page_bridge.h index ceee306..7293639 100644 --- a/chrome/browser/android/offline_pages/offline_page_bridge.h +++ b/chrome/browser/android/offline_pages/offline_page_bridge.h @@ -11,6 +11,7 @@ #include "base/android/jni_weak_ref.h" #include "base/macros.h" #include "base/supports_user_data.h" +#include "components/offline_pages/offline_page_item.h" #include "components/offline_pages/offline_page_model.h" namespace content { @@ -35,7 +36,8 @@ class OfflinePageBridge : public OfflinePageModel::Observer, // OfflinePageModel::Observer implementation. void OfflinePageModelLoaded(OfflinePageModel* model) override; void OfflinePageModelChanged(OfflinePageModel* model) override; - void OfflinePageDeleted(int64_t offline_id) override; + void OfflinePageDeleted(int64_t offline_id, + const ClientId& client_id) override; void GetAllPages(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj, @@ -113,6 +115,10 @@ class OfflinePageBridge : public OfflinePageModel::Observer, JNIEnv* env, const OfflinePageItem& offline_page) const; + base::android::ScopedJavaLocalRef<jobject> CreateClientId( + JNIEnv* env, + const ClientId& clientId) const; + base::android::ScopedJavaGlobalRef<jobject> java_ref_; // Not owned. content::BrowserContext* browser_context_; diff --git a/components/offline_pages/offline_page_model.cc b/components/offline_pages/offline_page_model.cc index 9316a97..ac33771 100644 --- a/components/offline_pages/offline_page_model.cc +++ b/components/offline_pages/offline_page_model.cc @@ -520,7 +520,8 @@ void OfflinePageModel::OnMarkPageForDeletionDone( kFinalDeletionDelay); FOR_EACH_OBSERVER(Observer, observers_, - OfflinePageDeleted(offline_page_item.offline_id)); + OfflinePageDeleted(offline_page_item.offline_id, + offline_page_item.client_id)); } void OfflinePageModel::OnUndoOfflinePageDone( @@ -662,8 +663,9 @@ void OfflinePageModel::OnRemoveOfflinePagesDone( // If the page is not marked for deletion at this point, the model has not // yet informed the observer that the offline page is deleted. if (!iter->second.IsMarkedForDeletion()) { - FOR_EACH_OBSERVER(Observer, observers_, - OfflinePageDeleted(iter->second.offline_id)); + FOR_EACH_OBSERVER( + Observer, observers_, + OfflinePageDeleted(iter->second.offline_id, iter->second.client_id)); } offline_pages_.erase(iter); } @@ -694,10 +696,20 @@ void OfflinePageModel::OnFindPagesMissingArchiveFile( if (ids_of_pages_missing_archive_file->empty()) return; + std::vector<std::pair<int64_t, ClientId>> offline_client_id_pairs; + for (auto offline_id : *ids_of_pages_missing_archive_file) { + // Since we might have deleted pages in between so we have to purge + // the list to make sure we still care about them. + auto iter = offline_pages_.find(offline_id); + if (iter != offline_pages_.end()) { + offline_client_id_pairs.push_back( + std::make_pair(offline_id, iter->second.client_id)); + } + } + DeletePageCallback done_callback( base::Bind(&OfflinePageModel::OnRemoveOfflinePagesMissingArchiveFileDone, - weak_ptr_factory_.GetWeakPtr(), - *ids_of_pages_missing_archive_file)); + weak_ptr_factory_.GetWeakPtr(), offline_client_id_pairs)); store_->RemoveOfflinePages( *ids_of_pages_missing_archive_file, @@ -708,10 +720,11 @@ void OfflinePageModel::OnFindPagesMissingArchiveFile( } void OfflinePageModel::OnRemoveOfflinePagesMissingArchiveFileDone( - const std::vector<int64_t>& offline_ids, + const std::vector<std::pair<int64_t, ClientId>>& offline_client_id_pairs, OfflinePageModel::DeletePageResult /* result */) { - for (int64_t offline_id : offline_ids) { - FOR_EACH_OBSERVER(Observer, observers_, OfflinePageDeleted(offline_id)); + for (const auto& id_pair : offline_client_id_pairs) { + FOR_EACH_OBSERVER(Observer, observers_, + OfflinePageDeleted(id_pair.first, id_pair.second)); } } diff --git a/components/offline_pages/offline_page_model.h b/components/offline_pages/offline_page_model.h index e26b538..e76be0d 100644 --- a/components/offline_pages/offline_page_model.h +++ b/components/offline_pages/offline_page_model.h @@ -115,7 +115,8 @@ class OfflinePageModel : public KeyedService, public base::SupportsUserData { // Invoked when an offline copy related to |offline_id| was deleted. // In can be invoked as a result of |CheckForExternalFileDeletion|, if a // deleted page is detected. - virtual void OfflinePageDeleted(int64_t offline_id) = 0; + virtual void OfflinePageDeleted(int64_t offline_id, + const ClientId& client_id) = 0; protected: virtual ~Observer() {} @@ -286,9 +287,9 @@ class OfflinePageModel : public KeyedService, public base::SupportsUserData { // Callbacks for checking if offline pages are missing archive files. void OnFindPagesMissingArchiveFile( - const std::vector<int64_t>* pages_missing_archive_file); + const std::vector<int64_t>* ids_of_pages_missing_archive_file); void OnRemoveOfflinePagesMissingArchiveFileDone( - const std::vector<int64_t>& offline_ids, + const std::vector<std::pair<int64_t, ClientId>>& offline_client_id_pairs, OfflinePageModel::DeletePageResult result); // Steps for clearing all. diff --git a/components/offline_pages/offline_page_model_unittest.cc b/components/offline_pages/offline_page_model_unittest.cc index 3c4e216..5c9e934 100644 --- a/components/offline_pages/offline_page_model_unittest.cc +++ b/components/offline_pages/offline_page_model_unittest.cc @@ -68,7 +68,8 @@ class OfflinePageModelTest // OfflinePageModel::Observer implementation. void OfflinePageModelLoaded(OfflinePageModel* model) override; void OfflinePageModelChanged(OfflinePageModel* model) override; - void OfflinePageDeleted(int64_t offline_id) override; + void OfflinePageDeleted(int64_t offline_id, + const ClientId& client_id) override; // OfflinePageTestArchiver::Observer implementation. void SetLastPathCreatedByArchiver(const base::FilePath& file_path) override; @@ -119,6 +120,8 @@ class OfflinePageModelTest int64_t last_deleted_offline_id() const { return last_deleted_offline_id_; } + ClientId last_deleted_client_id() const { return last_deleted_client_id_; } + const base::FilePath& last_archiver_path() { return last_archiver_path_; } private: @@ -132,6 +135,7 @@ class OfflinePageModelTest DeletePageResult last_delete_result_; base::FilePath last_archiver_path_; int64_t last_deleted_offline_id_; + ClientId last_deleted_client_id_; }; OfflinePageModelTest::OfflinePageModelTest() @@ -166,8 +170,10 @@ void OfflinePageModelTest::OfflinePageModelChanged(OfflinePageModel* model) { ASSERT_EQ(model_.get(), model); } -void OfflinePageModelTest::OfflinePageDeleted(int64_t offline_id) { +void OfflinePageModelTest::OfflinePageDeleted(int64_t offline_id, + const ClientId& client_id) { last_deleted_offline_id_ = offline_id; + last_deleted_client_id_ = client_id; } void OfflinePageModelTest::SetLastPathCreatedByArchiver( @@ -523,6 +529,7 @@ TEST_F(OfflinePageModelTest, DeletePageSuccessful) { PumpLoop(); EXPECT_EQ(last_deleted_offline_id(), offline1); + EXPECT_EQ(last_deleted_client_id(), kTestPageBookmarkId1); EXPECT_EQ(DeletePageResult::SUCCESS, last_delete_result()); ASSERT_EQ(1u, store->GetAllPages().size()); EXPECT_EQ(kTestUrl2, store->GetAllPages()[0].url); @@ -537,6 +544,7 @@ TEST_F(OfflinePageModelTest, DeletePageSuccessful) { PumpLoop(); EXPECT_EQ(last_deleted_offline_id(), offline2); + EXPECT_EQ(last_deleted_client_id(), kTestPageBookmarkId2); EXPECT_EQ(DeletePageResult::SUCCESS, last_delete_result()); EXPECT_EQ(0u, store->GetAllPages().size()); } @@ -597,6 +605,7 @@ TEST_F(OfflinePageModelTest, DetectThatOfflineCopyIsMissingAfterLoad) { PumpLoop(); EXPECT_EQ(last_deleted_offline_id(), offline_id); + EXPECT_EQ(last_deleted_client_id(), kTestPageBookmarkId1); EXPECT_EQ(0UL, model()->GetAllPages().size()); } @@ -928,14 +937,16 @@ TEST_F(OfflinePageModelBookmarkChangeTest, RemoveBookmark) { // Creates a bookmark with offline copy. bookmark_node = CreateBookmarkNode(kTestUrl); - SavePage(kTestUrl, ClientId(BOOKMARK_NAMESPACE, - base::Int64ToString(bookmark_node->id()))); + ClientId client_id = + ClientId(BOOKMARK_NAMESPACE, base::Int64ToString(bookmark_node->id())); + SavePage(kTestUrl, client_id); EXPECT_EQ(1UL, model()->GetAllPages().size()); // The offline copy should also be removed upon the bookmark removal. bookmark_model()->Remove(bookmark_node); PumpLoop(); EXPECT_EQ(0UL, model()->GetAllPages().size()); + EXPECT_EQ(client_id, last_deleted_client_id()); } TEST_F(OfflinePageModelBookmarkChangeTest, UndoBookmarkRemoval) { |