Steps to reproduce: 1) Save a bookmark with URL "http://example.com/a/%s" and keyword "foo". 2) Edit the bookmark to change the URL to "http://example.com/b/%s" and the keyword to "bar". 3) Type "bar x" in the URL bar and press enter. 4) Type "foo x" in the URL bar and press enter. Expected results: Step 3 should use the bookmark keyword and go to example.com/b/x. Step 4 should not use the bookmark keyword. Actual results: Both step 3 and step 4 go to http://example.com/a/x (the bookmark's original URL, not the current). I first noticed this in a recent Firefox Nightly 40.0a1 build on Linux. I don't know when this bug first appeared, but it's still present in today's Nightly. Maybe caused by bug 1125115 and related work. [Tracking Requested - why for this release]: Possible regression in Fx40 or 39.

Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=cdec6c4e2995&tochange=296d8da26609

[Tracking Requested - why for this release]:

ugh, yes the problem is that keywords are now bound to uris, not to bookmarks, but the UI is still the same (basically now you don't need anymore to create a bookmark to have a keyword, but there's no UI to do that). We likely need to handle an onItemChanged notification and move the keyword to the new uri, until we unify the keywords UIs somehow. I think the cases where the user needs to change the url are rare, so probably not the highest priority atm.

that is, I'd personally not track it cause it's likely to affect a very very tiny percentage of users who are very able to workaround the problem (by creating a new bookmark)

