Closed Bug 421180 Opened 12 years ago Closed 12 years ago

When removing bookmarks existing keywords aren't deleted/removed

Categories

(Firefox :: Bookmarks & History, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: whimboo, Assigned: mak)

References

Details

(Keywords: privacy)

Attachments

(1 file, 3 obsolete files)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008030504 Minefield/3.0b5pre ID:2008030504

When keywords are used to quickly access special bookmarks and these bookmarks are deleted from within places the keywords stay within the database and aren't removed.

Steps to reproduce:
1. Bookmark a website and give it a keyword
2. Open this website via the keyword to ensure it is set correctly
3. Delete the bookmark
4. Run the 'vacuum' command on places.sqlite
5. Run 'select * from moz_keywords'

Even the bookmark is completely deleted the keyword stays in the database. Even it is not visible it is still an privacy issue. Asking for blocking-firefox3.
Flags: blocking-firefox3?
The keywords alone don't seem like they'd represent any private data, but maybe my tinfoil hat is slipping... it doesn't preserve any data about the URL itself, just that there was a keyword?
mmh could be a privacy concern if the user uses part of the website name/url as the keyword

hwv we should always clean the db, if the user try to set the same keyword for another bookmark that will silently fail because we have a unique index there
Attached patch patch (obsolete) — Splinter Review
a quick turn-around, this deletes the keywords on item removal.
As follow-ups we should support transaction to restore them on undo.

we should at least delete them, for this STRs:
- create bookmark abc
- assign keyword "a" to the bookmark abc
- remove the bookmark abc
- create a new bookmark def
- assign keyword "a" to the bookmark def

this should actually fail because we already have that keyword in the db and the unique index does not allow creating a new one with the same name

