Closed
Bug 421180
Opened 16 years ago
Closed 16 years ago
When removing bookmarks existing keywords aren't deleted/removed
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3
People
(Reporter: whimboo, Assigned: mak)
References
Details
(Keywords: privacy)
Attachments
(1 file, 3 obsolete files)
7.26 KB,
patch
|
mak
:
review+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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?
Comment 1•16 years ago
|
||
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?
Assignee | ||
Comment 2•16 years ago
|
||
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
Assignee | ||
Comment 3•16 years ago
|
||
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)
Comment 4•16 years ago
|
||
(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.
Assignee | ||
Comment 5•16 years ago
|
||
(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...
Comment 6•16 years ago
|
||
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).
Assignee | ||
Comment 7•16 years ago
|
||
(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...
Comment 8•16 years ago
|
||
(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?
Assignee | ||
Comment 9•16 years ago
|
||
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...
Assignee | ||
Comment 10•16 years ago
|
||
if needed we can add a trigger after bug 416313 lands in the new CreateTrigger. A BEFORE DELETE should be fine.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → mak77
Comment 11•16 years ago
|
||
(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.
Comment 12•16 years ago
|
||
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
Assignee | ||
Comment 13•16 years ago
|
||
yes, we can cleanup old undeleted keywords on idle. normal cleanup will be on trigger.
Comment 14•16 years ago
|
||
This will not block the final release of Firefox 3.
Flags: blocking-firefox3? → blocking-firefox3-
Assignee | ||
Updated•16 years ago
|
Attachment #307711 -
Flags: review?(dietrich)
Reporter | ||
Comment 15•16 years ago
|
||
Beltzner, due to the privacy issue could this bug be added to the wanted1.9 list?
Assignee | ||
Comment 16•16 years ago
|
||
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
Comment 17•16 years ago
|
||
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.
Assignee | ||
Comment 18•16 years ago
|
||
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?
Comment 19•16 years ago
|
||
Drivers: seconding blocking request - we should get this in for the privacy aspect.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [swag:1h]
Comment 20•16 years ago
|
||
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-
Assignee | ||
Comment 21•16 years ago
|
||
this should be safe enough, with unit test too!
Attachment #307711 -
Attachment is obsolete: true
Attachment #315133 -
Flags: review?(dietrich)
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite?
Assignee | ||
Updated•16 years ago
|
Whiteboard: [swag:1h] → [has patch]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch] → [has patch][needs review dietrich]
Assignee | ||
Comment 22•16 years ago
|
||
Drivers: shouldn't this be wanted-firefox3 rather than wanted1.9.0.x??
Flags: blocking-firefox3- → blocking-firefox3?
Assignee | ||
Comment 23•16 years ago
|
||
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
Comment 24•16 years ago
|
||
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 25•16 years ago
|
||
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/
Assignee | ||
Comment 26•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #318171 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 27•16 years ago
|
||
oops, forgot the unit test
Attachment #318171 -
Attachment is obsolete: true
Attachment #318173 -
Flags: review?(dietrich)
Assignee | ||
Comment 28•16 years ago
|
||
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?
Assignee | ||
Updated•16 years ago
|
Whiteboard: [has patch][needs review dietrich] → [has patch][has reviews]
Comment 29•16 years ago
|
||
Comment on attachment 318173 [details] [diff] [review] patch a1.9+=damons
Attachment #318173 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 30•16 years ago
|
||
mozilla/toolkit/components/places/tests/unit/test_421180.js 1.1 mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.300
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: --- → Firefox 3
Comment 31•16 years ago
|
||
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
Updated•16 years ago
|
Flags: wanted1.9.0.x+
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 32•15 years ago
|
||
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.
Description
•