610501 - Places smart bookmarks are mishandled by Sync

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Smart bookmarks (e.g. most visited on the toolbar, most recent tags on menu...) are not correctly handled by Sync, thus I keep getting duplicates.
I'ts possible I've missed something in the bookmarks engine, but looks like they are just not handled.

Smart bookmarks have an annotation "Places/SmartBookmark", since I guess you don't sync browser.places.smartBookmarksVersion pref, you should not save items with that annotation.

In my profile all smart bookmarks are missing the annotation, plus I have a bogus one that has it but is missing title (?), when I start firefox on my Mac I sometimes end up having double smart bookmarks on Win. This means that next time we will update smart bookmarks, users will get duplicates since there is no way to recognize them without the annotation. there is no fix for this, apart the fact they can delete the dupe.

There is some code handling place: queries in http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#780 that is told to handle smart bookmarks, but it looks bogus. It seems to think that all place: queries are tags, that is not the case (tags have not only folder= but also type=7 that is RESULTS_AS_TAG_CONTENTS). If you clarify me scope of that code I could evaluate if it's fine or file a new bug about it.

Comment 1

[Tracy Walker [:tracy]]

To reproduce

0) using default fresh profiles
1) Create an account with a storage version 5 client (used minefield nightly)
2) attempt to sync to that account with by adding a storage version 3 client (used 3.5.x with Sync 1.5.1)
3) forced to upgrade Sync to 1.6
4) After upgrade, Sync from the upgraded client

Tested results:
Two "Most Visited" Smart marks appear on the toolbar, the content of which is identical.

Expected result: only one "Most Visited" bookmark

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to comment #0)
> Smart bookmarks (e.g. most visited on the toolbar, most recent tags on menu...)
> are not correctly handled by Sync, thus I keep getting duplicates.
> I'ts possible I've missed something in the bookmarks engine, but looks like
> they are just not handled.

I don't understand what you mean by "they are just not handled". Are you talking about the dupe detection for smart bookmarks? We do handle them but based on their title and URI. If they're different URIs, we don't recognize them as the same.

I guess this is what Tracy is running into. Firefox 3.5 and nightlies have different smart bookmark schemas so he's getting dupes because their URIs don't match. We should probably look at Places/SmartBookmark here to identify dupes, but the problem is that our server records don't contain that information.

> Smart bookmarks have an annotation "Places/SmartBookmark", since I guess you
> don't sync browser.places.smartBookmarksVersion pref, you should not save items
> with that annotation.

