--- 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,