1150678 - Autocomplete produces incorrect results for modified bookmark keyword searches

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Matt Brubeck (:mbrubeck)]

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.

Comment 1

[Alice0775 White]

Pushlog:

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

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

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)

Comment 5

[Matt Brubeck (:mbrubeck)]

(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.

Comment 6

[Marco Bonardo [:mak]]

(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.

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking for 39+ since this is a regression.

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 10

[Marco Bonardo [:mak]]

(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.

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 13

[Marco Bonardo [:mak]]

(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.

Comment 14

[Joni Chan]

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

Comment 15

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Comment 16

[Kate Glazko]

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

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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

Comment 18

[Marco Bonardo [:mak]]

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.

Comment 19

[Marco Bonardo [:mak]]

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

Comment 20

[Sylvestre Ledru [:Sylvestre]]

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).

Comment 22

[Marco Bonardo [:mak]]

Attachment: part 1 - add oldValue to onItemChanged

Comment 23

[Marco Bonardo [:mak]]

Attachment: part2 - actually fix the bug

Comment 24

[Marco Bonardo [:mak]]

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 25

[Tim Taubert [:ttaubert] (inactive)]

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 26

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 27

[Pulsebot]

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

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 29

[Marco Bonardo [:mak]]

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

Comment 30

[Marco Bonardo [:mak]]

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

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/729b6b663f49

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/729b6b663f49

Comment 35

[Marc Bejarano]

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

Comment 36

[Marco Bonardo [:mak]]

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.

Comment 38

[avada]

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

Comment 39

[Caspy7]

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.

Comment 40

[Marco Bonardo [:mak]]

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.

Comment 42

[Marc Bejarano]

(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 44

[Marco Bonardo [:mak]]

Attachment: Bug 1150678 - Changing url of a bookmark with a keyword breaks the keyword forever.

Review commit: https://reviewboard.mozilla.org/r/75324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75324/

Comment 45

[Marco Bonardo [:mak]]

Comment on attachment 8786344 [details]
Bug 1150678 - Changing url of a bookmark with a keyword breaks the keyword forever.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75324/diff/1-2/

Comment 46

[Drew Willcoxon :adw]

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

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/347ed1510d82
Changing url of a bookmark with a keyword breaks the keyword forever. r=adw

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/347ed1510d82

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 54

[Marco Bonardo [:mak]]

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.

Comment 55

[Marco Bonardo [:mak]]

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

Comment 56

[Marco Bonardo [:mak]]

Attachment: aurora patch

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 57

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c62d6e8a7407

Comment 59

[Marco Bonardo [:mak]]

I must remember to update a comment in 51.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

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

Comment 61

[Marco Bonardo [:mak]]

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 62

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788272 [details] [diff] [review]
aurora patch

We decided not to land this in Aurora50.

Comment 64

[avada]

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.

Comment 65

[Marco Bonardo [:mak]]

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

Comment 71

[Aaron Marcuse-Kubitza]

(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?

Comment 72

[Marco Bonardo [:mak]]

(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