1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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)

Comment 1

[Nicolas Ouellet-Payeur [:nicolaso]]

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?

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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.

Comment 3

[Nicolas Ouellet-Payeur [:nicolaso]]

Attachment: * Bug 1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine

Review commit: https://reviewboard.mozilla.org/r/165198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165198/

Comment 4

[Nicolas Ouellet-Payeur [:nicolaso]]

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

Comment 5

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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!

Comment 6

[Nicolas Ouellet-Payeur [:nicolaso]]

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

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(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!

Comment 8

[Nicolas Ouellet-Payeur [:nicolaso]]

Comment on attachment 8894139 [details]
* Bug 1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165198/diff/1-2/

Comment 9

[Nicolas Ouellet-Payeur [:nicolaso]]

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 10

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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!

Comment 11

[Nicolas Ouellet-Payeur [:nicolaso]]

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?

Comment 12

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(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 13

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: Bug 1353217 - importing bookmarks from html doesn't need to reset the bookmarks engine.

Review commit: https://reviewboard.mozilla.org/r/169996/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169996/

Comment 14

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf6b1b8c3291
importing bookmarks from html doesn't need to reset the bookmarks engine. r=markh

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf6b1b8c3291