BookmarkJSONUtils mostly, there could be some related code around should be fake-async already.

Here we must replace nsINavBookmarks API calls with Bookmarks.jsm calls

let's start with the part that does the Backup, I have some fears regarding the Restore part, cause in the past, changing from sync to async made us take many minutes for an import that originally was taking a few seconds. So likely I'd split the restore part to a separate bug where we can also verify the performance problem.

ugh, ok nevermind, I just verified that the backup is async already! So, let's do the conversion, but we need performance testing before pushing it.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e2fd57260ee

note: I'm basically rewriting toolkit/components/places/tests/unit/test_384370.js test_bookmarks_html.js and test_bookmarks_json.js in bug 1148466. so don't worry too much about those failures for now. I'll also have to slightly touch (minor changes I think) HTMLUtils and JSONUtils there. I need that bug in aurora, so it will have to land before these refactorings.

Comment on attachment 8587605 [details] [diff] [review] Patch - Convert JSON bookmarks backup to Bookmarks.jsm API Review of attachment 8587605 [details] [diff] [review]: ----------------------------------------------------------------- Now that bug 1148466 is fixed, this requires some unbitrotting, on the positive side we should not touch this code anymore, so it should not bitrot again. ::: toolkit/components/places/Bookmarks.jsm @@ +428,5 @@ > + * @resolves to an object representing the emptied bookmark folder. > + * @rejects if the provided guid doesn't match any existing bookmark folder. > + * @throws if the arguments are invalid. > + */ > + empty(guidOrInfo) { We decided to not implement an equivalent of the old removeFolderChildren API I think it should be possible to use eraseEverything() when we are requested to restore a json. maybe that requires some refactoring? I don't think there's a case where we don't need to remove all the roots. If it's a problem please let me know and I can better look into that.

(In reply to Marco Bonardo [::mak] from comment #7) > We decided to not implement an equivalent of the old removeFolderChildren API > > I think it should be possible to use eraseEverything() when we are requested > to restore a json. maybe that requires some refactoring? I don't think > there's a case where we don't need to remove all the roots. > If it's a problem please let me know and I can better look into that. The issue I hit with eraseEverything is that it doesn't take into account the EXCLUDE_FROM_BACKUP annotation and erases too much. I thought about introducing an option to the function, but decided to go with adding a new empty method instead. How do you think we should move forward here?

what eraseEverything does, is removing children of unfiled/menu/toolbar. Tags should be removed as a consequence, provided the tagging service is alive and listening (bug 820876 will remove that limitation). What importFromJSON does today is getting all children of placesRootId, that is: unfiled/menu/toolbar/tags/excluded_folders/3rd_party_folders It excludes excluded_folders and tags from this list. What is left is basically unfiled/menu/toolbar/3rd_party_folders then it goes through these, if they are roots it invokes removeFolderChildren, otherwise it removes the whole folder. Now, as you can see, by using eraseEverything you are done with the roots, what is left is those 3rd party folders that add-ons didn't mark as EXCLUDE_FROM_BACKUP, and we might end up creating a dupe of them. So, I'd use eraseEverything, and then we could add some special code (even an async query in the worst case) just to fetch and remove those 3rd party folders. For example this should give you a tree of these items PlacesUtils.promiseBookmarksTree(undefined, {includeItemIds: true, excludeItemsCallback: item => item.root}}); from the tree you only need to check EXCLUDE from the first level of children

or you could use promiseItemGuid to convert the excluded item ids to guids, and make excludeItemsCallback exclude both roots and items having guids in this list of excluded guids.

hm, now that I rethink about it, promiseBookmarksTree might be a little bit too expensive for this thing cause it won't stop at the first level, so let's just do a custom query using PlacesUtils.promiseDBConnection(), I can probably make the query for you. This small piace of code should not block the conversion, it's just a small detail in the global plot.

Hi Steven, I'd like to know if you have any news about these conversions. That also includes bad news, so no worries! We are rebalancing the work to bring it cose to completion, so if you think you might not have time for both bugs or having problems with the code, please let me know and I could probably steal something in the next week, or help with specific parts.