Still i don't understand the actual db design for keywords, a single bookmark can only have 1 keyword, and keyword name is unique. So why a separate table?! they should live in the bookmark row, with a unique index on keyword column.
Attachment #307711 - Flags: review?(dietrich)
(In reply to comment #3)
> Still i don't understand the actual db design for keywords, a single bookmark
> can only have 1 keyword, and keyword name is unique. So why a separate table?!
> they should live in the bookmark row, with a unique index on keyword column.
According to good database design theory, you shouldn't have columns that can have null data.  In that sense, this design makes sense.
(In reply to comment #4)
> (In reply to comment #3)
> According to good database design theory, you shouldn't have columns that can
> have null data.  In that sense, this design makes sense.

so instead of having a column that could be "keyword or null" you have a column that could be "id or null" and an additional table that does not remap anything since it's 1:1...

so wait, the bookmark table stores the id of the keyword table entry?  If so, yeah that's messed up.  What should be happening is that the keyword table stores the bookmark id (as a foreign key).
(In reply to comment #6)
> so wait, the bookmark table stores the id of the keyword table entry?  If so,
> yeah that's messed up.  What should be happening is that the keyword table
> stores the bookmark id (as a foreign key).

yes, sorry i was not clear :) it should be like you said, if you want to have them in a separate table you should have the itemId in the same table... if not leave them directly into the table... the actual impl is the worst between the 3 choices...

(In reply to comment #3)
> Still i don't understand the actual db design for keywords, a single bookmark
> can only have 1 keyword, and keyword name is unique. So why a separate table?!

While sound, your assumption here is erroneous. Fx2 allows multiple bookmarks to have the same keyword. We didn't want to lose data on import by dropping some, so ended up just leaving the feature as it was for Fx2. Which means the schema allows bookmark to keyword relationship of N:1.

Yes, I agree this is nonsense. I am definitely open to ideas for resolving this. File a followup targeting Fx4 please :)

For this bug, could we add a trigger to bookmark removal that deletes dangling keyword entries?
multiple bookmarks for the same keyword?! how can the ui choose the correct one? 

Yes we could usea trigger, but delete here does not appear a so complex problem like sync...
if needed we can add a trigger after bug 416313 lands in the new CreateTrigger. A BEFORE DELETE should be fine.
Depends on: 416313
Assignee: nobody → mak77
(In reply to comment #9)
> multiple bookmarks for the same keyword?! how can the ui choose the correct
> one? 
Bug 392143 will fix bug 249468 by showing all keywords matches. So it'll provide extra functionality if someone wants to define multiple keywords and pick which one to use from the drop down.
We want to do something like..

DELETE these:

SELECT * FROM moz_keywords k
LEFT OUTER JOIN moz_bookmarks b ON b.keyword_id = k.id
WHERE b.id IS NULL
yes, we can cleanup old undeleted keywords on idle. normal cleanup will be on trigger.

This will not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
Attachment #307711 - Flags: review?(dietrich)
Beltzner, due to the privacy issue could this bug be added to the wanted1.9 list?
i'd prefer having this blocking as a P3 or P4, having a clean DB should not be an optional choice, and this is an easy and safe fix with
Could someone make sure what is in comment#12 and comment#13 gets implemented? You all have a better idea if opening a new bug is the thing to do.

One reason: Extension(NewsFox) needs to share data(0.2Kb-5Kb) between machines to stay synchronized.  The easiest(AFAIK) current method is to put the data in a bookmark and use a bookmark syncer.  Current bookmark syncers only sync title, uri, tags, and keywords.  Annotations are not synced including bookmark description(see for example bug#425875). Data contains colons, commas, and other characters, so using keyword is the only thing that works easily.  NewsFox changes the keyword on feed update.  A user(https://www.mozdev.org/bugs/show_bug.cgi?id=18860) has reported Firefox3betaX becoming unusable within two weeks of changing keywords.  Disclaimer: This [silly, crazy, whatever you like] use of keywords will stop when a better method is found, or syncers improve.
this is quite easy to fix now, so i'm reasking blocking, polluting db is not a good idea.
Status: NEW → ASSIGNED
Flags: blocking-firefox3- → blocking-firefox3?
Drivers: seconding blocking request - we should get this in for the privacy aspect.
Whiteboard: [swag:1h]
Not a blocker, we can take this on a stability release. If it's easy to do, we'd consider a safe and tested patch.
Flags: wanted1.9.0.x+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Attached patch patch (obsolete) — Splinter Review
this should be safe enough, with unit test too!
Attachment #307711 - Attachment is obsolete: true
Attachment #315133 - Flags: review?(dietrich)
Flags: in-testsuite?
Whiteboard: [swag:1h] → [has patch]
Whiteboard: [has patch] → [has patch][needs review dietrich]
Drivers: shouldn't this be wanted-firefox3 rather than wanted1.9.0.x??
Flags: blocking-firefox3- → blocking-firefox3?
ops i see "we can take this on a stability release", however since the patch is here and has a unit test i think we could consider it for final
Still not a blocker, but if the patch is ready and safe enough, we will consider the patch if approval is requested,.
Flags: blocking-firefox3? → blocking-firefox3-
Comment on attachment 315133 [details] [diff] [review]
patch


>Index: toolkit/components/places/src/nsNavHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v
>retrieving revision 1.289
>diff -u -8 -p -r1.289 nsNavHistory.cpp
>--- toolkit/components/places/src/nsNavHistory.cpp	8 Apr 2008 18:42:54 -0000	1.289
>+++ toolkit/components/places/src/nsNavHistory.cpp	11 Apr 2008 17:05:30 -0000
>@@ -923,19 +923,48 @@ nsNavHistory::CreateTriggers()
>       "WHEN OLD.visit_type NOT IN (0,4,7) "
>       "BEGIN "
>         "UPDATE moz_places SET visit_count = visit_count - 1 "
>         "WHERE moz_places.id = OLD.place_id AND visit_count > 0; "
>       "END"));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     rv = createTriggersTransaction.Commit();
>+    NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>+  // we are creating 1 trigger on moz_bookmarks to remove no more used keywords

s/no more used/unused/

>+    // Remove dangling keywords.
>+    // we must remove old keywords that has not been deleted with bookmarks.
>+    // See bug 421180 for details.
>+    // XXX REMOVE ME LATE AFTER FINAL
>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+        "DELETE FROM moz_keywords WHERE id IN ("
>+          "SELECT k.id FROM moz_keywords k "
>+          "LEFT OUTER JOIN moz_bookmarks b ON b.keyword_id = k.id "
>+          "WHERE b.id IS NULL)"));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+

hm, we instead do this when the trigger is created? then it would only ever run once. perf should be ok, since there's likely not many keywords.

nit: s/has/have/
Attached patch patch (obsolete) — Splinter Review
fixed comments, moved migration code on trigger creation (good point, at this stage that will not be a problem).
Attachment #315133 - Attachment is obsolete: true
Attachment #318171 - Flags: review?(dietrich)
Attachment #315133 - Flags: review?(dietrich)
Attachment #318171 - Flags: review?(dietrich) → review+
Attached patch patchSplinter Review
oops, forgot the unit test
Attachment #318171 - Attachment is obsolete: true
Attachment #318173 - Flags: review?(dietrich)
Comment on attachment 318173 [details] [diff] [review]
patch

bring on review since the unit test was already there in previous version. 
asking approval for this keywords database cleanup fix
Attachment #318173 - Flags: review?(dietrich)
Attachment #318173 - Flags: review+
Attachment #318173 - Flags: approval1.9?
Whiteboard: [has patch][needs review dietrich] → [has patch][has reviews]
Comment on attachment 318173 [details] [diff] [review]
patch

a1.9+=damons
Attachment #318173 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
mozilla/toolkit/components/places/tests/unit/test_421180.js 	1.1
mozilla/toolkit/components/places/src/nsNavHistory.cpp 	1.300 
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: --- → Firefox 3
Verified with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008043004 Minefield/3.0pre
and 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008043007 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Flags: wanted1.9.0.x+
Flags: in-testsuite? → in-testsuite+
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.