Or we should make sure that Places/SmartBookmark is synced as well and used in the dupe detection. If we did that, we should be ok, right? It would mean a semi-backwards-compatible change to the bookmark spec (https://wiki.mozilla.org/Labs/Weave/Developer/BrowserObjects#query). New clients would include that information in the record and know how to deal with it, but old clients wouldn't. Old clients in this case would be 4.0b8 and 1.6.

> In my profile all smart bookmarks are missing the annotation, plus I have a
> bogus one that has it but is missing title (?), when I start firefox on my Mac
> I sometimes end up having double smart bookmarks on Win. This means that next
> time we will update smart bookmarks, users will get duplicates since there is
> no way to recognize them without the annotation. there is no fix for this,
> apart the fact they can delete the dupe.

Yes, I'm afraid we'll have to bite that bullet. It's too late now since Sync might already have duped them and created smart bookmarks without the annotation.

> There is some code handling place: queries in
> http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#780
> that is told to handle smart bookmarks, but it looks bogus. It seems to think
> that all place: queries are tags, that is not the case (tags have not only
> folder= but also type=7 that is RESULTS_AS_TAG_CONTENTS). If you clarify me
> scope of that code I could evaluate if it's fine or file a new bug about it.

You're right, we should explicitly check for that. That code is pretty scary anyway, using a regex to manipulate the query URI. I think we should rather use nsINavHistoryService::queryStringToQueries(), manipulate the resulting query object and then serialize it back.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #2)
> (In reply to comment #0)
> > Smart bookmarks (e.g. most visited on the toolbar, most recent tags on menu...)
> > are not correctly handled by Sync, thus I keep getting duplicates.
> > I'ts possible I've missed something in the bookmarks engine, but looks like
> > they are just not handled.
> 
> I don't understand what you mean by "they are just not handled". Are you
> talking about the dupe detection for smart bookmarks? We do handle them but
> based on their title and URI. If they're different URIs, we don't recognize
> them as the same.

These bookmarks are created by the browser glue code when browser.places.smartBookmarksVersion is not current, they are not created by the user.
It's possible we correctly manage dupes here, but then browser glue notices version is not current, and that there is no item with the smart bookmark annotation. Then they are created from scratch, making dupes.

> > Smart bookmarks have an annotation "Places/SmartBookmark", since I guess you
> > don't sync browser.places.smartBookmarksVersion pref, you should not save items
> > with that annotation.
> 
> Or we should make sure that Places/SmartBookmark is synced as well and used in
> the dupe detection. If we did that, we should be ok, right? It would mean a
> semi-backwards-compatible change to the bookmark spec

If we sync the SmartBookmark annotation, then the browser glue code will be able to detect them and won't try to create duplicates.

> Yes, I'm afraid we'll have to bite that bullet. It's too late now since Sync
> might already have duped them and created smart bookmarks without the
> annotation.

I'd say to fix future use, we can't go back.

> > There is some code handling place: queries in
> > that is told to handle smart bookmarks, but it looks bogus.
> You're right, we should explicitly check for that. That code is pretty scary
> anyway, using a regex to manipulate the query URI. I think we should rather use
> nsINavHistoryService::queryStringToQueries(), manipulate the resulting query
> object and then serialize it back.

it will probably be a bit slower but since we don't have lots of place: uris should be fine.

Comment 4

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 5

[LegNeato]

Fixing fields my automated script accidentally blanked. Apologies for the bugspam

Comment 6

[Philipp von Weitershausen [:philikon]]

Richard, can you take this? Comment 2 has the details for the todo list:

* Add an (optional) field to "query" records that contains the value of the "Places/SmartBookmark" annotation.

* Use the query API to manipulate the actual places query rather than regex.

In addition to that, the duping code for queries should try to dupe them according to the "Places/SmartBookmark" value (if present). Since older records can exist without that value, we still need to fall back to duping by name.

Comment 7

[Richard Newman [:rnewman]]

On my plate.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 1. v1

This patch adds a new test, test_bookmark_smart_bookmarks. Said test creates a smart bookmark with annotation, checks that an attribute(smartBookmarksAnno) exists when recordified, and verifies that it survives round-tripping through the server.

Then I made the test pass, of course.

There is a dependency on the WBO fix in Bug 627097, if I'm not mistaken. That'll be landing shortly (just waiting for Try xpcshell tests to finish).

Comment 9

[Richard Newman [:rnewman]]

(In reply to comment #8)

> There is a dependency on the WBO fix in Bug 627097, if I'm not mistaken.

Ignore this. Nonsense.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 1. v2

Philipp helpfully pointed out that the annos in question aren't numeric; I was misreading the Places code. Changed.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #10)
> Philipp helpfully pointed out that the annos in question aren't numeric; I was
> misreading the Places code. Changed.

yes, they contain a string identifier that allows to recognize each smart bookmark, so that in case they are modified we can replace each of them with the updated version in place.

Comment 12

[Richard Newman [:rnewman]]

Attachment: Notes on part 2.

OK, I'm confused here.

Firstly, I don't understand what the Sync code is trying to do. It does this:

    record.bmkUri.replace(/([:&]folder=)\d+/, "$1" +
                          child.itemId);

i.e., it tries to replace a digit string "folder" param value in the URI with an item ID. Well, the Places docs say:

https://developer.mozilla.org/en/Places_query_URIs
---
  folder 	string 	The folder to query. This may be one of the following:

     PLACES_ROOT
       The Places root folder.
     ...
---

and looking at user data, the docs are correct: URIs look like this

     place:folder=BOOKMARKS_MENU&folder=UNFILED_BOOKMARKS&folder=TOOLBAR&queryType=1&sort=12&exc...

The Sync code won't do anything, because BOOKMARKS_MENU doesn't match \d+. When will we see a record arriving with a numeric folder?

Secondly, the query parser response doesn't expose folders, as far as I can see; only a count of folders.

Attached is what happens when I run with a synthetic query (adding numeric folder IDs in place of strings). Note that there's no folder output in the JSON dump of any of the parse results.

Also attached is the output of running with a real user's URI.

Am I doing something wrong here? Unless someone can point me in the right direction (i.e., tells me where I can find "1234" in those JSON blobs), it doesn't look like we can rewrite place: queries in this way.

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to comment #12)
> Firstly, I don't understand what the Sync code is trying to do. 

The idea is to handle tag queries. Tags are hidden folders within the "Tags" folder (which resides in the places root). An bookmark in a tag folder constitutes a tag for that URL. (IIRC)

You can create a smart bookmark that queries a particular tag by dragging a tag from the "Tags" folder to another folder (Most. Obscure. Feature. Ever.) The code below tries to handle this case:

>     record.bmkUri.replace(/([:&]folder=)\d+/, "$1" +
>                           child.itemId);

...

> When will we see a record arriving with a numeric folder?

For tag queries. store.createRecord() leaves the query untouched but sets record.folderName to the title of the folder (title of tag folder == tag name). Then in applyIncoming, the above code tries to find the local places id of said tag folder and rewrite the query. Using regexes. Priceless.

> Secondly, the query parser response doesn't expose folders, as far as I can
> see; only a count of folders.

https://developer.mozilla.org/en/nsINavHistoryQuery#setFolders.28.29 seems to do the trick, no?

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 507040 [details] [diff] [review]
Part 1. v2

>-Utils.deferGetSet(BookmarkQuery, "cleartext", ["folderName"]);
>+Utils.deferGetSet(BookmarkQuery, "cleartext", ["folderName",
>+                                               "smartBookmarksAnno"]);

Our payload objects try to be agnostic of the underlying storage model. so I would suggest a different name here. How about "queryid"?

>+          
>+          // Persist the Smart Bookmark anno, if found.
>+          try {
>+            let anno = Utils.anno(placeId, SMART_BOOKMARKS_ANNO);
>+            this._log.debug("query anno: " + SMART_BOOKMARKS_ANNO +
>+                            " = " + anno);

This feels more like a trace msg :)


So this takes care of store.create() and store.createRecord(), but not store.update(). For the sake of completeness I think we should support it, even if it will probably never happen. r=me with that and above comments.

Also need part 2 to sanitize our tag query URI processing.

Comment 15

[Richard Newman [:rnewman]]

(In reply to comment #14)

> Our payload objects try to be agnostic of the underlying storage model. so I
> would suggest a different name here. How about "queryid"?

That makes more sense, now that I know what the heck the anno actually contains :D

> This feels more like a trace msg :)

Was just following the pattern immediately above:

   this._log.debug("query id: " + folder + " = " + record.folderName);

Happy to change them both to trace... :)

> So this takes care of store.create() and store.createRecord(), but not
> store.update(). For the sake of completeness I think we should support it, even
> if it will probably never happen. r=me with that and above comments.

Sounds good, thanks for the review.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #12)
> Firstly, I don't understand what the Sync code is trying to do. It does this:
> The Sync code won't do anything, because BOOKMARKS_MENU doesn't match \d+. When
> will we see a record arriving with a numeric folder?

Actually, the most common case is a numeric value, those strings are there as placeholders for most common roots, indeed we could not set bookmarks menu to folder=2, since it's not sure that in each profile bookmarks menu has id 2
So that param accepts both numeric values or predefined constants.

(In reply to comment #13)
> You can create a smart bookmark that queries a particular tag by dragging a tag
> from the "Tags" folder to another folder (Most. Obscure. Feature. Ever.) The
> code below tries to handle this case:
> 
> >     record.bmkUri.replace(/([:&]folder=)\d+/, "$1" +
> >                           child.itemId);

this is not completely correct, tags are folders inside the tags root, true, but a tag query in our UI is not place:folder=X but place:folder=X&type=7 (where 7 is RESULTS_AS_TAG_CONTENTS). There is a big difference between the 2, syncing a tag container without the type part would break it.
On the other side, it's absolutely possible a user has a place:folder=X shortcut to a folder, and this is not a tag if the folder is not inside the tags root.

I hope this helps clarifying how queries work.

Comment 17

[Richard Newman [:rnewman]]

Part 1 pushed:

http://hg.mozilla.org/services/fx-sync/rev/3229cd3b518e

Blocked the merge bug.

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to comment #17)
> Part 1 pushed:
> 
> http://hg.mozilla.org/services/fx-sync/rev/3229cd3b518e

Thanks. I think we should also land this on 1.6.x so that users start syncing this new property asap.

Comment 19

[Philipp von Weitershausen [:philikon]]

I forgot, we also need an addendum to part 1 as mentioned in comment #6:

> In addition to that, the duping code for queries should try to dupe them
> according to the "Places/SmartBookmark" value (if present). Since older records
> can exist without that value, we still need to fall back to duping by name.

That way we don't end up with duplicate "Recently Bookmarked" items when you're syncing across different versions of Firefox and the query URI has changed every so slightly.

Comment 20

[Richard Newman [:rnewman]]

(In reply to comment #18)

> Thanks. I think we should also land this on 1.6.x so that users start syncing
> this new property asap.

Done:

http://hg.mozilla.org/services/fx-sync/rev/7fa9963402d2

This time I even remembered to r=. *sigh*

Part 1.5 and 2 coming soon.

Comment 21

[Philipp von Weitershausen [:philikon]]

Updated https://wiki.mozilla.org/Labs/Weave/Developer/BrowserObjects#query

Comment 22

[Richard Newman [:rnewman]]

Attachment: Part 1.5. v1

Let's see if I got this right!

Comment 23

[Richard Newman [:rnewman]]

Attachment: Part 2. v1

This alters the query rewriting technique, and also pulls out the tag manipulation into a separate method, rather than leaving it stuffed into applyIncoming.

I also added logic to only rewrite URIs when they're tag queries. The check for this uses the query parsing that's in the rewrite code, which was a motivation for the shape of the resulting code.

Tests added for the factored-out function.

Running CW now.

Comment 24

[Richard Newman [:rnewman]]

(In reply to comment #23)

> Running CW now.

All passed.

Comment 25

[Philipp von Weitershausen [:philikon]]

Comment on attachment 507263 [details] [diff] [review]
Part 1.5. v1

This still dupes smart bookmarks based on their parent. I think we can simplify this while making it a bit more generic: if we detect a query record with a queryId attribute, we dupe it *just* according to the queryId. If there isn't a queryId we'll just continue down the normal code path.

Comment 26

[Philipp von Weitershausen [:philikon]]

Comment on attachment 507281 [details] [diff] [review]
Part 2. v1

>+  /*
>+   * If the record is a tag query, rewrite it to refer to the local tag ID.
>+   * 
>+   * Otherwise, just return.
>+   */
>+  preprocessTagRecord: function preprocessTagRecord(record) {

nit: preprocessTagQuery seems more accurate. Looks great otherwise!

Comment 27

[Philipp von Weitershausen [:philikon]]

Comment on attachment 507263 [details] [diff] [review]
Part 1.5. v1

So it turns out that Places allows multiple queries with the same anno to appear when you copy a smart bookmark in the UI. IMHO that's a Places bug (it should really just whitelist the annotations that are allowed to be copied) but until that's fixed, this looks like an ok solution.

Let's make sure we file a bug on the Places guys and revamp this when that is fixed.

Comment 28

[Richard Newman [:rnewman]]

Pushed:

http://hg.mozilla.org/services/fx-sync/rev/eb2ac0e0fdcd
http://hg.mozilla.org/services/fx-sync/rev/d5bd3cab0a7d

Will file a Places bug to investigate anno duping.

Comment 29

[Richard Newman [:rnewman]]

(In reply to comment #28)

> Will file a Places bug to investigate anno duping.

Bug 629620.

Comment 30

[Philipp von Weitershausen [:philikon]]

(In reply to comment #28)
> Pushed:
> 
> http://hg.mozilla.org/services/fx-sync/rev/eb2ac0e0fdcd
> http://hg.mozilla.org/services/fx-sync/rev/d5bd3cab0a7d

Please also land on 1.6.x. Thanks!

Comment 31

[Richard Newman [:rnewman]]

(In reply to comment #30)

> Please also land on 1.6.x. Thanks!

Done:

http://hg.mozilla.org/services/fx-sync/rev/0876761b4d79
http://hg.mozilla.org/services/fx-sync/rev/3244ae97fe52

Hooray for `hg transplant`!

Comment 32

[Tracy Walker [:tracy]]

Verified with 1.6.3 on Mac and Win 7