(In reply to Marco Bonardo [::mak] from comment #12) > Hi Steven, I'd like to know if you have any news about these conversions. > That also includes bad news, so no worries! > We are rebalancing the work to bring it cose to completion, so if you think > you might not have time for both bugs or having problems with the code, > please let me know and I could probably steal something in the next week, or > help with specific parts. Hi Marco, I haven't had much time to finish these up - it'd be great if you could steal Bug 1095427 as I've made much less progress there. That might help me in finishing off this as well if that functionality requires some of the queries mentioned above that I require.

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. Marco, this is still highly WIP, but if you could provide feedback on the approach, that would be very useful. Thanks.

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. https://reviewboard.mozilla.org/r/138744/#review142438 The approach looks ok afaict, there is that detail about livemarks to be figured out, for which I'm not yet sure what makes more sense, it depends on how hard is to support those with the current status of insertTree, if it's a challenge, then we may need to change insertTree ::: toolkit/components/places/BookmarkJSONUtils.jsm:678 (Diff revision 1) > + node.dateAdded = new Date(parseInt(node.dateAdded / 1000)); > + } > + > + if (node.lastModified) { > + node.lastModified = new Date(parseInt(node.lastModified / 1000)); > + } probably worth checking that lastModified >= dateAdded, or ignore the lastModified value. The bookmarks API doesn't like bogus values

The new patch I've just uploaded is still WIP, however, I believe it is most of the way there. Things to still address: - Handle importing files with multiple folder specifications (fix test_import_mobile_bookmarks.js). - Work out what is going wrong with restoring queries and fix it (test_405938_restore_queries.js). - Tidy up the code & clean up debugging info. - Final testing & validation.

The change I just uploaded fixes a minor issue in insertTree where it was sending out the wrong parentIds for nested folders which was confusing the GuidHelper cache in PlacesUtils (used for promiseItemId etc). This goes part way to fixing test_import_mobile_bookmarks.js. I should be able to fix the rest tomorrow.

I've fixed the remaining known issues with the patch, and done the tidying up that I was going to do. I have just requested try builds to pick up any other test failures I might not have noticed. I'll take a look through it again on Monday, however for now, I think it is probably ready for a first round of review.

Shouldn't this: node.dateAdded = new Date(parseInt(node.dateAdded / 1000)); be: node.dateAdded = new Date(parseInt(node.dateAdded) / 1000); ?

(In reply to Josh Aas from comment #24) > Shouldn't this: > > node.dateAdded = new Date(parseInt(node.dateAdded / 1000)); > > be: > > node.dateAdded = new Date(parseInt(node.dateAdded) / 1000); > > ? Yes, thanks for spotting that. I'd copied it from the original patch without thinking about it.

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. https://reviewboard.mozilla.org/r/138744/#review145144 I didn't have enough time go through BookmarkJSONUtils yet, but I first wanted to have a quick overhead of PlacesUtils and Bookmarks changes, and I think those should be addressed first. So trying to move this on faster, I'll start by posting those comments. ::: toolkit/components/places/Bookmarks.jsm:157 (Diff revision 6) > > - /** > + /** > + * The GUIDs of the core top-level folders that we support, for easy access > + * as a set. > + */ > + coreFolders: ["toolbar_____", "menu________", "unfiled_____", "mobile______"], I'm not extremely happy to expose this here but... I guess I can take the hit. We could name this userContentRoots maybe? Since the other roots are not made to contain user content. Also, can we reuse the previous defined properties here, so the guid values are only set in one place? ::: toolkit/components/places/Bookmarks.jsm:277 (Diff revision 6) > - * @param tree > + * @param {Object} tree > * object representing a tree of bookmark items to insert. > + * @param {Object} [options={}] > + * Additional options that can be passed to the function. > + * Currently supports the following properties: > + * - useSuppliedIndexes: Forces insertTree to use supplied indexes. I'd prefer if we could avoid this, because consumers will start passing this AND completely bogus indices. We can't have bogus indices in the db for various perf and coherency reasons, so the API is built in a way to avoid being a footgun. insertTree explicitly ignores passed in indices, it will always append stuff in the order that is hardcoded in the children arrays. It's up to the caller to properly sort the array before the insertion call. ::: toolkit/components/places/Bookmarks.jsm:351 (Diff revision 6) > let lastAddedForParent = new Date(0); > + // As we import Livemarks at a later stage to the main bookmarks, but > + // we also want to preserve indexes, then we need to adjust the indexes, > + // so that when the Livemarks are inserted, the bookmarks will get the > + // correct indexes. > + let livemarkIndexAdjustment = 0; see later comment in PlacesUtils, to make it short I'd prefer not doing this. ::: toolkit/components/places/Bookmarks.jsm:378 (Diff revision 6) > validIf: b => (!b.dateAdded && b.lastModified >= time) || > (b.dateAdded && b.lastModified >= b.dateAdded) } > - , index: { replaceWith: indexToUse++ } > + , index: options.useSuppliedIndexes ? { adjustBy: livemarkIndexAdjustment } : { replaceWith: indexToUse++ } > , source: { replaceWith: source } > + , annos: { validIf: b => [ TYPE_BOOKMARK > + , TYPE_FOLDER ].includes(b.type) && Array.isArray(b.annos) } shouldn't the array check happen in the validator rather than here? also, IIRC, any bookmark can have annotations, included separators. ::: toolkit/components/places/Bookmarks.jsm:747 (Diff revision 6) > - eraseEverything(options = {}) { > - const folderGuids = [this.toolbarGuid, this.menuGuid, this.unfiledGuid, > - this.mobileGuid]; > + async eraseEverything(options = {}) { > + if (!("source" in options)) { > + options.source = this.SOURCES.DEFAULT; > + } > + > + if (options.reallyEraseEverything) { oook, I don't like this here because it's all stuff that should be removed once we limit roots creation to internal code... Can we do this in an external method, maybe an earseEverything wrapper in PlacesBackups.jsm (if it should be shared with BookmarkHTMLUtils). ::: toolkit/components/places/Bookmarks.jsm:1425 (Diff revision 6) > + * added. > + */ > +async function maybeHandleLivemarkItemData(items) { > + for (let item of items) { > + if (!isLivemark(item)) { > + continue; Can we store livemarks in a separate array while we walk the array, and pass that separate array here? Since we're speaking about an array with potentially thousands entries, that seems more performant. ::: toolkit/components/places/Bookmarks.jsm:1453 (Diff revision 6) > + parentGuid: item.parentGuid, > + lastModified: item.lastModified, > + siteURI, > + guid: item.guid, > + source: item.source > + }; is there a reason to not just add feedURI and siteURI to item, instead of copying all of item into this new object? ::: toolkit/components/places/Bookmarks.jsm:1463 (Diff revision 6) > + let livemark = await PlacesUtils.livemarks.addLivemark(livemarkInfo); > + > + let id = livemark.id; > + if (item.dateAdded) > + PlacesUtils.bookmarks.setItemDateAdded(id, item.dateAdded, > + item.source); addLivemark accepts and sets a dateAdded already, afaict ::: toolkit/components/places/Bookmarks.jsm:1480 (Diff revision 6) > + * @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.type == Bookmarks.TYPE_FOLDER || item.type == Bookmarks.TYPE_BOOKMARK)) { why not separators? ::: toolkit/components/places/Bookmarks.jsm:1510 (Diff revision 6) > + Cu.reportError(`Unable to set tags "${tags.join(", ")}" for ${item.url}: ${ex}`); > + } > + } > + } > + if ("charset" in item && item.charset) { > + PlacesUtils.annotations.setPageAnnotation( PlacesUtils.setCharsetForURI ::: toolkit/components/places/PlacesUtils.jsm:252 (Diff revision 6) > return new URL(v.spec); > return v; > }, > source: simpleValidateFunc(v => Number.isInteger(v) && > Object.values(PlacesUtils.bookmarks.SOURCES).includes(v)), > + annos: simpleValidateFunc(v => (typeof(v) == "object")), object is a bit generic, null is an object. And looks like this is a non-empty Array instead? ::: toolkit/components/places/PlacesUtils.jsm:255 (Diff revision 6) > source: simpleValidateFunc(v => Number.isInteger(v) && > Object.values(PlacesUtils.bookmarks.SOURCES).includes(v)), > + annos: simpleValidateFunc(v => (typeof(v) == "object")), > + keyword: simpleValidateFunc(v => (typeof(v) == "string")), > + charset: simpleValidateFunc(v => (typeof(v) == "string")), > + postData: simpleValidateFunc(v => (typeof(v) == "string")), keyword, charset and postData should all be non-empty if passed-in. ::: toolkit/components/places/PlacesUtils.jsm:256 (Diff revision 6) > Object.values(PlacesUtils.bookmarks.SOURCES).includes(v)), > + annos: simpleValidateFunc(v => (typeof(v) == "object")), > + keyword: simpleValidateFunc(v => (typeof(v) == "string")), > + charset: simpleValidateFunc(v => (typeof(v) == "string")), > + postData: simpleValidateFunc(v => (typeof(v) == "string")), > + tags: simpleValidateFunc(v => (typeof(v) == "string")) I'm not sure whether this should be a string, or an array of strings. Off-hand I'd say the latter, because we didn't solve some longstanding issues with the tags separator (currently comma). How hard would it be to make this a non-empty array? ::: toolkit/components/places/PlacesUtils.jsm:535 (Diff revision 6) > * - 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. > + * - adjustBy: if the value exists, then it is adjusted by this amount. As discussed over irc, I'd prefer if there would be no need for this, because people will start passing in this with broken or bogus values and I don't want to complicate the bookmarks API for it. in case of livemarks, instead of storing an adjust index I'd suggest to skip them AND not increase indexToUse, set the livemark index property to the index it should have at the end insteaf. Then later when the livemark will be inserted at the given index it will automatically bump the index of the items after itself. ::: toolkit/components/places/PlacesUtils.jsm:2634 (Diff revision 6) > updateCache(aItemId, aGuid) { > - if (typeof(aItemId) != "number" || aItemId <= 0) > + if (typeof(aItemId) != "number" || aItemId <= 0) { > throw new Error("Trying to update the GUIDs cache with an invalid itemId"); > + } > if (typeof(aGuid) != "string" || !/^[a-zA-Z0-9\-_]{12}$/.test(aGuid)) > throw new Error("Trying to update the GUIDs cache with an invalid GUID"); For consistency, either brace both, or leave it alone. ::: toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js:151 (Diff revision 6) > > aNode.QueryInterface(Ci.nsINavHistoryContainerResultNode); > aNode.containerOpen = true; > do_check_eq(aNode.childCount, 1); > var child = aNode.getChild(0); > - do_check_true(uri(child.uri).equals(uri("http://0"))) > + nit: is this additional new line a leftover? ::: toolkit/components/places/tests/bookmarks/test_417228-exclude-from-backup.js:119 (Diff revision 6) > +add_task(async function test_export_import_excluded_file() { > // populate db > test.populate(); > > await BookmarkJSONUtils.exportToFile(jsonFile); > + await PlacesUtils.bookmarks.eraseEverything(); this eraseEverything should probably not be here, it's shadowing the next replace=true call.

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. https://reviewboard.mozilla.org/r/138744/#review145144 > I'm not extremely happy to expose this here but... I guess I can take the hit. We could name this userContentRoots maybe? Since the other roots are not made to contain user content. > > Also, can we reuse the previous defined properties here, so the guid values are only set in one place? I've changed it to userContentRoots. Unfortunately we can't use the previous defined properties, as we're within an object, and changing it to use a self-replacing getter doesn't work as Bookmarks is frozen. The only other option I see is to put it in PlacesUtils maybe. > addLivemark accepts and sets a dateAdded already, afaict Thanks, I fixed this and also added an extra test to check the dates were imported correctly (as they weren't due to PRTime vs Date). > why not separators? I think I must have read the original code wrong.

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. https://reviewboard.mozilla.org/r/138744/#review151216 ::: toolkit/components/places/BookmarkJSONUtils.jsm:247 (Diff revision 7) > > /** > * Import bookmarks from a JSON string. > * > - * @param aString > - * JSON string of serialized bookmark data. > + * @param {String} aString JSON string of serialized bookmark data. > + * @returns {Promise} @return (no s) ::: toolkit/components/places/BookmarkJSONUtils.jsm:264 (Diff revision 7) > > - let batch = { > - nodes: nodes[0].children, > - runBatched: () => { > + // Change to nodes[0].children as we don't import the root, and also filter > + // out any obsolete "tagsFolder" sections. > + nodes = nodes[0].children.filter(node => !node.root || node.root != "tagsFolder"); > + > + // If we're replacing, then erase everything first. s/everything/existing bookmarks/ ::: toolkit/components/places/BookmarkJSONUtils.jsm:266 (Diff revision 7) > - nodes: nodes[0].children, > - runBatched: () => { > + // out any obsolete "tagsFolder" sections. > + nodes = nodes[0].children.filter(node => !node.root || node.root != "tagsFolder"); > + > + // If we're replacing, then erase everything first. > - if (this._replace) { > + if (this._replace) { > - // Get roots excluded from the backup, we will not remove them > + await eraseEverything({ source: this._source }); we should probably name this differently so it doesn't clash with the original API when searching the code, and move it to PlacesBackups.jsm so it can be reused by other importers if needed. ::: toolkit/components/places/BookmarkJSONUtils.jsm:274 (Diff revision 7) > + let folderIdMap = {}; > + let searchGuids = []; > + > + // Now do some cleanup on the imported nodes so that the various guids > + // match what we need for insertTree, and we also have mappings of folders > + // so we can repair any searches after inserting the bookmarks. let's append a " (See BUG 824502)." ::: toolkit/components/places/BookmarkJSONUtils.jsm:289 (Diff revision 7) > + > + folderIdMap = Object.assign(folderIdMap, folders); > + searchGuids = searchGuids.concat(searches); > - } > + } > + > + // Check for duplicates nodes and merge them together. Please expand this comment explaining that the previous code path may assign the same guid to multiple folders, and this is checking to merge those. ::: toolkit/components/places/BookmarkJSONUtils.jsm:322 (Diff revision 7) > + > + // Now we can add the actual nodes to the database. > + for (let node of nodes) { > + // 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. let's point this comment to bug 1310299 ::: toolkit/components/places/BookmarkJSONUtils.jsm:331 (Diff revision 7) > - } > + } > - } else { > - let [folders, searches] = this.importJSONNode( > - node, PlacesUtils.placesRootId, node.index, 0); > - for (let i = 0; i < folders.length; i++) { > - if (folders[i]) > + > + await PlacesUtils.bookmarks.insertTree(node, { > + // We only use the supplied indexes if we're replacing the entire tree. > + // Otherwise we let insertTree sort out the bookmarks as necessary. > + useSuppliedIndexes: this._replace is this still relevant? I can't find "useSuppliedIndexes" anywhere else ::: toolkit/components/places/BookmarkJSONUtils.jsm:335 (Diff revision 7) > - for (let i = 0; i < folders.length; i++) { > - if (folders[i]) > - folderIdMap[i] = folders[i]; > + // Otherwise we let insertTree sort out the bookmarks as necessary. > + useSuppliedIndexes: this._replace > + }); > + > + // Now add any favicons. > + insertFaviconsForTree(node); should probably try/catch, non-critical information. ::: toolkit/components/places/BookmarkJSONUtils.jsm:361 (Diff revision 7) > + * > + * @param {nsIURI} aQueryURI > + * A place: URI with folder ids. > + * @param {Object} aFolderIdMap > + * An array mapping old folder id to new folder ids. > + * @returns {String} the fixed up URI if all matched. If some matched, it returns @return ::: toolkit/components/places/BookmarkJSONUtils.jsm:375 (Diff revision 7) > + // we find the folder guids we need to know the ids for first, then > + // do the async request, and finally replace everything in one go. > + > + let uri = aQueryURI.href; > + let found = uri.match(reGlobal); > + let queryFolderIds = []; these are Guids, not Ids, right? please name the variables accordingly ::: toolkit/components/places/BookmarkJSONUtils.jsm:400 (Diff revision 7) > + "placesRoot": PlacesUtils.bookmarks.rootGuid, > + "bookmarksMenuFolder": PlacesUtils.bookmarks.menuGuid, > + "unfiledBookmarksFolder": PlacesUtils.bookmarks.unfiledGuid, > + "toolbarFolder": PlacesUtils.bookmarks.toolbarGuid, > + "mobileFolder": PlacesUtils.bookmarks.mobileGuid > - }; > +}; in the future we should probably replace those custom strings with the guids, since now they are static guids... btw not for now! ::: toolkit/components/places/BookmarkJSONUtils.jsm:474 (Diff revision 7) > + break; > - } > + } > - }); > > - if (feedURI) { > - let lmPromise = PlacesUtils.livemarks.addLivemark({ > + if (node.dateAdded) { > + node.dateAdded = new Date(parseInt(node.dateAdded) / 1000); PlacesUtils.toDate ::: toolkit/components/places/BookmarkJSONUtils.jsm:478 (Diff revision 7) > - }); > - this._importPromises.push(lmPromise); > - } > + } > + > + if (node.lastModified) { > + let lastModified = new Date(parseInt(node.lastModified) / 1000); ditto ::: toolkit/components/places/BookmarkJSONUtils.jsm:601 (Diff revision 7) > + > + let excludeItems = > + PlacesUtils.annotations.getItemsWithAnnotation(PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO); > + let rootFolder = await PlacesUtils.promiseBookmarksTree(this.rootGuid, { > + includeItemIds: true > + }); this is too expensive since it will build a complete tree of all the bookmarks. For now let's run a custom query selecting guids that have parentId = placesRootId, using promiseDBConnection, and add a todo comment pointing to bug 1310299. ::: toolkit/components/places/Bookmarks.jsm:366 (Diff revision 7) > , lastModified: { defaultValue: time, > validIf: b => (!b.dateAdded && b.lastModified >= time) || > (b.dateAdded && b.lastModified >= b.dateAdded) } > , index: { replaceWith: indexToUse++ } > , source: { replaceWith: source } > + , annos: { validIf: b => true } just don't pass a validIf? ::: toolkit/components/places/Bookmarks.jsm:481 (Diff revision 7) > item.parentGuid, item.source ], > { isTagging: false }); > // Remove non-enumerable properties. > delete item.source; > + > + await handleBookmarkItemSpecialData(itemId, item); it's unclear if we're going to set annos twice for livemarks since insertLivemarkData is also inserting other annotations? maybe it shouldn't? ::: toolkit/components/places/Bookmarks.jsm:1413 (Diff revision 7) > + * > + * @param {Array} items Livemark items that need to be added. > + */ > +async function insertLivemarkData(items) { > + for (let item of items) { > + // Node is a livemark this comment seems unhelpful considered the function name ::: toolkit/components/places/Bookmarks.jsm:1442 (Diff revision 7) > + > + if (feedURI) { > + item.feedURI = feedURI; > + item.siteURI = siteURI; > + item.index = index; > + if (item.index == null) { it's unclear how can this happen? ::: toolkit/components/places/Bookmarks.jsm:1487 (Diff revision 7) > + await PlacesUtils.keywords.insert({ > + keyword: item.keyword, > + url: item.url, > + postData, > + source: item.source > + }); we should probably try/catch and reportError around it, it's non-critical. ::: toolkit/components/places/PlacesUtils.jsm:1743 (Diff revision 7) > + /** > + * Get the item ids for multiple items (a bookmark, a folder or a separator) > + * given the unique ids for each item. > + * > + * @param {Array} aGuids An array of item GUIDs. > + * @return {PRomise} casing typo in PRomise ::: toolkit/components/places/tests/unit/test_bookmarks_json.js:77 (Diff revision 7) > { guid: "OCyeUO5uu9FR", > title: "Latest Headlines", > url: "http://en-us.fxfeeds.mozilla.com/en-US/firefox/livebookmarks/", > - feedUrl: "http://en-us.fxfeeds.mozilla.com/en-US/firefox/headlines.xml" > + feedUrl: "http://en-us.fxfeeds.mozilla.com/en-US/firefox/headlines.xml", > + // Note: date gets truncated to seconds, whereas the value in bookmarks.json > + // has full milliseconds. I think we truncate microseconds to milliseconds, and not milliseconds to seconds? this number looks like microseconds (PRTime) indeed.

Comment on attachment 8875318 [details] Bug 1095426 - Make the bookmarks.json test file prettified so it can be read easily. https://reviewboard.mozilla.org/r/146728/#review151320

Mark - can you confirm that your patch here does in fact significantly speed up large JSON imports? If so, what does the improvement look like, patched vs. unpatched build times?

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. https://reviewboard.mozilla.org/r/138744/#review151216 > it's unclear if we're going to set annos twice for livemarks since insertLivemarkData is also inserting other annotations? maybe it shouldn't? We won't since we're deleting them earlier, I've added a comment to clarify.

(In reply to Josh Aas from comment #33) > Mark - can you confirm that your patch here does in fact significantly speed > up large JSON imports? If so, what does the improvement look like, patched > vs. unpatched build times? Note that the scope here is not to have faster imports, it's to avoid main-thread IO in doing them. That said, numbers would be welcome, but I'd not expect those to be smaller.

I've just been fixing a couple of other small issues - found when importing my own copies of the bookmarks. I've extended the tests to cover these. I also realised that now we've reverted to insertTree handling the indexes, we no longer need the folder de-duplication code in importFromJSON, so I'm dropping that as well.

for EraseEverything slowness, we can try Bug 1371679 first, it should be easier. Bug 1087580 is a better approach, but involves scary changes to Sync and thus could be more involving. Also bug 1107364 may help, Milind is working on it already, but we'll delay further complex landings to after the merge, for the soft-freeze. Apart from that, any profiling on import/export is welcome, I'm sure we can find a lot of low hanging fruit improvements.

(In reply to Marco Bonardo [::mak] from comment #36) > Note that the scope here is not to have faster imports, it's to avoid > main-thread IO in doing them. That said, numbers would be welcome, but I'd > not expect those to be smaller. I thought converting to insertTree was supposed to make things a lot faster. That's the impression I got from comments in bug 1095426 anyway. HTML imports are horribly slow, I assume JSON is horribly slow as well.

(In reply to Josh Aas from comment #41) > I thought converting to insertTree was supposed to make things a lot faster. > That's the impression I got from comments in bug 1095426 anyway. HTML > imports are horribly slow, I assume JSON is horribly slow as well. The initial conversion is to remove main-thread jankiness. It's unlikely that moving from a synchronous API to an asynchronous one the wall clock time will reduce. We have other bugs that will improve the wall clock timing. The fact is if we don't remove the synchronous API, we can't proceed with other perf work and we have to spend more time maintaining two parallel APIs. Clearly, we should try to not regress the import times too much in this phase, so any things that we identified as critical should be fixed sooner.

Testing with the 2.8k json file that's in the example test patch on bug 1372496, and with that patch applied, I'm seeing: - Without both patches: ~5.9s initial import (into a basically empty db). ~5.3s subsequent imports. - With both patches: ~5.3s initial import, ~8.5s subsequent imports. With both patches it does seem to be slightly better to respond to other items whilst the import is happening.

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. https://reviewboard.mozilla.org/r/138744/#review154388 ::: toolkit/components/places/BookmarkJSONUtils.jsm (Diff revision 10) > + * @resolves When the new bookmarks have been created. > + * @rejects JavaScript exception. > */ > async importFromJSON(aString) { > - this._importPromises = []; > - let deferred = PromiseUtils.defer(); please remove the PromiseUtils import if no more used. ::: toolkit/components/places/BookmarkJSONUtils.jsm:367 (Diff revision 10) > + > + let convert = function(str, p1) { > + return "folder=" + newFolderIds.get(aFolderIdMap[p1]); > + } > + > + return uri.replace(reGlobal, convert); nit: I'd probably remove some newlines here, the code is a bit sparse ::: toolkit/components/places/BookmarkJSONUtils.jsm:395 (Diff revision 10) > - delete this._importPromises; > - } > +} > - }, > > - /** > +/** > - * Takes a JSON-serialized node and inserts it into the db. > + * Translates the JSON types for a node and its children into places compatible nit: UC Places ::: toolkit/components/places/BookmarkJSONUtils.jsm:450 (Diff revision 10) > - siteURI = NetUtil.newURI(aAnno.value); > - return false; > - default: > + default: > - return true; > + Cu.reportError(`Unexpected bookmark type ${node.type}`); > + break; > - } > + } How would insertTree handle such a broken entry? wouldn't it throw and discard the whole insert? I was wondering if we could throw here and remove the node in the caller code (that would require changing from for..of to indexing and fixing the looping index). In practice it should not happen, so it'd be fine to also just file a follow-up to investigate making this more fail-safe. ::: toolkit/components/places/BookmarkJSONUtils.jsm:470 (Diff revision 10) > - } > + } > + > + if (node.tags) { > + // Separate any tags into an array, and ignore any that are too long. > + node.tags = node.tags.split(",").filter(aTag => > + aTag.length <= Ci.nsITaggingService.MAX_TAG_LENGTH); empty strings are not good, so better checking aTag.legth > 0 too ::: toolkit/components/places/BookmarkJSONUtils.jsm:514 (Diff revision 10) > + */ > +function insertFaviconForNode(node) { > + if (node.icon) { > - try { > + try { > - // Create a fake faviconURI to use (FIXME: bug 523932) > + // Create a fake faviconURI to use (FIXME: bug 523932) > - let faviconURI = NetUtil.newURI("fake-favicon-uri:" + aData.uri); > + let faviconURI = NetUtil.newURI("fake-favicon-uri:" + node.url); while here, could you please replace all NetUtil.newURI with Services.io.newURI and make NetUtil a lazyModuleGetter? ::: toolkit/components/places/BookmarkJSONUtils.jsm:557 (Diff revision 10) > - let convert = function(str, p1, offset, s) { > + return; > - return "folder=" + aFolderIdMap[p1]; > } > - let stringURI = aQueryURI.spec.replace(/folder=([0-9]+)/g, convert); > > - return NetUtil.newURI(stringURI); > + for (let child of nodeTree.children) { nit: it's more compact if (nodeTree.children) { for ... ::: toolkit/components/places/Bookmarks.jsm:461 (Diff revision 10) > let observers = PlacesUtils.bookmarks.getObservers(); > for (let i = 0; i < insertInfos.length; i++) { > let item = insertInfos[i]; > let itemId = itemIdMap.get(item.guid); > let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null; > - notify(observers, "onItemAdded", [ itemId, parent._id, item.index, > + // For sub-folders, we need to make sure their children have the correct parents ids. typo (I think): parent ids ::: toolkit/components/places/Bookmarks.jsm:463 (Diff revision 10) > let item = insertInfos[i]; > let itemId = itemIdMap.get(item.guid); > let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null; > - notify(observers, "onItemAdded", [ itemId, parent._id, item.index, > + // For sub-folders, we need to make sure their children have the correct parents ids. > + let parentId; > + if (item.guid === parent.guid || Bookmarks.userContentRoots.includes(item.parentGuid)) { reindent? ::: toolkit/components/places/PlacesBackups.jsm:91 (Diff revision 10) > + * > + * @return {Promise} Resolve with an array of objects containing id and guid > + * when the query is complete. > + */ > +async function getTopLevelFolderIds() { > + let parentId = await PlacesUtils.promiseItemId(PlacesUtils.bookmarks.rootGuid); Well, for now you can use PlacesUtils.placesRootId. We'll change it when we are closer to removing ids globally. alternatively, you can change the query to: SELECT id, guid FROM moz_bookmarks WHERE parent = (SELECT id FROM moz_bookmarks WHERE parent = 0) ::: toolkit/components/places/PlacesBackups.jsm:581 (Diff revision 10) > + * Wrapper for PlacesUtils.bookmarks.eraseEverything that removes non-default > + * roots. > + * > + * Note that default roots are preserved, only their children will be removed. > + * > + * XXX Ideally we wouldn't need to worry about non-default roots. However, s/XXX/TODO/ ::: toolkit/components/places/tests/head_common.js:969 (Diff revision 10) > + */ > +function makeGuid() { > + return ChromeUtils.base64URLEncode(crypto.getRandomValues(new Uint8Array(9)), { > + pad: false, > + }); > +} Just use PlacesUtils.history.makeGuid() (remember to also remove the crypto import when you remove this) ::: toolkit/components/places/tests/unit/test_bookmarks_json.js:197 (Diff revision 10) > case "icon": > let deferred = Promise.defer(); > PlacesUtils.favicons.getFaviconDataForPage( > NetUtil.newURI(aExpected.url), > function(aURI, aDataLen, aData, aMimeType) { > deferred.resolve(aData); hm, I think that you actually removed the deferred declaration?! ::: toolkit/components/places/tests/unit/test_bookmarks_json.js:199 (Diff revision 10) > PlacesUtils.favicons.getFaviconDataForPage( > NetUtil.newURI(aExpected.url), > function(aURI, aDataLen, aData, aMimeType) { > deferred.resolve(aData); > }); > let data = await deferred.promise; ditto

Comment on attachment 8867152 [details] Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. https://reviewboard.mozilla.org/r/138744/#review154388 > How would insertTree handle such a broken entry? wouldn't it throw and discard the whole insert? > I was wondering if we could throw here and remove the node in the caller code (that would require changing from for..of to indexing and fixing the looping index). > In practice it should not happen, so it'd be fine to also just file a follow-up to investigate making this more fail-safe. Filed bug 1373610 and added a comment.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0cf7134d243a Make the bookmarks.json test file prettified so it can be read easily. r=mak https://hg.mozilla.org/integration/autoland/rev/4c465fe2d7c9 Convert JSON backups code to the new async Bookmarks.jsm API. r=mak

Requesting qe-verify. Even though I think the unit tests now cover most of it, it would be good to get some extra testing on this. Rough outline of what to test: 1) On an existing profile with various bookmarks, go to Bookmarks -> Show All Bookmarks. 2) Click the star type icon (the right-most), and select Backup... 3) Save the file to a json file 4) Create a new profile, and this time choose Restore -> Choose file... => The bookmarks should be imported successfully. => Then check the bookmarks in the existing & new profiles appear the same. Note: You can add more columns for last modified and date added (visit count/most recent visit will be zero) by right-clicking on the headers. If this could be tried with a few different profiles/people's bookmarks, that would be good in giving us more confidence that this is working correctly.

Backed out for frequently failing xpcshell's test_bookmark_engine.js | test_sync_dateAdded, especially on OS X: https://hg.mozilla.org/integration/autoland/rev/ff45f0da3efe1151151936e8c7c6340478875198 https://hg.mozilla.org/integration/autoland/rev/8b59ef19993d5ecf05413b4375535a199749a6a3 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=107718148&repo=autoland 08:12:05 INFO - PID 6314 | 1497625922714 Sqlite.Connection TRACE places.sqlite#1: Stmt #169 08:12:05 INFO - PID 6314 | SELECT frecency 08:12:05 INFO - PID 6314 | FROM moz_places 08:12:05 INFO - PID 6314 | WHERE url_hash = hash(:url) AND url = :url 08:12:05 INFO - PID 6314 | LIMIT 1 - {"url":"https://example.com/"} 08:12:05 INFO - PID 6314 | 1497625922718 Sqlite.Connection DEBUG places.sqlite#1: Stmt #169 finished. 08:12:05 INFO - PID 6314 | 1497625922719 Sqlite.Connection TRACE places.sqlite#0: Stmt #461 SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index', 08:12:05 INFO - PID 6314 | b.dateAdded, b.lastModified, b.type, b.title, h.url AS url, 08:12:05 INFO - PID 6314 | b.id AS _id, b.parent AS _parentId, 08:12:05 INFO - PID 6314 | (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount, 08:12:05 INFO - PID 6314 | p.parent AS _grandParentId, b.syncStatus AS _syncStatus 08:12:05 INFO - PID 6314 | FROM moz_bookmarks b 08:12:05 INFO - PID 6314 | LEFT JOIN moz_bookmarks p ON p.id = b.parent 08:12:05 INFO - PID 6314 | LEFT JOIN moz_places h ON h.id = b.fk 08:12:05 INFO - PID 6314 | WHERE b.guid = :guid 08:12:05 INFO - PID 6314 | - {"guid":"aaaaaaaaaaaa"} 08:12:05 INFO - PID 6314 | 1497625922720 Sqlite.Connection DEBUG places.sqlite#0: Stmt #461 finished. 08:12:05 INFO - TEST-PASS | services/sync/tests/unit/test_bookmark_engine.js | test_sync_dateAdded - [test_sync_dateAdded : 642] dateAdded in past should be synced - 1466089922289 == 1466089922289 08:12:05 WARNING - TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_bookmark_engine.js | test_sync_dateAdded - [test_sync_dateAdded : 643] dateAdded in future should be ignored in favor of last modified - "undefined" == 1497625822289

Kit/Mark, can one of you try and help me with the intermittent here please? Here's a summary: - We are trying to change BookmarkJSONUtils.importFromFile to use the new async bookmarks API - test_bookmark_engine.js is now intermittently failing. Possibly worst on Mac opt. On my MacBook Pro, I can reproduce > 50% of the time. So far, I have found: - test_sync_dateAdded sub-test fails due to record2 not having been added to the bookmarks DB. - In actual fact, several records in that test aren't added to the bookmarks DB as the last sync time is set to an actual value from an earlier sync. - In good runs, the last sync time is set to 1. - If I force the last sync time to 1, the test passes all the time. However it then concerns me that there's async/timing issues lying in wait for random oranges. - Although I can see the code that is setting the incorrect last sync time, I can't find where it is triggered from due to stacks being cut of due to the event handling nature of some of the code. If either of you could help out, it would be greatly appreciated.

I'm fairly sure there are extra unexpected syncs here, and fairly certain they aren't related to your patch (I *think* they are related to bug 1335891). I'm fine with you landing the "set to 1" you mentioned and we can open a new bug (as the problem is multiple oranges waiting to happen.)

(and FTR, I'm fairly sure this is a test-only issue, and I'm fairly sure the test *should* be setting .lastSync anyway - it's relying on the state left behind from a previous test)

(In reply to Mark Hammond [:markh] from comment #54) > I'm fairly sure there are extra unexpected syncs here, and fairly certain > they aren't related to your patch (I *think* they are related to bug > 1335891). I'm fine with you landing the "set to 1" you mentioned and we can > open a new bug (as the problem is multiple oranges waiting to happen.) Thanks, I've added that in with a reference to the new bug I filed - bug 1374599.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s f853a2766bd5 -d 5e0adea68a9b: rebasing 402818:f853a2766bd5 "Bug 1095426 - Make the bookmarks.json test file prettified so it can be read easily. r=mak" merging toolkit/components/places/tests/unit/bookmarks.json rebasing 402819:e6de736e1894 "Bug 1095426 - Convert JSON backups code to the new async Bookmarks.jsm API. r=mak" (tip) merging toolkit/components/places/BookmarkJSONUtils.jsm merging toolkit/components/places/Bookmarks.jsm merging toolkit/components/places/PlacesBackups.jsm merging toolkit/components/places/PlacesUtils.jsm merging toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js merging toolkit/components/places/tests/bookmarks/test_417228-exclude-from-backup.js merging toolkit/components/places/tests/bookmarks/test_417228-other-roots.js merging toolkit/components/places/tests/unit/bookmarks.json merging toolkit/components/places/tests/unit/test_384370.js merging toolkit/components/places/tests/unit/test_bookmarks_json.js merging toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js merging toolkit/components/places/tests/unit/test_sync_utils.js warning: conflicts while merging toolkit/components/places/Bookmarks.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/926a0b221b64 Make the bookmarks.json test file prettified so it can be read easily. r=mak https://hg.mozilla.org/integration/autoland/rev/25fe56e55e61 Convert JSON backups code to the new async Bookmarks.jsm API. r=mak

https://hg.mozilla.org/mozilla-central/rev/926a0b221b64 https://hg.mozilla.org/mozilla-central/rev/25fe56e55e61

Comment on attachment 8879804 [details] Bug 1095426 - neuter the sync scheduler during some tests to avoid oranges. Sorry, wrong bug #

In reply to Mark Banner from comment #51. I've tested the bookmarks import/export as requested on Windows 10 x64, Ubuntu 14.04 x64 and Mac 10.12.6. Please let me know if I’ve missed anything. Here are the results: 1) I have exported the bookmarks from an existing profile and I have successfully managed to import them on a new profile, on the same operating system. (covered all the above OS's) 2) I have exported the bookmarks from an existing profile and I have successfully imported them in a new profile, on a different OS. (covered all the combinations of the above OS's) 3) I have exported the bookmarks from an existing profile and I have successfully imported them on another profile with stored history and bookmarks on the same OS. The imported bookmarks successfully replaced the existing ones. (covered all the above OS's) 4) I have exported the bookmarks from an existing profile and then I have imported them in another profile with saved bookmarks, on a different OS. The imported bookmarks successfully replaced the existing ones. (covered all the combinations of the above OS's) 5) I have exported a json file from a newly created profile and have successfully imported it in an older profile, on the same operating system. The imported bookmarks successfully replaced the existing ones. (covered all the above OS's) 6) I have exported a json file from a newly created profile and have successfully imported it in an older profile, on a different OS. The imported bookmarks successfully replaced the existing ones. (covered all the combinations of the above OS's) 7) I have manually changed, added and removed some of the websites from the bookmarks "json" file and after, I have successfully imported the edited file in a new profile on same OS . All edited/added websites were correctly displayed and the ones removed were not. (covered all the above OS's) 8) I have manually changed, added and removed some of the websites from the bookmarks "json" file and after, I have successfully imported the edited file in a new profile, on a different OS. All edited/added websites were correctly displayed and the ones removed were not. (covered all the combinations of the above OS's) 9) I've organized the bookmarks in folders, added a few separators, exported them and after, I have successfully imported them in a new profile on the same OS. (covered all the above OS's) 10) I've organized the bookmarks in folders, added few separators, exported them and after, I have successfully imported them in a new profile, on a different OS. (covered all the combinations of the above OS's) 11) I have partially deleted the bookmarks "json" file content and when I've tried to import the bookmarks on a new profile the "Unable to process the backup file." message error appeared. (covered all the above OS's) Is this the expected behavior?

(In reply to Marius Coman [:cmarius] from comment #65) > I've tested the bookmarks import/export as requested on Windows 10 x64, > Ubuntu 14.04 x64 and Mac 10.12.6. Please let me know if I’ve missed anything. This looks like a good list. > 11) I have partially deleted the bookmarks "json" file content and when > I've tried to import the bookmarks on a new profile the "Unable to process > the backup file." message error appeared. (covered all the above OS's) > Is this the expected behavior? I would think so as it was likely the remaining content was invalid in some way.

Based on comment #66, I'm changing the status to VERIFIED FIXED.