Bug 646993 - Cap the bookmark title length to TITLE_LENGTH_MAX r=mak
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Fri, 28 Oct 2011 20:37:13 -0700
changeset 79392 1a52298b6718
parent 79391 3ad7f12bde01
child 79393 c33a85b6c9d4
child 79417 6094246d2abc
push id21391
push usermak77@bonardo.net
push date2011-10-29 08:32 +0000
treeherdermozilla-central@1a52298b6718 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs646993
milestone10.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 646993 - Cap the bookmark title length to TITLE_LENGTH_MAX r=mak
toolkit/components/places/Helpers.cpp
toolkit/components/places/Helpers.h
toolkit/components/places/nsINavBookmarksService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/tests/bookmarks/test_bookmarks.js
toolkit/components/places/tests/head_common.js
--- a/toolkit/components/places/Helpers.cpp
+++ b/toolkit/components/places/Helpers.cpp
@@ -348,16 +348,25 @@ IsValidGUID(const nsCString& aGUID)
       continue;
     }
     return false;
   }
   return true;
 }
 
 void
+TruncateTitle(const nsACString& aTitle, nsACString& aTrimmed)
+{
+  aTrimmed = aTitle;
+  if (aTitle.Length() > TITLE_LENGTH_MAX) {
+    aTrimmed = StringHead(aTitle, TITLE_LENGTH_MAX);
+  }
+}
+
+void
 ForceWALCheckpoint()
 {
   nsRefPtr<Database> DB = Database::GetDatabase();
   if (DB) {
     nsCOMPtr<mozIStorageAsyncStatement> stmt = DB->GetAsyncStatement(
       "pragma wal_checkpoint "
     );
     if (stmt) {
--- a/toolkit/components/places/Helpers.h
+++ b/toolkit/components/places/Helpers.h
@@ -145,17 +145,17 @@ nsresult GetReversedHostname(nsIURI* aUR
 void GetReversedHostname(const nsString& aForward, nsString& aRevHost);
 
 /**
  * Reverses a string.
  *
  * @param aInput
  *        The string to be reversed
  * @param aReversed
- *        Ouput parameter will contain the reversed string
+ *        Output parameter will contain the reversed string
  */
 void ReverseString(const nsString& aInput, nsString& aReversed);
 
 /**
  * Generates an 12 character guid to be used by bookmark and history entries.
  *
  * @note This guid uses the characters a-z, A-Z, 0-9, '-', and '_'.
  */
@@ -166,16 +166,26 @@ nsresult GenerateGUID(nsCString& _guid);
  *
  * @param aGUID
  *        The guid to test.
  * @return true if it is a valid guid, false otherwise.
  */
 bool IsValidGUID(const nsCString& aGUID);
 
 /**
+ * Truncates the title if it's longer than TITLE_LENGTH_MAX.
+ *
+ * @param aTitle
+ *        The title to truncate (if necessary)
+ * @param aTrimmed
+ *        Output parameter to return the trimmed string
+ */
+void TruncateTitle(const nsACString& aTitle, nsACString& aTrimmed);
+
+/**
  * Used to finalize a statementCache on a specified thread.
  */
 template<typename StatementType>
 class FinalizeStatementCacheProxy : public nsRunnable
 {
 public:
   /**
    * Constructor.
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -328,16 +328,19 @@ interface nsINavBookmarksService : nsISu
    *         The id of the parent folder
    *  @param aURI
    *         The URI to insert
    *  @param aIndex
    *         The index to insert at, or DEFAULT_INDEX to append
    *  @param aTitle
    *         The title for the new bookmark
    *  @return The ID of the newly-created bookmark.
+   *
+   *  @note aTitle will be truncated to TITLE_LENGTH_MAX and
+   *        aURI will be truncated to URI_LENGTH_MAX.
    */
   long long insertBookmark(in long long aParentId, in nsIURI aURI,
                            in long aIndex, in AUTF8String aTitle);
 
   /**
    * Removes a child item. Used to delete a bookmark or separator.
    *  @param aItemId
    *         The child item to remove
@@ -466,19 +469,21 @@ interface nsINavBookmarksService : nsISu
    *          The GUID string of the item to search for
    * @return The item ID, or -1 if not found.
    */
   [deprecated] long long getItemIdForGUID(in AString aGUID);
 
   /**
    * Set the title for an item.
    *  @param aItemId
-   *         The id of the item whose title should be updated 
+   *         The id of the item whose title should be updated.
    *  @param aTitle
    *         The new title for the bookmark.
+   *
+   *  @note  aTitle will be truncated to TITLE_LENGTH_MAX.
    */
   void setItemTitle(in long long aItemId, in AUTF8String aTitle);
 
   /**
    * Get the title for an item.
    *
    * If no item title is available it will return a void string (null in JS).
    *
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -701,18 +701,21 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
     // Create space for the insertion.
     rv = AdjustIndices(aFolder, index, PR_INT32_MAX, 1);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   *aNewBookmarkId = -1;
   PRTime dateAdded = PR_Now();
   nsCAutoString guid;
+  nsCString title;
+  TruncateTitle(aTitle, title);
+
   rv = InsertBookmarkInDB(placeId, BOOKMARK, aFolder, index,
-                          aTitle, dateAdded, nsnull, EmptyString(),
+                          title, dateAdded, nsnull, EmptyString(),
                           aNewBookmarkId, guid);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Recalculate frecency for this entry, since it changed.
   rv = history->UpdateFrecency(placeId);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = transaction.Commit();
@@ -720,17 +723,17 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
 
   if (!mBatching) {
     ForceWALCheckpoint();
   }
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemAdded(*aNewBookmarkId, aFolder, index, TYPE_BOOKMARK,
-                               aURI, aTitle, dateAdded, guid, folderGuid));
+                               aURI, title, dateAdded, guid, folderGuid));
 
   // If the bookmark has been added to a tag container, notify all
   // bookmark-folder result nodes which contain a bookmark for the new
   // bookmark's url.
   if (grandParentId == mTagsRoot) {
     // Notify a tags change to all bookmarks for this URI.
     nsTArray<BookmarkData> bookmarks;
     rv = GetBookmarksForURI(aURI, bookmarks);
@@ -989,32 +992,35 @@ nsNavBookmarks::CreateContainerWithID(PR
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   *aNewFolder = aItemId;
   PRTime dateAdded = PR_Now();
   nsCAutoString guid;
   ItemType containerType = aIsBookmarkFolder ? FOLDER
                                              : DYNAMIC_CONTAINER;
+  nsCString title;
+  TruncateTitle(aTitle, title);
+
   rv = InsertBookmarkInDB(-1, containerType, aParent, index,
-                          aTitle, dateAdded, nsnull, aContractId, aNewFolder,
+                          title, dateAdded, nsnull, aContractId, aNewFolder,
                           guid);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (!mBatching) {
     ForceWALCheckpoint();
   }
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemAdded(*aNewFolder, aParent, index, containerType,
-                               nsnull, aTitle, dateAdded, guid, folderGuid));
+                               nsnull, title, dateAdded, guid, folderGuid));
 
   *aIndex = index;
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::InsertSeparator(PRInt64 aParent,
@@ -1878,23 +1884,27 @@ nsNavBookmarks::SetItemTitle(PRInt64 aIt
 
   nsCOMPtr<mozIStorageStatement> statement = mDB->GetStatement(
     "UPDATE moz_bookmarks SET title = :item_title, lastModified = :date "
     "WHERE id = :item_id "
   );
   NS_ENSURE_STATE(statement);
   mozStorageStatementScoper scoper(statement);
 
+
+  nsCString title;
+  TruncateTitle(aTitle, title);
+
   // Support setting a null title, we support this in insertBookmark.
-  if (aTitle.IsVoid()) {
+  if (title.IsVoid()) {
     rv = statement->BindNullByName(NS_LITERAL_CSTRING("item_title"));
   }
   else {
     rv = statement->BindUTF8StringByName(NS_LITERAL_CSTRING("item_title"),
-                                         aTitle);
+                                         title);
   }
   NS_ENSURE_SUCCESS(rv, rv);
   bookmark.lastModified = PR_Now();
   rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("date"),
                                   bookmark.lastModified);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), bookmark.id);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -1904,17 +1914,17 @@ nsNavBookmarks::SetItemTitle(PRInt64 aIt
 
   END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemChanged(bookmark.id,
                                  NS_LITERAL_CSTRING("title"),
                                  false,
-                                 aTitle,
+                                 title,
                                  bookmark.lastModified,
                                  bookmark.type,
                                  bookmark.parentId,
                                  bookmark.guid,
                                  bookmark.parentGuid));
   return NS_OK;
 }
 
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -206,19 +206,23 @@ public:
    * @param _pendingStmt
    *        The Storage pending statement that will be used to control async
    *        execution.
    */
   nsresult QueryFolderChildrenAsync(nsNavHistoryFolderResultNode* aNode,
                                     PRInt64 aFolderId,
                                     mozIStoragePendingStatement** _pendingStmt);
 
-  // If aFolder is -1, uses the autoincrement id for folder index. Returns
-  // the index of the new folder in aIndex, whether it was passed in or
-  // generated by autoincrement.
+  /**
+   * @return index of the new folder in aIndex, whether it was passed in or
+   *         generated by autoincrement.
+   *
+   * @note If aFolder is -1, uses the autoincrement id for folder index.
+   * @note aTitle will be truncated to TITLE_LENGTH_MAX
+   */
   nsresult CreateContainerWithID(PRInt64 aId, PRInt64 aParent,
                                  const nsACString& aTitle,
                                  const nsAString& aContractId,
                                  bool aIsBookmarkFolder,
                                  PRInt32* aIndex,
                                  PRInt64* aNewFolder);
 
   /**
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks.js
@@ -51,16 +51,17 @@ let bookmarksObserver = {
     this._endUpdateBatch = true;
   },
   onItemAdded: function(id, folder, index, itemType, uri, title, dateAdded,
                         guid) {
     this._itemAddedId = id;
     this._itemAddedParent = folder;
     this._itemAddedIndex = index;
     this._itemAddedURI = uri;
+    this._itemAddedTitle = title;
 
     // Ensure that we've created a guid for this item.
     let stmt = DBConn().createStatement(
       "SELECT guid "
     + "FROM moz_bookmarks "
     + "WHERE id = :item_id "
     );
     stmt.params.item_id = id;
@@ -642,16 +643,33 @@ function run_test() {
   bs.removeItem(newId3);
   do_check_false(anno.itemHasAnnotation(newId3, "test-annotation"));
 
   // bug 378820
   let uri1 = uri("http://foo.tld/a");
   bs.insertBookmark(testRoot, uri1, bs.DEFAULT_INDEX, "");
   hs.addVisit(uri1, Date.now() * 1000, null, hs.TRANSITION_TYPED, false, 0);
 
+  // bug 646993 - test bookmark titles longer than the maximum allowed length
+  let title15 = Array(TITLE_LENGTH_MAX + 5).join("X");
+  let title15expected = title15.substring(0, TITLE_LENGTH_MAX);
+  let newId15 = bs.insertBookmark(testRoot, uri("http://evil.com/"),
+                                  bs.DEFAULT_INDEX, title15);
+
+  do_check_eq(bs.getItemTitle(newId15).length,
+              title15expected.length);
+  do_check_eq(bookmarksObserver._itemAddedTitle, title15expected);
+  // test title length after updates
+  bs.setItemTitle(newId15, title15 + " updated");
+  do_check_eq(bs.getItemTitle(newId15).length,
+              title15expected.length);
+  do_check_eq(bookmarksObserver._itemChangedId, newId15);
+  do_check_eq(bookmarksObserver._itemChangedProperty, "title");
+  do_check_eq(bookmarksObserver._itemChangedValue, title15expected);
+
   testSimpleFolderResult();
 }
 
 function testSimpleFolderResult() {
   // the time before we create a folder, in microseconds
   // Workaround possible VM timers issues subtracting 1us.
   let beforeCreate = Date.now() * 1000 - 1;
   do_check_true(beforeCreate > 0);
@@ -683,39 +701,57 @@ function testSimpleFolderResult() {
   let item = bs.insertBookmark(parent, uri("about:blank"),
                                bs.DEFAULT_INDEX, "");
   bs.setItemTitle(item, "test bookmark");
 
   // see above
   let folder = bs.createFolder(parent, "test folder", bs.DEFAULT_INDEX);
   bs.setItemTitle(folder, "test folder");
 
+  let longName = Array(TITLE_LENGTH_MAX + 5).join("A");
+  let folderLongName = bs.createFolder(parent, longName, bs.DEFAULT_INDEX);
+  do_check_eq(bookmarksObserver._itemAddedTitle, longName.substring(0, TITLE_LENGTH_MAX));
+
   let options = hs.getNewQueryOptions();
   let query = hs.getNewQuery();
   query.setFolders([parent], 1);
   let result = hs.executeQuery(query, options);
   let rootNode = result.root;
   rootNode.containerOpen = true;
-  do_check_eq(rootNode.childCount, 3);
+  do_check_eq(rootNode.childCount, 4);
 
   let node = rootNode.getChild(0);
   do_check_true(node.dateAdded > 0);
   do_check_eq(node.lastModified, node.dateAdded);
   do_check_eq(node.itemId, sep);
   do_check_eq(node.title, "");
   node = rootNode.getChild(1);
   do_check_eq(node.itemId, item);
   do_check_true(node.dateAdded > 0);
   do_check_true(node.lastModified > 0);
   do_check_eq(node.title, "test bookmark");
   node = rootNode.getChild(2);
   do_check_eq(node.itemId, folder);
   do_check_eq(node.title, "test folder");
   do_check_true(node.dateAdded > 0);
   do_check_true(node.lastModified > 0);
+  node = rootNode.getChild(3);
+  do_check_eq(node.itemId, folderLongName);
+  do_check_eq(node.title, longName.substring(0, TITLE_LENGTH_MAX));
+  do_check_true(node.dateAdded > 0);
+  do_check_true(node.lastModified > 0);
+
+  // update with another long title
+  bs.setItemTitle(folderLongName, longName + " updated");
+  do_check_eq(bookmarksObserver._itemChangedId, folderLongName);
+  do_check_eq(bookmarksObserver._itemChangedProperty, "title");
+  do_check_eq(bookmarksObserver._itemChangedValue, longName.substring(0, TITLE_LENGTH_MAX));
+
+  node = rootNode.getChild(3);
+  do_check_eq(node.title, longName.substring(0, TITLE_LENGTH_MAX));
 
   rootNode.containerOpen = false;
 }
 
 function getChildCount(aFolderId) {
   let cc = -1;
   try {
     let options = hs.getNewQueryOptions();
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -50,16 +50,18 @@ const TRANSITION_FRAMED_LINK = Ci.nsINav
 const TRANSITION_REDIRECT_PERMANENT = Ci.nsINavHistoryService.TRANSITION_REDIRECT_PERMANENT;
 const TRANSITION_REDIRECT_TEMPORARY = Ci.nsINavHistoryService.TRANSITION_REDIRECT_TEMPORARY;
 const TRANSITION_DOWNLOAD = Ci.nsINavHistoryService.TRANSITION_DOWNLOAD;
 
 // This error icon must stay in sync with FAVICON_ERRORPAGE_URL in
 // nsIFaviconService.idl, aboutCertError.xhtml and netError.xhtml.
 const FAVICON_ERRORPAGE_URL = "chrome://global/skin/icons/warning-16.png";
 
+const TITLE_LENGTH_MAX = 4096;
+
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyGetter(this, "Services", function() {
   Cu.import("resource://gre/modules/Services.jsm");
   return Services;
 });
 
 XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {