--- a/toolkit/components/places/BookmarkHTMLUtils.jsm
+++ b/toolkit/components/places/BookmarkHTMLUtils.jsm
@@ -915,17 +915,17 @@ BookmarkImporter.prototype = {
let bookmarksTrees = this._getBookmarkTrees();
for (let tree of bookmarksTrees) {
if (!tree.children.length) {
continue;
}
// Give the tree the source.
tree.source = this._source;
- await PlacesUtils.bookmarks.insertTree(tree);
+ await PlacesUtils.bookmarks.insertTree(tree, { fixupOrSkipInvalidEntries: true });
insertFaviconsForTree(tree);
}
},
/**
* Imports data into the places database from the supplied url.
*
* @param {String} href The url to import data from.
--- a/toolkit/components/places/BookmarkJSONUtils.jsm
+++ b/toolkit/components/places/BookmarkJSONUtils.jsm
@@ -296,17 +296,17 @@ BookmarkImporter.prototype = {
// Places is moving away from supporting user-defined folders at the top
// of the tree, however, until we have a migration strategy we need to
// ensure any non-built-in folders are created (xref bug 1310299).
if (!PlacesUtils.bookmarks.userContentRoots.includes(node.guid)) {
node.parentGuid = PlacesUtils.bookmarks.rootGuid;
await PlacesUtils.bookmarks.insert(node);
}
- await PlacesUtils.bookmarks.insertTree(node);
+ await PlacesUtils.bookmarks.insertTree(node, { fixupOrSkipInvalidEntries: true });
// Now add any favicons.
try {
insertFaviconsForTree(node);
} catch (ex) {
Cu.reportError(`Failed to insert favicons: ${ex}`);
}
}
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -266,47 +266,54 @@ var Bookmarks = Object.freeze({
*
* Children will be appended to any existing children of the parent
* that is specified. The source specified on the root of the tree
* will be used for all the items inserted. Any indices or custom parentGuids
* set on children will be ignored and overwritten.
*
* @param {Object} tree
* object representing a tree of bookmark items to insert.
+ * @param {Object} options [optional]
+ * object with properties representing options. Current options are:
+ * - fixupOrSkipInvalidEntries: makes the insert more lenient to
+ * mistakes in the input tree. Properties of an entry that are
+ * fixable will be corrected, otherwise the entry will be skipped.
+ * This is particularly convenient for import/restore operations,
+ * but should not be abused for common inserts, since it may hide
+ * bugs in the calling code.
*
* @return {Promise} resolved when the creation is complete.
* @resolves to an object representing the created bookmark.
* @rejects if it's not possible to create the requested bookmark.
* @throws if the arguments are invalid.
*/
- insertTree(tree) {
+ insertTree(tree, options) {
if (!tree || typeof tree != "object") {
throw new Error("Should be provided a valid tree object.");
}
-
if (!Array.isArray(tree.children) || !tree.children.length) {
throw new Error("Should have a non-zero number of children to insert.");
}
-
if (!PlacesUtils.isValidGuid(tree.guid)) {
throw new Error(`The parent guid is not valid (${tree.guid} ${tree.title}).`);
}
-
if (tree.guid == this.rootGuid) {
throw new Error("Can't insert into the root.");
}
-
if (tree.guid == this.tagsGuid) {
throw new Error("Can't use insertTree to insert tags.");
}
-
if (tree.hasOwnProperty("source") &&
!Object.values(this.SOURCES).includes(tree.source)) {
throw new Error("Can't use source value " + tree.source);
}
+ if (options && typeof options != "object") {
+ throw new Error("Options should be a valid object");
+ }
+ let fixupOrSkipInvalidEntries = options && !!options.fixupOrSkipInvalidEntries;
// Serialize the tree into an array of items to insert into the db.
let insertInfos = [];
let insertLivemarkInfos = [];
let urlsThatMightNeedPlaces = [];
// We want to use the same 'last added' time for all the entries
// we import (so they won't differ by a few ms based on where
@@ -314,16 +321,17 @@ var Bookmarks = Object.freeze({
// multiple dates).
let fallbackLastAdded = new Date();
const {TYPE_BOOKMARK, TYPE_FOLDER, SOURCES} = this;
// Reuse the 'source' property for all the entries.
let source = tree.source || SOURCES.DEFAULT;
+ // This is recursive.
function appendInsertionInfoForInfoArray(infos, indexToUse, parentGuid) {
// We want to keep the index of items that will be inserted into the root
// NULL, and then use a subquery to select the right index, to avoid
// races where other consumers might add items between when we determine
// the index and when we insert. However, the validator does not allow
// NULL values in in the index, so we fake it while validating and then
// correct later. Keep track of whether we're doing this:
let shouldUseNullIndices = false;
@@ -334,50 +342,59 @@ var Bookmarks = Object.freeze({
// When a folder gets an item added, its last modified date is updated
// to be equal to the date we added the item (if that date is newer).
// Because we're inserting a tree, we keep track of this date for the
// loop, updating it for inserted items as well as from any subfolders
// we insert.
let lastAddedForParent = new Date(0);
for (let info of infos) {
- // Add a guid if none is present.
- if (!info.hasOwnProperty("guid")) {
- info.guid = PlacesUtils.history.makeGuid();
- }
- // Set the correct parent guid.
- info.parentGuid = parentGuid;
// Ensure to use the same date for dateAdded and lastModified, even if
// dateAdded may be imposed by the caller.
let time = (info && info.dateAdded) || fallbackLastAdded;
- let insertInfo = validateBookmarkObject("Bookmarks.jsm: insertTree",
- info, {
+ let insertInfo = {
+ guid: { defaultValue: PlacesUtils.history.makeGuid() },
type: { defaultValue: TYPE_BOOKMARK },
url: { requiredIf: b => b.type == TYPE_BOOKMARK,
validIf: b => b.type == TYPE_BOOKMARK },
- parentGuid: { required: true },
+ parentGuid: { replaceWith: parentGuid }, // Set the correct parent guid.
title: { defaultValue: "",
validIf: b => b.type == TYPE_BOOKMARK ||
b.type == TYPE_FOLDER ||
b.title === "" },
dateAdded: { defaultValue: time,
validIf: b => !b.lastModified ||
- b.dateAdded <= b.lastModified },
+ b.dateAdded <= b.lastModified },
lastModified: { defaultValue: time,
validIf: b => (!b.dateAdded && b.lastModified >= time) ||
(b.dateAdded && b.lastModified >= b.dateAdded) },
index: { replaceWith: indexToUse++ },
source: { replaceWith: source },
annos: {},
keyword: { validIf: b => b.type == TYPE_BOOKMARK },
charset: { validIf: b => b.type == TYPE_BOOKMARK },
postData: { validIf: b => b.type == TYPE_BOOKMARK },
tags: { validIf: b => b.type == TYPE_BOOKMARK },
children: { validIf: b => b.type == TYPE_FOLDER && Array.isArray(b.children) }
- });
+ };
+ if (fixupOrSkipInvalidEntries) {
+ insertInfo.guid.fixup = b => b.guid = PlacesUtils.history.makeGuid();
+ insertInfo.dateAdded.fixup = insertInfo.lastModified.fixup =
+ b => b.lastModified = b.dateAdded = fallbackLastAdded;
+ }
+ try {
+ insertInfo = validateBookmarkObject("Bookmarks.jsm: insertTree", info, insertInfo);
+ } catch (ex) {
+ if (fixupOrSkipInvalidEntries) {
+ indexToUse--;
+ continue;
+ } else {
+ throw ex;
+ }
+ }
if (shouldUseNullIndices) {
insertInfo.index = null;
}
// Store the URL if this is a bookmark, so we can ensure we create an
// entry in moz_places for it.
if (insertInfo.type == Bookmarks.TYPE_BOOKMARK) {
urlsThatMightNeedPlaces.push(insertInfo.url);
@@ -434,17 +451,34 @@ var Bookmarks = Object.freeze({
if (treeParent._parentId == PlacesUtils.tagsFolderId) {
throw new Error("Can't use insertTree to insert tags.");
}
await insertBookmarkTree(insertInfos, source, treeParent,
urlsThatMightNeedPlaces, lastAddedForParent);
- await insertLivemarkData(insertLivemarkInfos);
+ for (let info of insertLivemarkInfos) {
+ try {
+ await insertLivemarkData(info);
+ } catch (ex) {
+ // This can arguably fail, if some of the livemarks data is invalid.
+ if (fixupOrSkipInvalidEntries) {
+ // The placeholder should have been removed at this point, thus we
+ // can avoid to notify about it.
+ let placeholderIndex = insertInfos.findIndex(item => item.guid == info.guid);
+ if (placeholderIndex != -1) {
+ insertInfos.splice(placeholderIndex, 1);
+ }
+ } else {
+ // Throw if we're not lenient to input mistakes.
+ throw ex;
+ }
+ }
+ }
// Now update the indices of root items in the objects we return.
// These may be wrong if someone else modified the table between
// when we fetched the parent and inserted our items, but the actual
// inserts will have been correct, and we don't want to query the DB
// again if we don't have to. bug 1347230 covers improving this.
let rootIndex = treeParent._childCount;
for (let insertInfo of insertInfos) {
@@ -476,17 +510,23 @@ var Bookmarks = Object.freeze({
notify(observers, "onItemAdded", [ itemId, parentId, item.index,
item.type, uri, item.title,
PlacesUtils.toPRTime(item.dateAdded), item.guid,
item.parentGuid, item.source ],
{ isTagging: false });
// Note, annotations for livemark data are deleted from insertInfo
// within appendInsertionInfoForInfoArray, so we won't be duplicating
// the insertions here.
- await handleBookmarkItemSpecialData(itemId, item);
+ try {
+ await handleBookmarkItemSpecialData(itemId, item);
+ } catch (ex) {
+ // This is not critical, regardless the bookmark has been created
+ // and we should continue notifying the next ones.
+ Cu.reportError(`An error occured while handling special bookmark data: ${ex}`);
+ }
// Remove non-enumerable properties.
delete item.source;
insertInfos[i] = Object.assign({}, item);
}
return insertInfos;
})();
@@ -1456,112 +1496,114 @@ function insertBookmarkTree(items, sourc
// We don't wait for the frecency calculation.
updateFrecency(db, urls, true).catch(Cu.reportError);
return items;
});
}
/**
- * Handles any Livemarks within the passed items.
+ * Handles data for a Livemark insert.
*
- * @param {Array} items Livemark items that need to be added.
+ * @param {Object} item Livemark item that need to be added.
*/
-async function insertLivemarkData(items) {
- for (let item of items) {
- let feedURI = null;
- let siteURI = null;
- item.annos = item.annos.filter(function(aAnno) {
- switch (aAnno.name) {
- case PlacesUtils.LMANNO_FEEDURI:
- feedURI = NetUtil.newURI(aAnno.value);
- return false;
- case PlacesUtils.LMANNO_SITEURI:
- siteURI = NetUtil.newURI(aAnno.value);
- return false;
- default:
- return true;
- }
- });
-
- let index = null;
+async function insertLivemarkData(item) {
+ // Delete the placeholder but note the index of it, so that we can insert the
+ // livemark item at the right place.
+ let placeholder = await Bookmarks.fetch(item.guid);
+ let index = placeholder.index;
+ await removeBookmark(item, {source: item.source});
- // Delete the placeholder but note the index of it, so that we
- // can insert the livemark item at the right place.
- let placeholder = await Bookmarks.fetch(item.guid);
- index = placeholder.index;
-
- await Bookmarks.remove(item.guid, {source: item.source});
-
- if (feedURI) {
- item.feedURI = feedURI;
- item.siteURI = siteURI;
- item.index = index;
+ let feedURI = null;
+ let siteURI = null;
+ item.annos = item.annos.filter(function(aAnno) {
+ switch (aAnno.name) {
+ case PlacesUtils.LMANNO_FEEDURI:
+ feedURI = NetUtil.newURI(aAnno.value);
+ return false;
+ case PlacesUtils.LMANNO_SITEURI:
+ siteURI = NetUtil.newURI(aAnno.value);
+ return false;
+ default:
+ return true;
+ }
+ });
- if (item.dateAdded) {
- item.dateAdded = PlacesUtils.toPRTime(item.dateAdded);
- }
- if (item.lastModified) {
- item.lastModified = PlacesUtils.toPRTime(item.lastModified);
- }
+ if (feedURI) {
+ item.feedURI = feedURI;
+ item.siteURI = siteURI;
+ item.index = index;
- let livemark = await PlacesUtils.livemarks.addLivemark(item);
+ if (item.dateAdded) {
+ item.dateAdded = PlacesUtils.toPRTime(item.dateAdded);
+ }
+ if (item.lastModified) {
+ item.lastModified = PlacesUtils.toPRTime(item.lastModified);
+ }
- let id = livemark.id;
- if (item.annos && item.annos.length) {
- // Note: for annotations, we intentionally skip updating the last modified
- // value for the bookmark, to avoid a second update of the added bookmark.
- PlacesUtils.setAnnotationsForItem(id, item.annos,
- item.source, true);
- }
+ let livemark = await PlacesUtils.livemarks.addLivemark(item);
+
+ let id = livemark.id;
+ if (item.annos && item.annos.length) {
+ // Note: for annotations, we intentionally skip updating the last modified
+ // value for the bookmark, to avoid a second update of the added bookmark.
+ PlacesUtils.setAnnotationsForItem(id, item.annos, item.source, true);
}
}
}
/**
* Handles special data on a bookmark, e.g. annotations, keywords, tags, charsets,
* inserting the data into the appropriate place.
*
* @param {Integer} itemId The ID of the item within the bookmarks database.
* @param {Object} item The bookmark item with possible special data to be inserted.
*/
async function handleBookmarkItemSpecialData(itemId, item) {
if (item.annos && item.annos.length) {
// Note: for annotations, we intentionally skip updating the last modified
// value for the bookmark, to avoid a second update of the added bookmark.
- PlacesUtils.setAnnotationsForItem(itemId, item.annos, item.source, true);
+ try {
+ PlacesUtils.setAnnotationsForItem(itemId, item.annos, item.source, true);
+ } catch (ex) {
+ Cu.reportError(`Failed to insert annotations for item: ${ex}`);
+ }
}
if ("keyword" in item && item.keyword) {
// POST data could be set in 2 ways:
// 1. new backups have a postData property
// 2. old backups have an item annotation
let postDataAnno = item.annos &&
item.annos.find(anno => anno.name == PlacesUtils.POST_DATA_ANNO);
let postData = item.postData || (postDataAnno && postDataAnno.value);
try {
await PlacesUtils.keywords.insert({
keyword: item.keyword,
url: item.url,
postData,
source: item.source
});
} catch (ex) {
- Cu.reportError(`Failed to insert keywords: ${ex}`);
+ Cu.reportError(`Failed to insert keyword "${item.keyword} for ${item.url}": ${ex}`);
}
}
if ("tags" in item) {
try {
PlacesUtils.tagging.tagURI(NetUtil.newURI(item.url), item.tags, item.source);
} catch (ex) {
// Invalid tag child, skip it.
Cu.reportError(`Unable to set tags "${item.tags.join(", ")}" for ${item.url}: ${ex}`);
}
}
if ("charset" in item && item.charset) {
- await PlacesUtils.setCharsetForURI(NetUtil.newURI(item.url), item.charset);
+ try {
+ await PlacesUtils.setCharsetForURI(NetUtil.newURI(item.url), item.charset);
+ } catch (ex) {
+ Cu.reportError(`Failed to set charset "${item.charset}" for ${item.url}: ${ex}`);
+ }
}
}
// Query implementation.
async function queryBookmarks(info) {
let queryParams = {
tags_folder: await promiseTagsFolderId(),
--- a/toolkit/components/places/PlacesTransactions.jsm
+++ b/toolkit/components/places/PlacesTransactions.jsm
@@ -1124,26 +1124,32 @@ PT.NewBookmark.prototype = Object.seal({
*/
PT.NewFolder = DefineTransaction(["parentGuid", "title"],
["index", "annotations", "children"]);
PT.NewFolder.prototype = Object.seal({
async execute({ parentGuid, title, index, annotations, children }) {
let folderGuid;
let info = {
children: [{
+ // Ensure to specify a guid to be restored on redo.
+ guid: PlacesUtils.history.makeGuid(),
title,
type: PlacesUtils.bookmarks.TYPE_FOLDER,
}],
// insertTree uses guid as the parent for where it is being inserted
// into.
guid: parentGuid,
};
if (children && children.length > 0) {
- info.children[0].children = children;
+ // Ensure to specify a guid for each child to be restored on redo.
+ info.children[0].children = children.map(c => {
+ c.guid = PlacesUtils.history.makeGuid();
+ return c;
+ });
}
async function createItem() {
// Note, insertTree returns an array, rather than the folder/child structure.
// For simplicity, we only get the new folder id here. This means that
// an undo then redo won't retain exactly the same information for all
// the child bookmarks, but we believe that isn't important at the moment.
let bmInfo = await PlacesUtils.bookmarks.insertTree(info);
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -532,16 +532,18 @@ this.PlacesUtils = {
* - required: this property is required.
* - replaceWith: this property will be overwritten with the value
* provided
* - requiredIf: if the provided condition is satisfied, then this
* property is required.
* - validIf: if the provided condition is not satisfied, then this
* property is invalid.
* - defaultValue: an undefined property should default to this value.
+ * - fixup: a function invoked when validation fails, takes the input
+ * object as argument and must fix the property.
*
* @return a validated and normalized item.
* @throws if the object contains invalid data.
* @note any unknown properties are pass-through.
*/
validateItemProperties(name, validators, props, behavior = {}) {
if (!props)
throw new Error(`${name}: Input should be a valid object`);
@@ -554,17 +556,21 @@ this.PlacesUtils = {
if (behavior[prop].hasOwnProperty("required") && behavior[prop].required) {
required.add(prop);
}
if (behavior[prop].hasOwnProperty("requiredIf") && behavior[prop].requiredIf(input)) {
required.add(prop);
}
if (behavior[prop].hasOwnProperty("validIf") && input[prop] !== undefined &&
!behavior[prop].validIf(input)) {
- throw new Error(`${name}: Invalid value for property '${prop}': ${JSON.stringify(input[prop])}`);
+ if (behavior[prop].hasOwnProperty("fixup")) {
+ behavior[prop].fixup(input);
+ } else {
+ throw new Error(`${name}: Invalid value for property '${prop}': ${JSON.stringify(input[prop])}`);
+ }
}
if (behavior[prop].hasOwnProperty("defaultValue") && input[prop] === undefined) {
input[prop] = behavior[prop].defaultValue;
}
if (behavior[prop].hasOwnProperty("replaceWith")) {
input[prop] = behavior[prop].replaceWith;
}
}
@@ -575,17 +581,22 @@ this.PlacesUtils = {
} else if (input[prop] === undefined) {
// Skip undefined properties that are not required.
continue;
}
if (validators.hasOwnProperty(prop)) {
try {
normalizedInput[prop] = validators[prop](input[prop], input);
} catch (ex) {
- throw new Error(`${name}: Invalid value for property '${prop}': ${JSON.stringify(input[prop])}`);
+ if (behavior.hasOwnProperty(prop) && behavior[prop].hasOwnProperty("fixup")) {
+ behavior[prop].fixup(input);
+ normalizedInput[prop] = input[prop];
+ } else {
+ throw new Error(`${name}: Invalid value for property '${prop}': ${JSON.stringify(input[prop])}`);
+ }
}
}
}
if (required.size > 0)
throw new Error(`${name}: The following properties were expected: ${[...required].join(", ")}`);
return normalizedInput;
},
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/bookmarks/test_insertTree_fixupOrSkipInvalidEntries.js
@@ -0,0 +1,90 @@
+function insertTree(tree) {
+ return PlacesUtils.bookmarks.insertTree(tree, { fixupOrSkipInvalidEntries: true });
+}
+
+add_task(async function() {
+ let guid = PlacesUtils.bookmarks.unfiledGuid;
+ await Assert.throws(() => insertTree({guid, children: []}),
+ /Should have a non-zero number of children to insert./);
+ await Assert.throws(() => insertTree({guid: "invalid", children: [{}]}),
+ /The parent guid is not valid/);
+
+ let now = new Date();
+ let url = "http://mozilla.com/";
+ let obs = {
+ count: 0,
+ lastIndex: 0,
+ onItemAdded(itemId, parentId, index, type, uri, title, dateAdded, itemGuid, parentGuid) {
+ this.count++;
+ let lastIndex = this.lastIndex;
+ this.lastIndex = index;
+ if (type == PlacesUtils.bookmarks.TYPE_BOOKMARK) {
+ Assert.equal(uri.spec, url, "Found the expected url");
+ }
+ Assert.ok(index == 0 || index == lastIndex + 1, "Consecutive indices");
+ Assert.ok(dateAdded >= PlacesUtils.toPRTime(now), "Found a valid dateAdded");
+ Assert.ok(PlacesUtils.isValidGuid(itemGuid), "guid is valid");
+ },
+ };
+ PlacesUtils.bookmarks.addObserver(obs);
+
+ let tree = {
+ guid,
+ children: [
+ { // Should be inserted, and the invalid guid should be replaced.
+ guid: "test",
+ url,
+ },
+ { // Should be skipped, since the url is invalid.
+ url: "fake_url",
+ },
+ { // Should be skipped, since the type is invalid.
+ url,
+ type: 999,
+ },
+ { // Should be skipped, since the type is invalid.
+ type: 999,
+ children: [
+ {
+ url,
+ },
+ ]
+ },
+ {
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "test",
+ children: [
+ { // Should fix lastModified and dateAdded.
+ url,
+ lastModified: null
+ },
+ { // Should be skipped, since the url is invalid.
+ url: "fake_url",
+ dateAdded: null,
+ },
+ { // Should fix lastModified and dateAdded.
+ url,
+ dateAdded: undefined
+ },
+ { // Should be skipped since it's a separator with a url
+ url,
+ type: PlacesUtils.bookmarks.TYPE_SEPARATOR,
+ },
+ { // Should fix lastModified and dateAdded.
+ url,
+ dateAdded: new Date(now - 86400000),
+ lastModified: new Date(now - 172800000) // less than dateAdded
+ },
+ ]
+ }
+ ]
+ };
+
+ let bms = await insertTree(tree);
+ for (let bm of bms) {
+ checkBookmarkObject(bm);
+ }
+ Assert.equal(bms.length, 5);
+ Assert.equal(obs.count, bms.length);
+});
+
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -32,15 +32,16 @@ skip-if = toolkit == 'android'
[test_bookmarks_getRecent.js]
[test_bookmarks_insert.js]
[test_bookmarks_insertTree.js]
[test_bookmarks_notifications.js]
[test_bookmarks_remove.js]
[test_bookmarks_reorder.js]
[test_bookmarks_search.js]
[test_bookmarks_update.js]
+[test_insertTree_fixupOrSkipInvalidEntries.js]
[test_keywords.js]
[test_nsINavBookmarkObserver.js]
[test_protectRoots.js]
[test_removeFolderTransaction_reinsert.js]
[test_savedsearches.js]
[test_sync_fields.js]
[test_untitled.js]
