Autocomplete produces incorrect results for modified bookmark keyword searches

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mbrubeck, Assigned: mak)

Tracking

(Blocks 1 bug, {regression, reproducible})

40 Branch
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox38 unaffected, firefox39+ wontfix, firefox40- wontfix, firefox41+ wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
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 2

4 years ago
[Tracking Requested - why for this release]:

Updated

4 years ago

Updated

4 years ago
Blocks: 1125113
No longer blocks: 1125115
Assignee

Comment 3

4 years ago
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.
Assignee

Comment 4

4 years ago
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)
Reporter

Comment 5

4 years ago
(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.
Assignee

Comment 6

4 years ago
(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.
Assignee

Comment 7

4 years ago
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.
Flags: needinfo?(mak77)
Flags: firefox-backlog?
Assignee

Comment 10

4 years ago
(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.
Flags: needinfo?(mak77)
Flags: firefox-backlog?
Flags: firefox-backlog+
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!
Flags: needinfo?(mak77)
Flags: needinfo?(jsavage)
Assignee

Comment 13

4 years ago
(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.
Flags: needinfo?(mak77)
Clearing need-info as there doesn't seem to be a need for a SUMO page at this point.
Flags: needinfo?(jsavage)
At this point we likely won't take this for beta 39. 
We could still uplift a fix to aurora though.

Comment 16

4 years ago
Assigning Mak because every tracked bug should have an assignee. Feel free to reassign someone else as necessary.
Assignee: nobody → mak77
It doesn't seem that it is critical enough to track. Untracking it.
Assignee

Comment 18

4 years ago
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.
Flags: needinfo?(sledru)
Assignee

Comment 19

4 years ago
ehr, I guess Sylvestre may be on PTO, so let's add another ni?
Flags: needinfo?(lhenry)
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).
Flags: needinfo?(sledru)
Flags: needinfo?(lhenry)
Assignee

Updated

4 years ago
Duplicate of this bug: 1187644
Assignee

Updated

4 years ago
Blocks: 1179076
Assignee

Updated

4 years ago
Blocks: 1181366
Assignee

Comment 22

4 years ago
Attachment #8643591 - Flags: review?(ttaubert)
Assignee

Comment 23

4 years ago
Posted patch part2 - actually fix the bug (obsolete) — Splinter Review
Attachment #8643592 - Flags: review?(ttaubert)
Assignee

Comment 24

4 years ago
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.
Attachment #8643591 - Flags: review?(ttaubert) → review+
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
Attachment #8643592 - Flags: review?(ttaubert) → review+
Assignee

Updated

4 years ago
Flags: in-testsuite+
Assignee

Updated

4 years ago
Blocks: 1191716
Assignee

Comment 29

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

Comment 30

4 years ago
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
Keywords: leave-open
Assignee

Updated

4 years ago
Duplicate of this bug: 1198294
Assignee

Updated

4 years ago
Priority: -- → P1

Updated

4 years ago
Blocks: 1211427

Updated

4 years ago
Duplicate of this bug: 1219808
Assignee

Updated

4 years ago
Blocks: 1228004

Comment 35

4 years ago
marco: any luck with the failure investigation you mentioned on 8/6?
Flags: needinfo?(mak77)
Assignee

Comment 36

4 years ago
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.
Flags: needinfo?(mak77)

Updated

4 years ago
Duplicate of this bug: 1230792

Comment 38

3 years ago
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

3 years ago
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.
Flags: needinfo?(mak77)
Assignee

Comment 40

3 years ago
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.
Flags: needinfo?(mak77)
Assignee

Updated

3 years ago
Duplicate of this bug: 1240756

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 42

3 years ago
(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?

Updated

3 years ago
Duplicate of this bug: 1251998
Assignee

Updated

3 years ago
Blocks: 1285142

Updated

3 years ago
Blocks: 1272227
Assignee

Updated

3 years ago
Attachment #8643592 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8643591 - Flags: checkin+
Comment hidden (mozreview-request)

Comment 46

3 years ago
mozreview-review
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
Attachment #8786344 - Flags: review?(adw) → review+

Comment 47

3 years ago
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
Assignee

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee

Updated

3 years ago
Duplicate of this bug: 1285142

Updated

3 years ago
Blocks: 1257549
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.
Flags: needinfo?(mak77)
No longer blocks: 1257549
Duplicate of this bug: 1257549
Duplicate of this bug: 1272227
Assignee

Updated

3 years ago
Duplicate of this bug: 1191716
Assignee

Comment 54

3 years ago
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.
Flags: needinfo?(mak77)
Assignee

Comment 55

3 years ago
Need to check if the patch applies cleanly to Aurora and fix a comment pointing to v51, before asking for approval.
Flags: needinfo?(mak77)
Assignee

Comment 56

3 years ago
Posted patch aurora patchSplinter Review
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
Flags: needinfo?(mak77)
Attachment #8788272 - Flags: approval-mozilla-aurora?
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.
Attachment #8788272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 59

3 years ago
I must remember to update a comment in 51.
Flags: needinfo?(mak77)
Assignee

Comment 61

3 years ago
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.
Flags: needinfo?(mak77)
Comment on attachment 8788272 [details] [diff] [review]
aurora patch

We decided not to land this in Aurora50.
Attachment #8788272 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
Assignee

Updated

3 years ago
Duplicate of this bug: 1179076

Comment 64

3 years ago
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.
Flags: needinfo?(mak77)
Assignee

Comment 65

3 years ago
please file a bug and post steps to reproduce, I'll see if I can reproduce your issue.
Flags: needinfo?(mak77)

Updated

3 years ago
Depends on: 1302770
Assignee

Updated

3 years ago
Duplicate of this bug: 1202398
Assignee

Updated

3 years ago
Duplicate of this bug: 1290683

Updated

3 years ago
Depends on: 1314013
Assignee

Updated

3 years ago
Duplicate of this bug: 1265309
Assignee

Updated

3 years ago
Duplicate of this bug: 1320554
Assignee

Updated

2 years ago
Duplicate of this bug: 1332305

Updated

2 years ago
Depends on: 1343256
(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?
Assignee

Comment 72

2 years ago
(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
You need to log in before you can comment on or make changes to this bug.