Bug 1345349 - Places keywords break if one of the keywords points to an invalid url r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 12 May 2017 17:28:48 +0200
changeset 576976 fde73d1090fd1de5547d31c5042c7b5aca2d30ca
parent 576975 8a73de724f644192973e88b7685689b048023676
child 628379 8de34a9f0cdc5b44bacbf5af3364c66f635a710c
push id58555
push usermak77@bonardo.net
push dateFri, 12 May 2017 16:09:21 +0000
reviewersstandard8
bugs1345349
milestone55.0a1
Bug 1345349 - Places keywords break if one of the keywords points to an invalid url r=standard8 MozReview-Commit-ID: IbTNqPrreAR
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/unit/test_keywords.js
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1500,18 +1500,18 @@ this.PlacesUtils = {
    *
    * This is intended to be used mostly internally, components outside of
    * Places should, when possible, use API calls and file bugs to get proper
    * APIs, where they are missing.
    * Keep in mind the Places DB schema is by no means frozen or even stable.
    * Your custom queries can - and will - break overtime.
    *
    * Example:
-   * let db = yield PlacesUtils.promiseDBConnection();
-   * let rows = yield db.executeCached(sql, params);
+   * let db = await PlacesUtils.promiseDBConnection();
+   * let rows = await db.executeCached(sql, params);
    */
   promiseDBConnection: () => gAsyncDBConnPromised,
 
   /**
    * Performs a read/write operation on the Places database through a Sqlite.jsm
    * wrapped connection to the Places database.
    *
    * This is intended to be used only by Places itself, always use APIs if you
@@ -1521,17 +1521,17 @@ this.PlacesUtils = {
    * Your custom queries can - and will - break overtime.
    *
    * As all operations on the Places database are asynchronous, if shutdown
    * is initiated while an operation is pending, this could cause dataloss.
    * Using `withConnectionWrapper` ensures that shutdown waits until all
    * operations are complete before proceeding.
    *
    * Example:
-   * yield withConnectionWrapper("Bookmarks: Remove a bookmark", Task.async(function*(db) {
+   * await withConnectionWrapper("Bookmarks: Remove a bookmark", Task.async(function*(db) {
    *    // Proceed with the db, asynchronously.
    *    // Shutdown will not interrupt operations that take place here.
    * }));
    *
    * @param {string} name The name of the operation. Used for debugging, logging
    *   and crash reporting.
    * @param {function(db)} task A function that takes as argument a Sqlite.jsm
    *   connection and returns a Promise. Shutdown is guaranteed to not interrupt
@@ -2403,22 +2403,35 @@ XPCOMUtils.defineLazyGetter(this, "gKeyw
   PlacesUtils.withConnectionWrapper("PlacesUtils: gKeywordsCachePromise",
     async function(db) {
       let cache = new Map();
       let rows = await db.execute(
         `SELECT keyword, url, post_data
          FROM moz_keywords k
          JOIN moz_places h ON h.id = k.place_id
         `);
+      let brokenKeywords = [];
       for (let row of rows) {
         let keyword = row.getResultByName("keyword");
-        let entry = { keyword,
-                      url: new URL(row.getResultByName("url")),
-                      postData: row.getResultByName("post_data") };
-        cache.set(keyword, entry);
+        try {
+          let entry = { keyword,
+                        url: new URL(row.getResultByName("url")),
+                        postData: row.getResultByName("post_data") };
+          cache.set(keyword, entry);
+        } catch (ex) {
+          // The url is invalid, don't load the keyword and remove it, or it
+          // would break the whole keywords API.
+          brokenKeywords.push(keyword);
+        }
+      }
+      if (brokenKeywords.length) {
+        await db.execute(
+          `DELETE FROM moz_keywords
+           WHERE keyword IN (${brokenKeywords.map(JSON.stringify).join(",")})
+          `);
       }
 
       // Helper to get a keyword from an href.
       function keywordsForHref(href) {
         let keywords = [];
         for (let [ key, val ] of cache) {
           if (val.url.href == href)
             keywords.push(key);
--- a/toolkit/components/places/tests/unit/test_keywords.js
+++ b/toolkit/components/places/tests/unit/test_keywords.js
@@ -14,17 +14,21 @@ async function check_keyword(aExpectExis
     Assert.equal(entry.postData, aPostData);
     Assert.deepEqual(entry, await PlacesUtils.keywords.fetch({ keyword: aKeyword, url: aHref }));
     let entries = [];
     await PlacesUtils.keywords.fetch({ url: aHref }, e => entries.push(e));
     Assert.ok(entries.some(e => e.url.href == aHref && e.keyword == aKeyword.toLowerCase()));
   } else {
     Assert.ok(!entry || entry.url.href != aHref,
               "The given keyword entry should not exist");
-    Assert.equal(null, await PlacesUtils.keywords.fetch({ keyword: aKeyword, url: aHref }));
+    if (aHref) {
+      Assert.equal(null, await PlacesUtils.keywords.fetch({ keyword: aKeyword, url: aHref }));
+    } else {
+      Assert.equal(null, await PlacesUtils.keywords.fetch({ keyword: aKeyword }));
+    }
   }
 }
 
 /**
  * Polls the keywords cache waiting for the given keyword entry.
  */
 async function promiseKeyword(keyword, expectedHref) {
   let href = null;
@@ -74,16 +78,40 @@ function expectBookmarkNotifications() {
         return target[name];
       return undefined;
     }
   });
   PlacesUtils.bookmarks.addObserver(observer);
   return observer;
 }
 
