diff --git a/toolkit/components/places/BookmarkHTMLUtils.jsm b/toolkit/components/places/BookmarkHTMLUtils.jsm index 696b50062546..ff61c08e1b6e 100644 --- a/toolkit/components/places/BookmarkHTMLUtils.jsm +++ b/toolkit/components/places/BookmarkHTMLUtils.jsm @@ -920,7 +920,7 @@ BookmarkImporter.prototype = { // Give the tree the source. tree.source = this._source; - await PlacesUtils.bookmarks.insertTree(tree); + await PlacesUtils.bookmarks.insertTree(tree, { fixupOrSkipInvalidEntries: true }); insertFaviconsForTree(tree); } }, diff --git a/toolkit/components/places/BookmarkJSONUtils.jsm b/toolkit/components/places/BookmarkJSONUtils.jsm index 045d898eac78..34dca9ac6a30 100644 --- a/toolkit/components/places/BookmarkJSONUtils.jsm +++ b/toolkit/components/places/BookmarkJSONUtils.jsm @@ -301,7 +301,7 @@ BookmarkImporter.prototype = { await PlacesUtils.bookmarks.insert(node); } - await PlacesUtils.bookmarks.insertTree(node); + await PlacesUtils.bookmarks.insertTree(node, { fixupOrSkipInvalidEntries: true }); // Now add any favicons. try { diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index 3eb6cdc4601b..dd4ed0aa20b5 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -271,37 +271,44 @@ var Bookmarks = Object.freeze({ * * @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 = []; @@ -319,6 +326,7 @@ var Bookmarks = Object.freeze({ // 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 @@ -339,28 +347,22 @@ var Bookmarks = Object.freeze({ // 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) }, @@ -372,7 +374,22 @@ var Bookmarks = Object.freeze({ 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; @@ -439,7 +456,24 @@ var Bookmarks = Object.freeze({ 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 @@ -481,7 +515,13 @@ var Bookmarks = Object.freeze({ // 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; @@ -1461,57 +1501,51 @@ function insertBookmarkTree(items, source, parent, urls, lastAddedForParent) { } /** - * 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; - } - }); +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}); - let index = null; + 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; + } + }); - // 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; + if (feedURI) { + item.feedURI = feedURI; + item.siteURI = siteURI; + item.index = index; - await Bookmarks.remove(item.guid, {source: item.source}); + 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 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); - } + 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); } } } @@ -1527,7 +1561,11 @@ 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: @@ -1544,7 +1582,7 @@ async function handleBookmarkItemSpecialData(itemId, item) { 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) { @@ -1556,7 +1594,11 @@ async function handleBookmarkItemSpecialData(itemId, item) { } } 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}`); + } } } diff --git a/toolkit/components/places/PlacesTransactions.jsm b/toolkit/components/places/PlacesTransactions.jsm index 93cda3c24732..39eff30477ac 100644 --- a/toolkit/components/places/PlacesTransactions.jsm +++ b/toolkit/components/places/PlacesTransactions.jsm @@ -1129,6 +1129,8 @@ PT.NewFolder.prototype = Object.seal({ 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, }], @@ -1138,7 +1140,11 @@ PT.NewFolder.prototype = Object.seal({ }; 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() { diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index 6f847c34c964..a9711de35054 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -537,6 +537,8 @@ this.PlacesUtils = { * - 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. @@ -559,7 +561,11 @@ this.PlacesUtils = { } 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; @@ -580,7 +586,12 @@ this.PlacesUtils = { 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])}`); + } } } } diff --git a/toolkit/components/places/tests/bookmarks/test_insertTree_fixupOrSkipInvalidEntries.js b/toolkit/components/places/tests/bookmarks/test_insertTree_fixupOrSkipInvalidEntries.js new file mode 100644 index 000000000000..229a7c66a71f --- /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); +}); + diff --git a/toolkit/components/places/tests/bookmarks/xpcshell.ini b/toolkit/components/places/tests/bookmarks/xpcshell.ini index 16f9de3ea0b2..7053d4a2eaae 100644 --- a/toolkit/components/places/tests/bookmarks/xpcshell.ini +++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini @@ -37,6 +37,7 @@ skip-if = toolkit == 'android' [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]