Closed Bug 1353217 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: markh, Assigned: nicolaso)

Details

Attachments

(2 files)

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)
Priority: -- → P3
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?
Flags: needinfo?(markh)
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.
Assignee: nobody → nicolaso
Flags: needinfo?(markh)
Attachment #8894139 - Flags: review?(markh)
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!
Attachment #8894139 - Flags: review?(markh) → review+
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
Attachment #8898602 - Flags: review?(markh) → review+
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
https://hg.mozilla.org/mozilla-central/rev/bf6b1b8c3291
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.