(In reply to Marco Bonardo [::mak] from comment #4) > that is, I'd personally not track it cause it's likely to affect a very very > tiny percentage of users who are very able to workaround the problem (by > creating a new bookmark) Note, this workaround doesn't cover all cases: there's no way to *delete* a keyword. Deleting the bookmark has no effect on the keyword.

(In reply to Matt Brubeck (:mbrubeck) from comment #5) > Note, this workaround doesn't cover all cases: there's no way to *delete* a > keyword. Deleting the bookmark has no effect on the keyword. yes we don't have a ui to remove keywords too. but setting the same keyword to another uri will overwrite it.

oh and I should also add that removing a bookmark that has a uri pointing to a keyword, will also remove the keyword if that was the last bookmark for that uri.

Tracking for 39+ since this is a regression.

Nomming this for the Firefox backlog. mak, even though this may be an edge case, it's still a regression we haven't shipped yet. Do you have a sense of what it may take to fix this? Thanks.

(In reply to Liz Henry (:lizzard) from comment #9) > Nomming this for the Firefox backlog. > > mak, even though this may be an edge case, it's still a regression we > haven't shipped yet. Do you have a sense of what it may take to fix this? I guess we could handle bookmarks url change notifications. It's not going to be trivial, but should be feasible. Though, to fix this properly, we'd need to remove old APIs and provide a new UI, that I don't see us doing very soon. I'm ok with the backlog, I'm not sure I agree with the tracking.

I'm going by release management's guidelines here: https://wiki.mozilla.org/Release_Management/Tracking_rules#Regression While we track all known regressions, they don't necessarily block a release. Engineering teams often decide to wontfix tracked bugs. I get that this may not be a priority to fix in 39 compared to other issues, especially given that the work to do it looks nontrivial.

Actually, mak or Matt, could you write up a clearer description of the workaround. If we can describe that and put it up on SUMO that may help people who run into this issue for now. Joni can you help us get together a workaround description and SUMO page? Thanks!

(In reply to Liz Henry (:lizzard) from comment #12) > Actually, mak or Matt, could you write up a clearer description of the > workaround. If we can describe that and put it up on SUMO that may help > people who run into this issue for now. > > Joni can you help us get together a workaround description and SUMO page? > Thanks! I don't think there's a "simple" workaround we can explain to users, the intended way to use keywords is through the "Add keyword for this search" context menu item and it's unlikely people would hit this bug that way. I think it's also a rarely used feature among non-technical users, and even more rare is the need to change the url for a keyword, I don't think it's even worth a sumo page. To remove keywords, until we have a dedicated UI in the Library, they must remove all bookmarks for the given url (visiting the given url and opening the star button panel will show a button to do that). I will try to find a solution asap, and in case we can backport it to Aurora.

Clearing need-info as there doesn't seem to be a need for a SUMO page at this point.

At this point we likely won't take this for beta 39. We could still uplift a fix to aurora though.

Assigning Mak because every tracked bug should have an assignee. Feel free to reassign someone else as necessary.

It doesn't seem that it is critical enough to track. Untracking it.

So we are tracking this for 39 but not for 40? I still think it should not be tracked at all, honestly. Moreover I don't think I have any resource to work on this immediately, since it's not critical. If you think this is critical, then I have to drop some other priorities.

ehr, I guess Sylvestre may be on PTO, so let's add another ni?

I didn't touch 39 because it was already tagged as wontfix... (which means that we used to track it for 39 but decided not to do anything about it).

Note, this is the part we cannot uplift since it involved idl changes. This should land before the merge, if possible. A Try run is ongoing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b94795ea6f2 I also told you I was planning to add a schema migration to remove orphan keywords, but I'm moving that to bug 1181366. Hopefully that can be uplifted since doesn't involve idl and "should" be trivial.

Comment on attachment 8643591 [details] [diff] [review] part 1 - add oldValue to onItemChanged Review of attachment 8643591 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Bookmarks.jsm @@ +177,5 @@ > let itemId = yield PlacesUtils.promiseItemId(item.guid); > notify(observers, "onItemAdded", [ itemId, parent._id, item.index, > item.type, uri, item.title || null, > toPRTime(item.dateAdded), item.guid, > + item.parentGuid, "" ]); Do we need to do that for onItemAdded? @@ +353,5 @@ > notify(observers, "onItemMoved", [ updatedItem._id, item._parentId, > item.index, updatedItem._parentId, > updatedItem.index, updatedItem.type, > updatedItem.guid, item.parentGuid, > + updatedItem.parentGuid, "" ]); Do we need to do that for onItemMoved? ::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js @@ +26,2 @@ > onEndUpdateBatch: function onEndUpdateBatch() > + this.validate("onEndUpdateBatch", arguments), While you're here you could switch to use real function definitions and help with bug 1083458.

Comment on attachment 8643592 [details] [diff] [review] part2 - actually fix the bug Review of attachment 8643592 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! ::: toolkit/components/places/PlacesUtils.jsm @@ +2362,5 @@ > + }, > + > + _onKeywordChanged: Task.async(function* (guid, keyword) { > + let bookmark = yield PlacesUtils.bookmarks.fetch(guid); > + // By this time the bookmark could have gone, there's nothing we can do. Nit: could be gone

https://hg.mozilla.org/integration/fx-team/rev/9f1f1e11ca37 https://hg.mozilla.org/integration/fx-team/rev/1c24d237080c

Backed out for xpcshell bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=4091946&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/6498bb256c33

Sorry, this requires IDL changes and a complex patch and thus it's not upliftable.

I'm re-landing the idl change part only, that otherwise could not be uplifted. I'll investigate the failure in the next days, regardless it's unrelated to the idl change. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bb324298d21

marco: any luck with the failure investigation you mentioned on 8/6?

The matter is a little bit more complex than expected, cause the fix introduces event loop spinning due to us not yet moving to async PlacesTransaction. So I wonder if the fix is worth it compared to the future plan for merging keywords with search keywords. That said, I've been busy with higher priorities, I hope to get back at this very soon.

Jesus, I still have to mess around with deleting/recreating bookmarks, cleaning places, restarting. Just to change the url of a keyworded search...

Can we get a status update on this? I've been troubled by this and have gotten yet another report from a [confused and frustrated] user.

which kind update do you want? I didn't have the time to work on this yet, there are other regressions that affect all of our users, rather than just the 1% using keywords, so clearly the priority shifted to those. I don't like the current patch and making a proper fix requires lots of time and completing other projects (async transactions). I was planning to make a simple add-on to provide an alternative ui to manage keywords in the meanwhile, but didn't have the time yet.

(In reply to Marco Bonardo [::mak] from comment #40) > making a proper fix requires [...] completing other projects (async transactions) what is the bug number for that?

Comment on attachment 8786344 [details] Bug 1150678 - Changing url of a bookmark with a keyword breaks the keyword forever. https://reviewboard.mozilla.org/r/75324/#review74262 LGTM

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/347ed1510d82 Changing url of a bookmark with a keyword breaks the keyword forever. r=adw

Marco, do you think we can get it uplifted at least to aurora? For beta the fix seems to be too late given that we merge to release today.

I usually don't uplift patches with schema changes... but in this case the change is only a cleanup, so I think we could do it.

Need to check if the patch applies cleanly to Aurora and fix a comment pointing to v51, before asking for approval.

Approval Request Comment [Feature/regressing bug #]: new keywords API [User impact if declined]: changing url of a keyword or keyword of a bookmark may break the keyword forever [Describe test coverage new/current, TreeHerder]: unit test, m-c [Risks and why]: The touched cost mostly deals with keywords, while there may be regression risk, the current situation is far less acceptable. [String/UUID change made/needed]: none

Comment on attachment 8788272 [details] [diff] [review] aurora patch This seems like a pretty big change but the regression also seems pretty core and basic. Given that 50 still have several weeks of stabilization before it hits the release, this is worth uplifting to Aurora.

I must remember to update a comment in 51.

Backed out for Windows opt test_bookmark_store.js failures. https://treeherder.mozilla.org/logviewer.html#?job_id=3491886&repo=mozilla-aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/3a0fd1bb116a

My try push was linux only :( This is a failure in Sync that is not in central, likely due to the async behavior that is better managed in central. I guess we won't uplift this, sorry, it would take too much resources to figure out that failure for a small benefit.

Comment on attachment 8788272 [details] [diff] [review] aurora patch We decided not to land this in Aurora50.

Hi! I believe I found a serious bug with this. If you set a conflicting keyword, the old bookmark becomes an invalid bookmark somehow. The properties windows doesn't show anything, but an "order by name" checkbox which doesn't even belong there. On hover either nothing is shown or the previously hovered bookmarks's url. Left click, middle click does nothing, deleting doesn't work. At first I was happy to see the when I used a keyword that was already used, it was deleted from the old bookmark. So it didn't happen immediately, but now all of those bookmarks are broken.

please file a bug and post steps to reproduce, I'll see if I can reproduce your issue.

(In reply to Marco Bonardo [::mak] from comment #23) > Created attachment 8643592 [details] [diff] [review] > part2 - actually fix the bug This actually breaks the keyword feature worse than before the fix, could we get this reverted? It was working fine in v49.0 but is broken in v53.0.2 (the patch was applied in v51), and the steps to reproduce it are: 1. rename the keywords of 2 keyworded bookmarks 2. add 2 new bookmarks and set their keywords to the old keywords of the step-1 bookmarks 3. notice that when you add a keyword to the 2nd bookmark, the keyword in the 1st one gets deleted. attempting to add it back to the 1st one either deletes the keyword in the 2nd bookmark, or in an unrelated bookmark in the same parent bookmarks folder. since unrelated bookmarks in the same parent bookmarks folder are affected, perhaps the parentId in some of the patch's function calls is involved?

(In reply to Aaron Marcuse-Kubitza from comment #71) > 3. notice that when you add a keyword to the 2nd bookmark, the keyword in > the 1st one gets deleted. Bug 1343256 is fixed in 54 and ESR52