Closed Bug 1328737 Opened 5 years ago Closed 5 years ago

Sync doesn't register bookmark keywords

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Assigned: tcsc)

References

Details

Attachments

(1 file)

STR:
(1) Add a bookmark for https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%s
(2) In the bookmark manager, set the 'keyword' field to 'bug'.
(3) Observe that typing "bug 1234" in the location bar brings up the relevant bug.
(4) Sync it to another device.
(5) Observe that typing "bug 1234" on the second device goes straight to the search provider.

If you go in and manually change the keyword on the second device, it will start working. So presumably there's some registration work that happens when the keyword is set from the bookmark manager, and this work is not being replicated by the sync code.
markh will investigate
Priority: -- → P2
Bobby, I'm guessing you on Nightly? Sadly I can't reproduce this - it works fine for me with exactly your STR. When you were on the second device and re-edited the keyword, was the keyword already in place or was it blank? (The STR sound alot like the former, but I thought I'd check!) Can you please try again and see if you have the same problem again?

Mak, does this ring a bell? Sync just uses PlacesUtils.keywords.insert - there's nothing else we should need to do, right?
Flags: needinfo?(mak77)
Flags: needinfo?(bobbyholley)
:tcsc also just reminded me that keywords work strangely when you have another keyword with the same postData/uri pair - I'm sure :mak understands this better than us, so maybe he has some other ideas about what might have caused this.
(In reply to Mark Hammond [:markh] from comment #2)
> Mak, does this ring a bell? Sync just uses PlacesUtils.keywords.insert -
> there's nothing else we should need to do, right?

Keywords are bound to uris, but presented in the UI as bound to bookmarks for historical reasons (mostly cause bug 648398 has not yet been implemented, and still that won't magically fix Sync, likely the opposite).
Adding keywords generates bookmark notifications yet (If any bookmark is found for that url), and in the new Sync implementation should also increase the bookmark sync delta. So from this point of view we may be fine.

I suspect what Tom remembered is bug 1313188, that is still unresolved (may be fixed changing the NULL to empty strings in the table, I think, cause NULL <> NULL). I don't know if that's related to this case, doesn't look like this keyword has post data.

There could be some edge case related to the fact one keyword can only point to one url (unless post data differs), in that case insert "moves" they keyword to the new url. Maybe in this case we should also increase the old url bookmarks delta, we don't seem to do that, we only increase the new url (http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/toolkit/components/places/PlacesUtils.jsm#2245). But again, unless there was another existing "bug" keyword I'm not sure this would matter? The "other" profile doesn't have a bug keyword clearly if he gets a search.

The other case is whether there is another existing keyword in the "other" profile that points to the same identical url, then it's possible the insert fails, it doesn't manage that case automatically, it will probably throw and consider an error to try to have 2 keywords for the same destination.

So, could you please verify (even just by inspecting the moz_keywords table in places.sqlite) if there is another keyword that points to the same url as the one "bug" was trying to point?
Flags: needinfo?(mak77)
Thanks for investigating this!

(In reply to Mark Hammond [:markh] from comment #2)
> Bobby, I'm guessing you on Nightly?

DevEdition 52 MacOS for device 1, Release 50.1.0 for device 2.

> Sadly I can't reproduce this - it works
> fine for me with exactly your STR. When you were on the second device and
> re-edited the keyword, was the keyword already in place or was it blank?

It was already in place.

I just tried adding a brand new bookmark keyword, and it got synced over successfully. So it's possible that it only happens on the initial sync (I set up device 2 just a few weeks ago), or for very old bookmarks (I created the 'bug' bookmark keyword several years ago), or that my places database is corrupt somehow, or that this was a non-deterministic issue on the wire, or something else.

Regardless, it looks like this isn't actionable, and I don't have cycles to chase it further. Thanks to all who jumped on this one! :-)
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(bobbyholley)
Resolution: --- → WORKSFORME
We were dropping them on the floor in some cases during insertion. Not a difficult fix.
Assignee: nobody → tchiovoloni
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment on attachment 8825858 [details]
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion.

Thanks! I think Kit's a better reviewer for this!
Attachment #8825858 - Flags: review?(markh) → review?(kit)
Comment on attachment 8825858 [details]
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion.

https://reviewboard.mozilla.org/r/103942/#review105540

Great catch! Nice, thorough comment, too.

::: toolkit/components/places/PlacesSyncUtils.jsm:913
(Diff revision 1)
> +// a different keyword, etc.
> +//
> +// If we don't handle those cases by removing the conflicting keywords first,
> +// the insertion  will fail, and the keywords will either be wrong, or missing.
> +// This function handles those cases.
> +var removeConflictingKeywords = Task.async(function* (bookmarkURL, newKeyword) {

It looks like we're calling `insert` after `removeConflictingKeywords` in both cases. Since we're already passing the new keyword, I wonder if it makes sense to have this function do the insertion, too.

Your call if you'd like to fix or drop; the way it's written is just as good.
Attachment #8825858 - Flags: review?(kit) → review+
Comment on attachment 8825858 [details]
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion.

https://reviewboard.mozilla.org/r/103942/#review105542

::: toolkit/components/places/PlacesSyncUtils.jsm:1173
(Diff revision 1)
>        updateInfo.syncId}`, ex);
>    }
>  
>    if (updateInfo.hasOwnProperty("keyword")) {
>      // Unconditionally remove the old keyword.
> -    let entry = yield PlacesUtils.keywords.fetch({
> +    yield removeConflictingKeywords(oldBookmarkItem.url.href, updateInfo.keyword);

Also, could you add a test with a conflicting keyword, please? I think test_sync_utils already has some keyword tests...
Priority: P2 → P1
See Also: → 1332897
Comment on attachment 8825858 [details]
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion.

https://reviewboard.mozilla.org/r/103942/#review107314

::: toolkit/components/places/PlacesSyncUtils.jsm:959
(Diff revision 1)
>      BookmarkSyncLog.warn(`insertBookmarkMetadata: Error tagging item ${
>        insertInfo.syncId}`, ex);
>    }
>  
>    if (insertInfo.keyword) {
> +    yield removeConflictingKeywords(bookmarkItem.url.href, insertInfo.keyword);

I haven't really looked deeply at this, but if some other bookmark has the keyword, then one of 2 things must have happened:

1) An incoming record that removes the keyword will be processed later as part of this sync.

2) Otherwise, we've a data integrity issue, meaning the server version of the bookmark originally with the keyword still has that keyword, so this change should cause that original record to be uploaded.

1) is obviously fine, but are we doing anything about (2)?
Attachment #8825858 - Flags: review?(markh)
Comment on attachment 8825858 [details]
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion.

apparently reviewboard is dumb.
Attachment #8825858 - Flags: review?(markh)
Comment on attachment 8825858 [details]
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion.

https://reviewboard.mozilla.org/r/103942/#review107314

> I haven't really looked deeply at this, but if some other bookmark has the keyword, then one of 2 things must have happened:
> 
> 1) An incoming record that removes the keyword will be processed later as part of this sync.
> 
> 2) Otherwise, we've a data integrity issue, meaning the server version of the bookmark originally with the keyword still has that keyword, so this change should cause that original record to be uploaded.
> 
> 1) is obviously fine, but are we doing anything about (2)?

No, although it's not clear to me it can be an issue in practice, since the other clients will handle this the same.

Although, I guess at this point bumping that item's sync change counter would be easy enough... kit: is that safe to do?
Comment on attachment 8825858 [details]
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion.

https://reviewboard.mozilla.org/r/103942/#review105540

> It looks like we're calling `insert` after `removeConflictingKeywords` in both cases. Since we're already passing the new keyword, I wonder if it makes sense to have this function do the insertion, too.
> 
> Your call if you'd like to fix or drop; the way it's written is just as good.

The update case isn't an unconditional insertion, so I didn't do this.
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f094bbd6ce46
Fix bug where sync wouldn't sync some bookmark keywords on insertion. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/f094bbd6ce46
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.