Read later: Fix crash and check URL scheme when adding a reading list.

We should expect ReadingListManager::Add to return nullptr when the
title can't be parsed as UTF strings, or the URL scheme is not http or
https.

Bug: 1157808,1163225
Change-Id: I1d4e9ceb74f2821bda19907936d43919c8cc9dbd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611628Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840779}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkBridge.java

@@ -840,9 +840,10 @@ public class BookmarkBridge {
      * bookmark ID will be returned.
      * @param title The title to be used for the reading list item.
      * @param url The URL of the reading list item.
-     * @return The bookmark ID created after saving the article to the reading list.
+     * @return The bookmark ID created after saving the article to the reading list, or null on
+     *         error.
      */
-    public BookmarkId addToReadingList(String title, String url) {
+    public @Nullable BookmarkId addToReadingList(String title, String url) {
         ThreadUtils.assertOnUiThread();
         assert title != null;
         assert url != null;

chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkBridgeTest.java

@@ -18,10 +18,13 @@ import org.chromium.base.test.UiThreadTest;
 import org.chromium.base.test.util.Batch;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.test.ChromeBrowserTestRule;
 import org.chromium.chrome.test.util.BookmarkTestUtil;
+import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.components.bookmarks.BookmarkId;
+import org.chromium.components.bookmarks.BookmarkType;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 
 import java.util.ArrayList;
@@ -314,4 +317,21 @@ public class BookmarkBridgeTest {
                         + "over partner bookmarks",
                 expectedSearchResults, searchResults);
     }
+
+    @Test
+    @SmallTest
+    @UiThreadTest
+    @Features.EnableFeatures({ChromeFeatureList.READ_LATER})
+    public void testAddToReadingList() {
+        Assert.assertNull("Should return null for non http/https URLs.",
+                mBookmarkBridge.addToReadingList("a", "chrome://flags"));
+        BookmarkId readingListId = mBookmarkBridge.addToReadingList("a", "https://www.google.com/");
+        Assert.assertNotNull(readingListId);
+        Assert.assertEquals(BookmarkType.READING_LIST, readingListId.getType());
+        BookmarkItem readingListItem =
+                mBookmarkBridge.getReadingListItem("https://www.google.com/");
+        Assert.assertNotNull(readingListItem);
+        Assert.assertEquals("https://www.google.com/", readingListItem.getUrl());
+        Assert.assertEquals("a", readingListItem.getTitle());
+    }
 }

chrome/browser/android/bookmarks/bookmark_bridge.cc

@@ -828,8 +828,9 @@ ScopedJavaLocalRef<jobject> BookmarkBridge::AddToReadingList(
   const BookmarkNode* node = reading_list_manager_->Add(
       GURL(base::android::ConvertJavaStringToUTF16(env, j_url)),
       base::android::ConvertJavaStringToUTF8(env, j_title));
-  DCHECK(node);
-  return JavaBookmarkIdCreateBookmarkId(env, node->id(), GetBookmarkType(node));
+  return node ? JavaBookmarkIdCreateBookmarkId(env, node->id(),
+                                               GetBookmarkType(node))
+              : ScopedJavaLocalRef<jobject>();
 }
 
 ScopedJavaLocalRef<jobject> BookmarkBridge::GetReadingListItem(

chrome/browser/reading_list/android/reading_list_manager.h

@@ -47,7 +47,7 @@ class ReadingListManager : public KeyedService {
   // Adds a reading list article to the unread section, and return the bookmark
   // node representation. The bookmark node is owned by this class. If there is
   // a duplicate URL, a new bookmark node will be created, and the old bookmark
-  // node pointer will be invalidated.
+  // node pointer will be invalidated. May return nullptr on error.
   virtual const bookmarks::BookmarkNode* Add(const GURL& url,
                                              const std::string& title) = 0;
 

chrome/browser/reading_list/android/reading_list_manager_impl.cc

@@ -132,13 +132,13 @@ void ReadingListManagerImpl::RemoveObserver(Observer* observer) {
 const BookmarkNode* ReadingListManagerImpl::Add(const GURL& url,
                                                 const std::string& title) {
   DCHECK(reading_list_model_->loaded());
+  if (!reading_list_model_->IsUrlSupported(url))
+    return nullptr;
 
   // Add or swap the reading list entry.
   const auto& new_entry = reading_list_model_->AddEntry(
       url, title, reading_list::ADDED_VIA_CURRENT_APP);
   const auto* node = FindBookmarkByURL(new_entry.URL());
-  DCHECK(node)
-      << "Bookmark node should have been create in ReadingListDidAddEntry().";
   return node;
 }
 

chrome/browser/reading_list/android/reading_list_manager_impl_unittest.cc

@@ -32,6 +32,7 @@ constexpr char kTitle1[] = "boring title about dogs.";
 constexpr char kReadStatusKey[] = "read_status";
 constexpr char kReadStatusRead[] = "true";
 constexpr char kReadStatusUnread[] = "false";
+constexpr char kInvalidUTF8[] = "\xc3\x28";
 
 class MockObserver : public ReadingListManager::Observer {
  public:
@@ -228,6 +229,31 @@ TEST_F(ReadingListManagerImplTest, AddTwice) {
   EXPECT_EQ(url, new_node->url());
 }
 
+// If Add() with an invalid title, nullptr will be returned.
+TEST_F(ReadingListManagerImplTest, AddInvalidTitle) {
+  GURL url(kURL);
+
+  // Use an invalid UTF8 string.
+  base::string16 dummy;
+  EXPECT_FALSE(
+      base::UTF8ToUTF16(kInvalidUTF8, base::size(kInvalidUTF8), &dummy));
+  const auto* new_node = Add(url, std::string(kInvalidUTF8));
+  EXPECT_EQ(nullptr, new_node)
+      << "Should return nullptr when failed to parse the title.";
+}
+
+// If Add() with an invalid URL, nullptr will be returned.
+TEST_F(ReadingListManagerImplTest, AddInvalidURL) {
+  GURL invalid_url("chrome://flags");
+  EXPECT_FALSE(reading_list_model()->IsUrlSupported(invalid_url));
+
+  // Use an invalid URL, the observer method ReadingListDidAddEntry() won't be
+  // invoked.
+  const auto* new_node = manager()->Add(invalid_url, kTitle);
+  EXPECT_EQ(nullptr, new_node)
+      << "Should return nullptr when the URL scheme is not supported.";
+}
+
 // Verifes SetReadStatus()/GetReadStatus() API.
 TEST_F(ReadingListManagerImplTest, ReadStatus) {
   GURL url(kURL);