Autocomplete produces incorrect results for modified bookmark keyword searches

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: mbrubeck, Assigned: mak)

Tracking

(Blocks: 1 bug, {regression, reproducible})

40 Branch
mozilla51
regression, reproducible
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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 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 1

3 years ago
Pushlog:

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

Comment 2

3 years ago
[Tracking Requested - why for this release]:
status-firefox38: --- → unaffected
status-firefox39: ? → affected
tracking-firefox39: --- → ?

Updated

3 years ago
Blocks: 1125115
Keywords: regressionwindow-wanted

Updated

3 years ago
Blocks: 1125113
No longer blocks: 1125115
(Assignee)

Comment 3

3 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.
Blocks: 519514
(Assignee)

Comment 4

3 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

3 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

3 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

3 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.
tracking-firefox39: ? → +
tracking-firefox40: ? → +
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

3 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

3 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.
status-firefox39: affected → wontfix
status-firefox41: --- → affected
tracking-firefox41: --- → +

Comment 16

2 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.
tracking-firefox40: + → -
(Assignee)

Comment 18

2 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

2 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

2 years ago
Duplicate of this bug: 1187644
(Assignee)

Updated

2 years ago
Blocks: 1179076
(Assignee)

Updated

2 years ago
Blocks: 1181366
(Assignee)

Comment 22

2 years ago
Created attachment 8643591 [details] [diff] [review]
part 1 - add oldValue to onItemChanged
Attachment #8643591 - Flags: review?(ttaubert)
(Assignee)

Comment 23

2 years ago
Created attachment 8643592 [details] [diff] [review]
part2 - actually fix the bug
Attachment #8643592 - Flags: review?(ttaubert)
(Assignee)

Comment 24

2 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+

Comment 27

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/9f1f1e11ca37
https://hg.mozilla.org/integration/fx-team/rev/1c24d237080c
(Assignee)

Updated

2 years ago
Flags: in-testsuite+
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
Flags: needinfo?(mak77)
(Assignee)

Updated

2 years ago
Blocks: 1191716
(Assignee)

Comment 29

2 years ago
Sorry, this requires IDL changes and a complex patch and thus it's not upliftable.
status-firefox40: affected → wontfix
status-firefox41: affected → wontfix
Flags: needinfo?(mak77)
(Assignee)

Comment 30

2 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

Comment 31

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/729b6b663f49
https://hg.mozilla.org/mozilla-central/rev/729b6b663f49
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1198294
(Assignee)

Updated

2 years ago
Priority: -- → P1

Updated

2 years ago
Blocks: 1211427

Updated

2 years ago
Duplicate of this bug: 1219808
(Assignee)

Updated

2 years ago
Blocks: 1228004

Comment 35

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

Comment 36

2 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

2 years ago
Duplicate of this bug: 1230792

Comment 38

2 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

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

Updated

2 years ago
Flags: needinfo?(mak77)
(Assignee)

Comment 40

2 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

2 years ago
Duplicate of this bug: 1240756

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 42

2 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

a year ago
Duplicate of this bug: 1251998
(Assignee)

Updated

a year ago
Blocks: 1285142

Updated

a year ago
Blocks: 1272227
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8643592 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8643591 - Flags: checkin+
Comment hidden (mozreview-request)

Comment 46

a year 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

a year 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

Comment 48

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/347ed1510d82
(Assignee)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

a year ago
Duplicate of this bug: 1285142

Updated

a year 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.
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → fixed
Flags: needinfo?(mak77)
No longer blocks: 1257549
Duplicate of this bug: 1257549
Duplicate of this bug: 1272227
(Assignee)

Updated

a year ago
Duplicate of this bug: 1191716
(Assignee)

Comment 54

a year 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.
status-firefox50: affected → wontfix
Flags: needinfo?(mak77)
(Assignee)

Comment 55

a year 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

a year ago
Created attachment 8788272 [details] [diff] [review]
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
Flags: needinfo?(mak77)
Attachment #8788272 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

a year ago
status-firefox49: affected → wontfix
status-firefox50: wontfix → affected
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+

Comment 58

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c62d6e8a7407
status-firefox50: affected → fixed
(Assignee)

Comment 59

a year ago
I must remember to update a comment in 51.
Flags: needinfo?(mak77)
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
status-firefox50: fixed → affected
(Assignee)

Comment 61

a year 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.
status-firefox50: affected → wontfix
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

a year ago
Duplicate of this bug: 1179076

Comment 64

a year 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

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

Updated

a year ago
Depends on: 1302770
(Assignee)

Updated

a year ago
Duplicate of this bug: 1202398
(Assignee)

Updated

a year ago
Duplicate of this bug: 1290683

Updated

a year ago
Depends on: 1314013
(Assignee)

Updated

a year ago
Duplicate of this bug: 1265309
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1320554
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1332305

Updated

8 months 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

5 months 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.