Ran into this while debugging 1312031. The code at [0] should remove keywords that already exist for the given URL and have the same postData (It should also remove them from the cache). Or I guess it could throw. :mak suggested in IRC that these inserts should be prevented by the db schema (which would make it throw), but isn't since NULL != NULL for the postData. [0]: http://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#2232-2275

But people expect that it's the specific bookmarks that have keywords, not the urls+postdata, because it always worked like this. The UI also shows this. There's a keyword field for a bookmark. And it's not labeled keyword for url/post data. This is a discrepancy, and results in unexpectedly overwritten keywords. Also I'm quite sure that binding keywords to bookmark + postdata is broken, hence Bug 1314013. It appears to be only binded to URL.

(In reply to avada from comment #1) > But people expect that it's the specific bookmarks that have keywords, not > the urls+postdata, because it always worked like this. Sometimes things change. > The UI also shows this. Yes, the UI is wrong.

(In reply to Marco Bonardo [::mak] from comment #2) > Sometimes things change. > > > The UI also shows this. > > Yes, the UI is wrong. Alright. I guess you mean 648398? (which no-one works on) I don't understand how a change like this can be pushed, without it's corresponding UI change. This way this is just +1 bug starting with FF51 and will remain such until someone in the undetermined future adjusts the UI.

Because it was blocking other work and was part of a larger project that (shortly after that change) was paused for a priorities shift. I can't predict future, unfortunately. Regardless, keywords is a feature used by a very tiny minority and as such it's hard to sell fixes as things-we-really-need.

This blocks some API removals :(

This is not ready yet, I'd like to rebase it on top of bug 1435354 and I'm not sure how to handle the Sync mirror. But let's do things one at a time.

Comment on attachment 8949065 [details] Bug 1313188 - Multiple bookmark keywords for same (uri, postData) pair possible. https://reviewboard.mozilla.org/r/218468/#review224424 ::: toolkit/components/places/SyncedBookmarksMirror.jsm:2025 (Diff revision 1) > INSERT INTO itemsAdded(guid) > VALUES(OLD.mergedGuid); > > /* Insert new keywords after the item, so that "noteKeywordAdded" can find > the new item by Place ID. */ > - INSERT INTO moz_keywords(keyword, place_id) > + INSERT OR IGNORE INTO moz_keywords(keyword, place_id, post_data) I'm not sure `OR IGNORE` is correct here, but I'm also not sure if Sync's current behavior is correct, either. :-) What we do today is remove `newKeyword` from all URLs, regardless of POST data, and remove all keywords from `newPlaceId`. By the time we run this statement, I expect `newKeyword` shouldn't exist in `moz_keywords` at all. This does clobber POST data, but, since we don't sync it (bug 1345417), it's going to behave differently on other clients, anyway.

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #8) > I'm not sure `OR IGNORE` is correct here, but I'm also not sure if Sync's > current behavior is correct, either. :-) That was my same doubt! I'm also not 100% sure what happens if one of the statements in the INSTEAD OF trigger fails, I added the OR IGNORE mostly to avoid that case. > This does clobber POST data, but, since we don't sync it (bug 1345417), it's > going to behave differently on other clients, anyway. Yep. A keyword ideally is made up of 3 parts: the keyword, optional post data and optional charset. The charset for now is still stored apart in a page annotation, maybe one day it will move to this table. But eventually keywords could be implementable through WebExt or Search, then all of this would be pointless.

Comment on attachment 8949065 [details] Bug 1313188 - Multiple bookmark keywords for same (uri, postData) pair possible. https://reviewboard.mozilla.org/r/218468/#review226718 I think I follow now. Thanks! ::: toolkit/components/places/Database.cpp:2008 (Diff revision 2) > + "UPDATE moz_places " > + "SET foreign_count = (SELECT count(*) FROM moz_bookmarks WHERE fk = moz_places.id) + " > + "(SELECT count(*) FROM moz_keywords WHERE place_id = moz_places.id) " > + "WHERE id IN (SELECT DISTINCT place_id FROM moz_keywords) " > + )); > + NS_ENSURE_SUCCESS(rv, rv); Noting for future reference: I don't think we need to bump the change counter for removed duplicate keywords, because Sync will automatically remove duplicate keywords today (https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/toolkit/components/places/PlacesSyncUtils.jsm#1272-1304), ignoring POST data. ::: toolkit/components/places/PlacesUtils.jsm:2075 (Diff revision 2) > frecency: url.protocol == "place:" ? 0 : -1 }); > await db.executeCached("DELETE FROM moz_updatehostsinsert_temp"); > + > + // A new keyword could be assigned to an url that already has one, > + // then we must replace the old keyword with the new one. > + let oldKeywords = []; Oh, I see...this is more complicated than I thought. :-) To summarize... Without POST data, a URL can only have one keyword. If I assign `g` to `google.com`, then assign `s`, we'll remove `g`. With POST data, a URL can have multiple keywords. So I can assign both `(g, a=b&c=d)` and `(s, e=f&g=h)` to `google.com`. However, one keyword and different POST data cannot be assigned to two different URLs. If I try to assign `(a, i=j&k=l)` to `altavista.com`, and then `(a, m=n&o=p)` to `ask.com`, we'll remove `a` from `altavista.com`. Also, if I try to assign `s` to `bing.com`, with or without POST data, we'll remove it from `google.com`. With Sync, we don't sync POST data, so syncing `(g, a=b&c=d); (s, e=f&g=h)` to another device will keep *either* `s` or `g` assigned to `google.com`, but not both, and no POST data. Is that accurate? ::: toolkit/components/places/PlacesUtils.jsm:2082 (Diff revision 2) > + if (entry.url.href == url.href && entry.postData == postData) > + oldKeywords.push(entry.keyword); > + } > + if (oldKeywords.length) { > + for (let oldKeyword of oldKeywords) { > + await db.execute( Nit: Should we use `executeCached`, or the binding params array form here? ::: toolkit/components/places/SyncedBookmarksMirror.jsm:2038 (Diff revision 2) > INSERT INTO itemsAdded(guid, level) > VALUES(OLD.mergedGuid, OLD.mergedLevel); > > /* Insert new keywords after the item, so that "noteKeywordAdded" can find > the new item by Place ID. */ > - INSERT INTO moz_keywords(keyword, place_id) > + INSERT OR IGNORE INTO moz_keywords(keyword, place_id, post_data) I thought some more about this, and I'm OK with leaving `INSERT OR IGNORE`. The `DELETE FROM moz_keywords` above should clear out the keyword and URL from `moz_keywords`, so we shouldn't end up with constraint violations...but there's a lot of "should"s there, and, unlike bookmarks, failing to sync keywords is not fatal. Goofy keyword handling shouldn't prevent you from syncing your bookmarks forever. We also use `INSERT OR IGNORE` for the tags trigger, and this matches that. I wonder if we could try to migrate existing POST data for synced keywords, or leave them. I'm thinking we leave them, because we won't sync POST data for new keywords, anyway, it would be more complicated, and it's uncommon (citation needed).

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #11) > Without POST data, a URL can only have one keyword. If I assign `g` to > `google.com`, then assign `s`, we'll remove `g`. > With POST data, a URL can have multiple keywords. So I can assign both `(g, > a=b&c=d)` and `(s, e=f&g=h)` to `google.com`. However, one keyword and > different POST data cannot be assigned to two different URLs. If I try to > assign `(a, i=j&k=l)` to `altavista.com`, and then `(a, m=n&o=p)` to > `ask.com`, we'll remove `a` from `altavista.com`. Also, if I try to assign > `s` to `bing.com`, with or without POST data, we'll remove it from > `google.com`. I think I can summarize it as: 1 keyword -> 1 url 1 url => many keywords, if they differ by POST DATA (included the empty one) I guess I'll add this to the top of PlacesUtils.keywords as a good summary. > With Sync, we don't sync POST data, so syncing `(g, a=b&c=d); (s, e=f&g=h)` > to another device will keep *either* `s` or `g` assigned to `google.com`, > but not both, and no POST data. > > Is that accurate? It sounds accurate (for how much "accurate" can be a dataloss).

There are a few failures in browser_bookmarklet_windowOpen.js and browser_keywordBookmarklets.js, looking into those.

we seem to have code that relies on postdata being null rather than a null string (could make sense), so I'm making so that we store an empty string, but from the outside of the API we keep using null, to reduce the likelihood of regressions. The change in general just means it's not possible to have a keyword with an empty string as post data, nothing crazy but worth noticing.

Comment on attachment 8949065 [details] Bug 1313188 - Multiple bookmark keywords for same (uri, postData) pair possible. https://reviewboard.mozilla.org/r/218468/#review226878 ::: toolkit/components/places/PlacesUtils.jsm:1931 (Diff revision 4) > - * Sooner or later these keywords will merge with search keywords, this is an > + * Sooner or later these keywords will merge with search aliases, this is an > * interim API that should then be replaced by a unified one. > * Keywords are associated with URLs and can have POST data. > - * A single URL can have multiple keywords, provided they differ by POST data. > + * The relations between URLs and keywords are the following: > + * - 1 keyword can only point to 1 URL > + * - 1 URL can have multiple keywords, iff they differ by POST data (included the empty one). nit: iff

Comment on attachment 8949065 [details] Bug 1313188 - Multiple bookmark keywords for same (uri, postData) pair possible. https://reviewboard.mozilla.org/r/218468/#review226886

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/0e1ed4c72242 Multiple bookmark keywords for same (uri, postData) pair possible. r=kitcambridge,standard8

relnote: it's no more possible to have multiple bookmark keywords for the same url, unless the request has different POST data.

Added to Firefox Nightly 60 release notes as : It is no longer possible to have multiple bookmark keywords for the same url, unless the request has different POST data.

comment 22.