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

Diff:

1. Remove test BookmarkBridgeTest.testAddToReadingList from this CL as
@Batch is causing multiple times failure on official bots. The test will
be added in a separate CL.

2. Add a logging in EmptyReadingListManager.Add.

3. Add a logging when failed to parse UTF16 in
ReadingListManagerImpl::SyncToBookmark, this will help us to debug
crbug/1163876.

Bug: 1157808,1163225

Change-Id: I6b734ea67db192c1108f843699e7835c0e19a9da
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626075Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843694}

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/browser/android/bookmarks/bookmark_bridge.cc

@@ -833,8 +833,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/empty_reading_list_manager.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/reading_list/android/empty_reading_list_manager.h"
 
+#include "base/logging.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 
 EmptyReadingListManager::EmptyReadingListManager() = default;
@@ -17,6 +18,9 @@ void EmptyReadingListManager::RemoveObserver(Observer* observer) {}
 const bookmarks::BookmarkNode* EmptyReadingListManager::Add(
     const GURL& url,
     const std::string& title) {
+  LOG(ERROR)
+      << "Try to add reading list with empty reading list backend with title:"
+      << title;
   return nullptr;
 }
 

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

@@ -31,7 +31,8 @@ bool SyncToBookmark(const ReadingListEntry& entry, BookmarkNode* bookmark) {
   DCHECK(bookmark);
   base::string16 title;
   if (!base::UTF8ToUTF16(entry.Title().c_str(), entry.Title().size(), &title)) {
-    LOG(ERROR) << "Failed to convert the title to string16.";
+    LOG(ERROR) << "Failed to convert the following title to string16:"
+               << entry.Title();
     return false;
   }
 
@@ -132,13 +133,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);