+// This test must be the first one, since it creates the keywords cache.
+add_task(async function test_invalidURL() {
+  await PlacesTestUtils.addVisits("http://test.com/");
+  // Change to url to an invalid one, there's no API for that, so we must do
+  // that manually.
+  await PlacesUtils.withConnectionWrapper("test_invalidURL", async function(db) {
+    await db.execute(
+      `UPDATE moz_places SET url = :broken, url_hash = hash(:broken)
+       WHERE id = (SELECT id FROM moz_places WHERE url_hash = hash(:url))`,
+      { url: "http://test.com/", broken: "<invalid url>" });
+
+    await db.execute(
+      `INSERT INTO moz_keywords (keyword, place_id)
+       VALUES (:kw, (SELECT id FROM moz_places WHERE url_hash = hash(:broken)))`,
+      { broken: "<invalid url>", kw: "keyword" });
+  });
+  await check_keyword(false, "http://broken.com/", "keyword");
+  await check_keyword(false, null, "keyword");
+  await PlacesUtils.withConnectionWrapper("test_invalidURL", async function(db) {
+    let rows = await db.execute(`SELECT * FROM moz_keywords`);
+    Assert.equal(rows.length, 0, "The broken keyword should have been removed");
+  });
+});
+
 add_task(async function test_invalid_input() {
   Assert.throws(() => PlacesUtils.keywords.fetch(null),
                 /Invalid keyword/);
   Assert.throws(() => PlacesUtils.keywords.fetch(5),
                 /Invalid keyword/);
   Assert.throws(() => PlacesUtils.keywords.fetch(undefined),
                 /Invalid keyword/);
   Assert.throws(() => PlacesUtils.keywords.fetch({ keyword: null }),
@@ -169,17 +197,16 @@ add_task(async function test_addKeyword(
 
   await check_no_orphans();
 });
 
 add_task(async function test_addBookmarkAndKeyword() {
   await check_keyword(false, "http://example.com/", "keyword");
   let fc = await foreign_count("http://example.com/");
   let bookmark = await PlacesUtils.bookmarks.insert({ url: "http://example.com/",
-                                                      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                       parentGuid: PlacesUtils.bookmarks.unfiledGuid });
 
   let observer = expectBookmarkNotifications();
   await PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" });
 
   observer.check([{ name: "onItemChanged",
                     arguments: [ (await PlacesUtils.promiseItemId(bookmark.guid)),
                                  "keyword", false, "keyword",
@@ -265,17 +292,16 @@ add_task(async function test_addBookmark
   await PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" });
   observer.check([]);
 
   await check_keyword(true, "http://example.com/", "keyword");
   Assert.equal((await foreign_count("http://example.com/")), fc + 1); // +1 keyword
 
   observer = expectBookmarkNotifications();
   let bookmark = await PlacesUtils.bookmarks.insert({ url: "http://example.com/",
-                                                      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                       parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   Assert.equal((await foreign_count("http://example.com/")), fc + 2); // +1 bookmark
   observer.check([]);
 
   observer = expectBookmarkNotifications();
   await PlacesUtils.bookmarks.remove(bookmark.guid);
   // the notification is synchronous but the removal process is async.
   // Unfortunately there's nothing explicit we can wait for.
@@ -287,21 +313,19 @@ add_task(async function test_addBookmark
   await check_keyword(false, "http://example.com/", "keyword");
 
   await check_no_orphans();
 });
 
 add_task(async function test_sameKeywordDifferentURL() {
   let fc1 = await foreign_count("http://example1.com/");
   let bookmark1 = await PlacesUtils.bookmarks.insert({ url: "http://example1.com/",
-                                                       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                        parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   let fc2 = await foreign_count("http://example2.com/");
   let bookmark2 = await PlacesUtils.bookmarks.insert({ url: "http://example2.com/",
-                                                       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                        parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   await PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example1.com/" });
 
   await check_keyword(true, "http://example1.com/", "keyword");
   Assert.equal((await foreign_count("http://example1.com/")), fc1 + 2); // +1 bookmark +1 keyword
   await check_keyword(false, "http://example2.com/", "keyword");
   Assert.equal((await foreign_count("http://example2.com/")), fc2 + 1); // +1 bookmark
 
@@ -353,17 +377,16 @@ add_task(async function test_sameKeyword
   await check_no_orphans();
 });
 
 add_task(async function test_sameURIDifferentKeyword() {
   let fc = await foreign_count("http://example.com/");
 
   let observer = expectBookmarkNotifications();
   let bookmark = await PlacesUtils.bookmarks.insert({ url: "http://example.com/",
-                                                      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                       parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   await PlacesUtils.keywords.insert({keyword: "keyword", url: "http://example.com/" });
 
   await check_keyword(true, "http://example.com/", "keyword");
   Assert.equal((await foreign_count("http://example.com/")), fc + 2); // +1 bookmark +1 keyword
 
   observer.check([{ name: "onItemChanged",
                     arguments: [ (await PlacesUtils.promiseItemId(bookmark.guid)),
@@ -418,20 +441,18 @@ add_task(async function test_sameURIDiff
   check_no_orphans();
 });
 
 add_task(async function test_deleteKeywordMultipleBookmarks() {
   let fc = await foreign_count("http://example.com/");
 
   let observer = expectBookmarkNotifications();
   let bookmark1 = await PlacesUtils.bookmarks.insert({ url: "http://example.com/",
-                                                       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                        parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   let bookmark2 = await PlacesUtils.bookmarks.insert({ url: "http://example.com/",
-                                                       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                        parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   await PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" });
 
   await check_keyword(true, "http://example.com/", "keyword");
   Assert.equal((await foreign_count("http://example.com/")), fc + 3); // +2 bookmark +1 keyword
   observer.check([{ name: "onItemChanged",
                     arguments: [ (await PlacesUtils.promiseItemId(bookmark2.guid)),
                                  "keyword", false, "keyword",
@@ -484,33 +505,31 @@ add_task(async function test_multipleKey
 
   await PlacesUtils.keywords.remove("keyword");
 
   check_no_orphans();
 });
 
 add_task(async function test_oldPostDataAPI() {
   let bookmark = await PlacesUtils.bookmarks.insert({ url: "http://example.com/",
-                                                      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                       parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   await PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" });
   let itemId = await PlacesUtils.promiseItemId(bookmark.guid);
   await PlacesUtils.setPostDataForBookmark(itemId, "postData");
   await check_keyword(true, "http://example.com/", "keyword", "postData");
   Assert.equal(PlacesUtils.getPostDataForBookmark(itemId), "postData");
 
   await PlacesUtils.keywords.remove("keyword");
   await PlacesUtils.bookmarks.remove(bookmark);
 
   check_no_orphans();
 });
 
 add_task(async function test_oldKeywordsAPI() {
   let bookmark = await PlacesUtils.bookmarks.insert({ url: "http://example.com/",
-                                                    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                     parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   await check_keyword(false, "http://example.com/", "keyword");
   let itemId = await PlacesUtils.promiseItemId(bookmark.guid);
 
   PlacesUtils.bookmarks.setKeywordForBookmark(itemId, "keyword");
   await promiseKeyword("keyword", "http://example.com/");
 
   // Remove the keyword.
@@ -527,17 +546,16 @@ add_task(async function test_oldKeywords
 
   check_no_orphans();
 });
 
 add_task(async function test_bookmarkURLChange() {
   let fc1 = await foreign_count("http://example1.com/");
   let fc2 = await foreign_count("http://example2.com/");
   let bookmark = await PlacesUtils.bookmarks.insert({ url: "http://example1.com/",
-                                                      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                       parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   await PlacesUtils.keywords.insert({ keyword: "keyword",
                                       url: "http://example1.com/" });
 
   await check_keyword(true, "http://example1.com/", "keyword");
   Assert.equal((await foreign_count("http://example1.com/")), fc1 + 2); // +1 bookmark +1 keyword
 
   await PlacesUtils.bookmarks.update({ guid: bookmark.guid,