Importing bookmarks (as opposed to restoring them) is very dumb - it blindly creates new bookmarks. It never de-dupes and never deletes items. Importing the same file n times always causes n duplicates. Sync treats this just like a restore - it resets the client and deletes the server. This shouldn't be necessary - the bookmarks should just look like a bunch of new bookmarks were created. (It's not clear if the notifications we receive can make the distinction between "import" and "restore", but that shouldn't be too hard)

From what I understand, we need to add a guard around this code: http://searchfox.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#976-981 So that it only runs when we "restore", but not when we "import". And we need to add some way of distinguishing between import & restore, since right now they both trigger a "bookmarks-restore-success" event. Is that right? Mind if I pick this up?

Yep, that's correct - the tricky bit will be working out how to determine if it is an import vs a restore. Most "restores" and up via http://searchfox.org/mozilla-central/source/toolkit/components/places/BookmarkJSONUtils.jsm#58 with aReplace = true, but there are a number of other places that send the notification which you will need to dig in to - we probably can't change the existing notification, but probably could (say) add new data to it if necessary - or we could just create a new notification. Once you dig in I'll be happy to change up the places maintainer to see how to proceed if it looks tricky.

I went with a straightforward solution, which is to just use a different data string for "json-append" (without replace) and "json" (with replace). Although, it doesn't seem like the "json-append" code would be reached at any point anyways. This change shouldn't break much, since we're the only ones listening for this notification as far as I know: http://searchfox.org/mozilla-central/search?q=bookmarks[_-]restore[_-]&case=false&regexp=true

That looks fine to me, although it would be great if we can get a couple of extra tests - test_bookmark_engine.js already has a test for BookmarkJSONUtils.importFromFile, so if you could add a test that calls that with aReplace=false and check that the existing bookmarks were *not* replace I think we'd be golden!

It seems like using aReplace=false doesn't work at all, at least unless some very specific conditions are met. The SQLite table used to store bookmarks expects unique GUIDs for each element (UNIQUE constraint...), and importing from a JSON file re-uses the old GUIDs. What this means is that if there is any common GUID (including the root of the bookmark tree!) between the loaded JSON file and old bookmarks, `importFromFile()` with aReplace=false will fail. This seems like a bug or unsupported feature in Places. We could file a separate bug for this if we want. At any rate, I think fixing that problem in `importFromFile()` falls outside the scope of this bug. Basically, I'm saying it should be fine to have no tests for code that doesn't work yet. > 1:09.27 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "Failed to restore bookmarks from /opt/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-profile-cOeCgv/t_b_e_1502266279997.json: Error: Error(s) encountered during statement execution: UNIQUE constraint failed: moz_bookmarks.guid" {file: "resource://gre/modules/BookmarkJSONUtils.jsm" line: 110}] > BJU_importFromFile/<@resource://gre/modules/BookmarkJSONUtils.jsm:110:9 > async*BJU_importFromFile@resource://gre/modules/BookmarkJSONUtils.jsm:96:19 > test_restore@/opt/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/services/sync/tests/unit/test_bookmark_engine.js:404:11 > async*test_restoreWithoutReplacePromptsReupload@/opt/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/services/sync/tests/unit/test_book > mark_engine.js:485:9 > async*asyncFunction@resource://gre/modules/Task.jsm:241:18 > Task_spawn@resource://gre/modules/Task.jsm:166:12 > _run_next_test@/opt/mozilla-central/testing/xpcshell/head.js:1488:9 > run@/opt/mozilla-central/testing/xpcshell/head.js:701:9 > _do_main@/opt/mozilla-central/testing/xpcshell/head.js:221:3 > _execute_test@/opt/mozilla-central/testing/xpcshell/head.js:544:5 > @-e:1:1 > "

(In reply to Nicolas Ouellet-Payeur from comment #6) > It seems like using aReplace=false doesn't work at all, at least unless some > very specific conditions are met. The SQLite table used to store bookmarks > expects unique GUIDs for each element (UNIQUE constraint...), and importing > from a JSON file re-uses the old GUIDs. Fair enough. How about a test which uses BookmarkHTMLUtils instead? That doesn't use GUIDs so will just create duplicate bookmarks, but that's kinda the entire point of this bug (ie, that BookmarkHTMLUtils.importFromFile with replace=false doesn't cause sync to nuke the world). Thanks!

Added tests for HTML imports, and checks whether the server gets wiped or not after `BookmarkHTMLUtils.importFile()`. Tried to share the code for the HTML import & JSON restore tests as much as possible to avoid duplication.

Comment on attachment 8894139 [details] * Bug 1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine https://reviewboard.mozilla.org/r/165198/#review173806 Sorry for the delay and thanks for the patch!

Kit triggered a try build for me, here it is. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f31e33c3557a Are there any blockers for landing this?

(In reply to Nicolas Ouellet-Payeur from comment #11) > Are there any blockers for landing this? Oops, sorry, I'll land it now - thanks again!

Comment on attachment 8898602 [details] Bug 1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine. https://reviewboard.mozilla.org/r/169996/#review175160

Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/autoland/rev/bf6b1b8c3291 importing bookmarks from html doesn't need to reset the bookmarks engine. r=markh