User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID: 20180607202626 Steps to reproduce: 1. Create a fresh profile 2. Load https://app.stopbreathethink.org/ 3. Add a bookmark forhttps://app.stopbreathethink.org/ with the tag "stop" 4. Open the bookmark manager (Ctrl-Shift-O) 5. Add keyword "stop" for https://app.stopbreathethink.org/ 6. Delete bookmark for https://app.stopbreathethink.org/ Actual results: The keyword remained in the moz_keywords table (confirmed via running ".dump moz_keywords" on places.sqlite), which causes typing "stop" in the search bar to suggest https://app.stopbreathethink.org/ at the top of the suggestions. This doesn't happen if I don't tag the bookmark. Expected results: The keyword should have been removed from moz_keywords, causing https://app.stopbreathethink.org/ to show up further down the search suggestions list from the search bar

I should mention that I reproduced this with a Firefox built from Mercurial (revision = 6ecc24376875). I've attached a patch to add a test to reproduce this issue; please let me know if the test looks ok!

Oh, interesting, thanks for digging into this! I think the query in https://searchfox.org/mozilla-central/rev/e6a0a192ea8691f7eb466506301da73fabe8fd23/toolkit/components/places/PlacesUtils.jsm#2491-2496 needs to be a bit smarter. Tags are stored as bookmarks under the hood, so `removeFromURLsIfNotBookmarked` still thinks the URL is bookmarked.

I just attached a patch to fix the issue (well, the test I wrote, anyway) - how does that look?

Comment on attachment 8984756 [details] [diff] [review] test.patch Review of attachment 8984756 [details] [diff] [review]: ----------------------------------------------------------------- Hi Rob, thank you for the patches. I've left a few comments about the test below. I've not looked at the SQL part, as I don't know enough about that particular bit to know how the performance is. Could you update your patch with my comments, then combine the two patches into one, and post a single new patch? When you post, request review from Marco (set the reviewer flag to `::mak`) as he'll be best to look at the SQL part of this. ::: toolkit/components/places/tests/unit/test_keywords.js @@ +493,5 @@ > }); > + > +add_task(async function test_tagDoesntPreventKeywordRemoval() { > + var tagssvc = Cc["@mozilla.org/browser/tagging-service;1"]. > + createInstance().QueryInterface(Ci.nsITaggingService); You can just use `PlacesUtils.tagging` instead of `tagsvc`. @@ +498,5 @@ > + await check_keyword(false, "http://example.com/", "example"); > + let fc = await foreign_count("http://example.com/"); > + > + let httpBookmark = await PlacesUtils.bookmarks.insert({ url: "http://example.com/", > + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); Normally we like to wrap these like this: let httpBookmark = await PlacesUtils.bookmarks.insert({ url: "http://example.com/", parentGuid: PlacesUtils.bookmarks.unfiledGuid }); @@ +513,5 @@ > + await PlacesUtils.bookmarks.remove(httpBookmark); > + > + // the notification is synchronous but the removal process is async. > + // Unfortunately there's nothing explicit we can wait for. > + while(await PlacesUtils.bookmarks.fetch({ url: "http://example.com/"})); This looks like it should be waiting for the foreign_count to have been updated. It would also be better to use TestUtils.waitForCondition() as well now (the code you copied this from is probably from before that was created).

Thanks for the feedback, Mark! I'll apply your fixes and send a new patch later today.

I've added a new patch, and I ran the previous and my revised query on my own places.sqlite as a thumb-to-the-wind benchmark: [9:23:25] rob@eridanus /tmp $ sqlite3 /home/rob/.mozilla/firefox/*.default/places.sqlite '.read before.sql' -- Loading resources from /home/rob/.sqliterc <snip> Run Time: real 0.002 user 0.001933 sys 0.000000 [9:23:30] rob@eridanus /tmp $ sqlite3 /home/rob/.mozilla/firefox/*.default/places.sqlite '.read after.sql' -- Loading resources from /home/rob/.sqliterc <snip> Run Time: real 0.008 user 0.006696 sys 0.002061 So my new query is definitely slower - whether it's too slow to include is up to you all!

I suppose an alternative could be to make bookmarks inserted for tagging purposes not count against a place's foreign_count, but as I'm a newbie who's unfamiliar with the codebase as a whole that might cause issues I haven't forseen. Also, it would require running a query to fix up existing users' places.sqlite files upon upgrade, which could be dicey.

This happens because tags are removed when the bookmarks API notifies onItemRemoved, that happens AFTER it asks keywords to cleanup themselves. So basically we try to cleanup keywords before tags. It makes sense to do that because we are in a transaction, and the problem will naturally disappear once we change the tags implementation in Bug 424160.

Comment on attachment 8985613 [details] [diff] [review] Patch with test and fix incorporating Mark's feedback Review of attachment 8985613 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesUtils.jsm @@ +2496,5 @@ > + UNION > + SELECT fst.* FROM moz_bookmarks AS fst > + INNER JOIN untagged_bookmarks AS snd > + ON fst.parent = snd.id > + WHERE fst.id <> ${PlacesUtils.tagsFolderId} ) This is pretty much selecting all the bookmarks, that in a small profile is likely quick, but in a profile with some thousands bookmarks is not the best option. I'd rather do the opposite, count tagged bookmarks for the given id, sum that to the number of keywords, and you can tell whether there are only keywords and tags. Something like this should work: SELECT h.id, h.url FROM moz_places h JOIN moz_keywords k ON k.place_id = h.id GROUP BY h.id HAVING h.foreign_count = count(*) + (SELECT count(*) FROM moz_bookmarks b JOIN moz_bookmarks p ON b.parent = p.id WHERE p.parent = :tags_root AND b.fk = h.id) Also, please use a binded param, don't build SQL strings manually

Thanks for the review, Marcos! I uploaded a new patch with your recommendations.

Comment on attachment 8985737 [details] [diff] [review] Patch incorporating SQL performance and safety feedback from Marco Review of attachment 8985737 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, it looks great, I just ran the test locally to confirm it. This kind of problems will go away naturally when we move tags away from the bookmarks table.

Comment on attachment 8986198 [details] Bug 1468104 - Existence of tag on a bookmark prevents deletion of associated keyword. https://reviewboard.mozilla.org/r/251612/#review257938

Comment on attachment 8985737 [details] [diff] [review] Patch incorporating SQL performance and safety feedback from Marco Converted the patch to mozreview to simplify landing.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/e717472a6ac6 Existence of tag on a bookmark prevents deletion of associated keyword. r=mak

I’ve reproduced this issue on Firefox 62.0a1 (20180610100220) under Ubuntu 16.04 x64. This behavior no longer occurs on Firefox 62.0b7 (20180709172241) under Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.13.