Closed
Bug 489443
Opened 15 years ago
Closed 15 years ago
Places depends on a buggy behavior of SQLite's GROUP_CONCAT
Categories
(Toolkit :: Places, defect, P2)
Toolkit
Places
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: sdwilsh, Assigned: Mardak)
References
Details
(Keywords: verified1.9.0.12, verified1.9.1)
Attachments
(2 files, 3 obsolete files)
8.68 KB,
patch
|
dveditz
:
approval1.9.0.12+
|
Details | Diff | Splinter Review |
795 bytes,
patch
|
Mardak
:
review+
|
Details | Diff | Splinter Review |
After SQLite 3.6.13, GROUP_CONCAT will concatenate empty strings. Our tag query is wrong, and needs to be updated so it doesn't break in future releases. Our unit tests fail with the newer SQLite. Because linux distributions can ship with newer SQLite's than what we ship with, we need to fix this on both branches too.
Flags: wanted1.9.0.x?
Flags: in-testsuite+
Flags: blocking1.9.1?
Reporter | ||
Comment 1•15 years ago
|
||
Make sure we don't get bookmarks with empty titles.
Attachment #373923 -
Flags: review?(dietrich)
Reporter | ||
Comment 2•15 years ago
|
||
Note: this behavior changed in http://www.sqlite.org/cvstrac/tktview?tn=3806
Reporter | ||
Updated•15 years ago
|
Whiteboard: [needs review dietrich]
Assignee | ||
Comment 3•15 years ago
|
||
There's another usage of GROUP_CONCAT to get tags for autocomplete in the BOOK_TAG_SQL #define.
Reporter | ||
Comment 4•15 years ago
|
||
Right, but I think our query is correct (at least, I think so, but that code is an obfuscated mess). A number of the autocomplete unit tests fail without this change, but pass with it.
Comment 5•15 years ago
|
||
imo Edward is correct, you should also fix SQL_STR_FRAGMENT_GET_BOOK_TAG, check if name is "tags" and in case add the condition on t.title.
Comment 6•15 years ago
|
||
and i'm not sure we have a test that checks if in the locationbar, with empty tags, we could return "tag1,,,,tag2". We should not even allow tagging with an empty name really, that is Bug 458026, and looks like it is still mine!
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment 7•15 years ago
|
||
Comment on attachment 373923 [details] [diff] [review] v1.0 canceling review request. from the comments, it's pretty clear that either the sql needs to be fixed, or conclusively confirmed to be ok.
Attachment #373923 -
Flags: review?(dietrich)
Updated•15 years ago
|
Whiteboard: [needs review dietrich]
Assignee | ||
Comment 8•15 years ago
|
||
Don't bother GROUP_CONCATing tag titles that are empty. Restructure the BOOK_TAG_SQL #define to be somewhat more readable. Test by adding tags to a uri then renaming the tag to empty and making sure they don't show up in autocmoplete results.
Assignee: sdwilsh → edilee
Attachment #373923 -
Attachment is obsolete: true
Attachment #374176 -
Flags: review?(dietrich)
Comment 9•15 years ago
|
||
Comment on attachment 374176 [details] [diff] [review] v2 >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >--- a/toolkit/components/places/src/nsNavHistory.cpp >+++ b/toolkit/components/places/src/nsNavHistory.cpp >@@ -1227,16 +1227,17 @@ nsNavHistory::InitStatements() > rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( > "/* do not warn (bug 487594) */ " > "SELECT GROUP_CONCAT(tag_title, ?1) FROM (" > "SELECT t.title AS tag_title " > "FROM moz_bookmarks b " > "JOIN moz_bookmarks t ON t.id = b.parent " > "WHERE b.fk = IFNULL((SELECT id FROM moz_places_temp WHERE url = ?3), " > "(SELECT id FROM moz_places WHERE url = ?3)) " >+ "AND LENGTH(t.title) >= 1 " > "AND b.type = ") + > nsPrintfCString("%d", nsINavBookmarksService::TYPE_BOOKMARK) + > NS_LITERAL_CSTRING(" AND t.parent = ?2 ORDER BY t.title)"), > getter_AddRefs(mDBGetTags)); > NS_ENSURE_SUCCESS(rv, rv); would " > 0 " save a nanosecond or two? >+ (function doSearch(query) { >+ print("Searching for: " + query); >+ ac.startSearch(query, "", null, { >+ onSearchResult: function(search, result) { >+ print("Got results with status: " + result.searchResult); >+ if (result.searchResult != result.RESULT_SUCCESS) >+ return; both searches expect results, so wouldn't this be a failure? >+ >+ let comment = result.getCommentAt(0); >+ print("Got the title/tags: " + comment); >+ >+ if (comment.indexOf("\u2013") == -1) { anytime this hack of interminable sadness is used, please link back to the bug #. is there a followup bug against autocomplete to provide some extensibility mechanism for extra data per result? >+ print("No tags in result, so making sure we expect no tags"); >+ do_check_eq(tagIds.length, 0); >+ do_test_finished(); >+ return; >+ } >+ >+ print("Making sure we get the expected number of tags: " + tagIds.length); >+ do_check_eq(comment.split(",").length, tagIds.length); >+ >+ print("Setting a tag's title to empty"); >+ bm.setItemTitle(tagIds.shift(), ""); note in the print/comment that you're also removing the id of the renamed tag from the id list >+ >+ print("Searching again with fewer tags"); >+ let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); >+ timer.initWithCallback({ notify: function() doSearch(query.slice(1)) }, >+ 0, timer.TYPE_ONE_SHOT); you're modifying the query only to force ac to refetch the results, yes? should make a note of that >+ } >+ }); >+ })("title"); >+ >+ do_test_pending(); >+} aside from writing test code that guarantees anyone else will have to spend extra time figuring out what it actually does, r=me :)
Attachment #374176 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 10•15 years ago
|
||
(In reply to comment #9) > >+ "AND LENGTH(t.title) >= 1 " > would " > 0 " save a nanosecond or two? Switched to > 0. >= 1 might involve a subtraction on platforms.. depending on what SQLite does. > >+ print("Got results with status: " + result.searchResult); > >+ if (result.searchResult != result.RESULT_SUCCESS) > both searches expect results, so wouldn't this be a failure? Both? There's 3 tags, so the test runs 4 times, but it gets a result each time. > >+ if (comment.indexOf("\u2013") == -1) { > is there a followup bug against autocomplete to provide some extensibility > mechanism for extra data per result? Filed bug 489852. > >+ print("Setting a tag's title to empty"); > >+ bm.setItemTitle(tagIds.shift(), ""); > note in the print/comment that you're also removing the id Printed out the current array and that we're removing an item. > >+ timer.initWithCallback({ notify: function() doSearch(query.slice(1)) }, > you're modifying the query only to force ac to refetch the results Yup.. I suppose that's a bug that we use cached results even if the item changes... I suppose slightly more serious is that I needed to do this on a timeout because nsNavHistoryAutoComplete calls onSearchComplete, so we want to avoid starting another query while it's about to finish up? > extra time figuring out what it actually does, r=me :) Added a bunch more "comments" ! :)
Attachment #374176 -
Attachment is obsolete: true
Reporter | ||
Comment 11•15 years ago
|
||
For what it's worth, drh gave me the sql for >= 1
Updated•15 years ago
|
Whiteboard: [has patch][has reviews]
Assignee | ||
Comment 12•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/fe47f4ffb7db Don't bother GROUP_CONCATing tag titles that are empty. Restructure the BOOK_TAG_SQL #define to be somewhat more readable. Test by adding tags to a uri then renaming the tag to empty and making sure they don't show up in autocmoplete results. Waiting to push to 191 when it's not closed.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Flags: in-litmus-
Target Milestone: --- → mozilla1.9.2a1
Reporter | ||
Updated•15 years ago
|
Attachment #374329 -
Flags: approval1.9.0.12?
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f7450eab80a2
Keywords: fixed1.9.1
Whiteboard: [has patch][has reviews]
Comment 14•15 years ago
|
||
This patch landing on 1.9.1 has caused all SeaMonkey test machines to time out on xpcshell tests, and I strongly suspect it's that new test, as the last one that is executed is test_places/unit/test_dynamic_containers.js
Comment 15•15 years ago
|
||
Yes, it's that new test that makes SeaMonkey reproducibly hang. --------------------------------------------------------------- js> _execute_test(); *** test pending Adding 3 tags to the bookmark Tagging uri with tag: 0 WARNING: EndUpdateBatch without a begin: file /mnt/mozilla/hg/comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 4170 Saving bookmark id of tag to rename it later: 7 Tagging uri with tag: 1 Saving bookmark id of tag to rename it later: 9 Tagging uri with tag: 2 Saving bookmark id of tag to rename it later: 11 Search 4 times: make sure we get the right amount of tags then remove one Searching for: title *** test pending *** test finished *** running event loop Got results with status: 5 Got results with status: 3 --------------------------------------------------------------- Here it stops doing anything and just hangs.
Assignee | ||
Comment 16•15 years ago
|
||
(In reply to comment #15) > Got results with status: 5 > Got results with status: 3 > Here it stops doing anything and just hangs. Hrm.. very odd. Status 5 = NOMATCH_ONGOING while 3 is RESULT_NOMATCH. Are there any changes to how places stores history or searches for history in seamonkey?
Assignee | ||
Comment 17•15 years ago
|
||
Comment on attachment 374329 [details] [diff] [review] v2.1 >+ (function doSearch(query) { >+ _("Searching for:", query); >+ ac.startSearch(query, "", null, { >+ onSearchResult: function(search, result) { >+ _("Got results with status:", result.searchResult); if (result.searchResult == result.RESULT_NO_MATCH) { do_check_true(false); do_test_finished(); } >+ if (result.searchResult != result.RESULT_SUCCESS) >+ return; I suppose this wouldn't fix the test hang, but it would at least give a test failure..
Reporter | ||
Comment 18•15 years ago
|
||
wouldn't do_check_eq be more sensible than using an if statement to call do_check_true(false)?
Comment 19•15 years ago
|
||
(In reply to comment #16) > Are there any changes to how places stores history or searches for history in > seamonkey? No, just plain places. And all other places tests work fine, actually - at least after we fixed them to not depend on different default preferences than SeaMonkey has. And thanks for reminding me of that, we could once again be running into a test that doesn't like us to return history only...
Comment 20•15 years ago
|
||
Yes, that's it. Setting the set of prefs we have in the other tests helps to make SeaMonkey pass. And that also explains why it was failing so hard across the board. :)
Attachment #375341 -
Flags: review?
Updated•15 years ago
|
Attachment #375341 -
Flags: review? → review?(edilee)
Assignee | ||
Comment 21•15 years ago
|
||
Comment on attachment 375341 [details] [diff] [review] Fix up test to work in SeaMonkey r=Mardak (In reply to comment #18) > wouldn't do_check_eq be more sensible than using an if statement to call > do_check_true(false)? You could, but you would still need an if to decide to do_test_finished instead of just returning. But I suppose it doesn't matter with this patch ;)
Attachment #375341 -
Flags: review?(edilee) → review+
Reporter | ||
Comment 22•15 years ago
|
||
If the check fails, you don't need to call do_test_finished, so I'm not sure why that matters.
Assignee | ||
Comment 23•15 years ago
|
||
Oh, I guess I was thinking that the test runs until it hits a do_test_finished. Or maybe those are mochi tests.
Comment 24•15 years ago
|
||
Pushed test fixup as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0bbf7e596626 and http://hg.mozilla.org/mozilla-central/rev/50fe1e2ca505
Reporter | ||
Comment 25•15 years ago
|
||
I'm not actually convinced that this fixed us - that, or SQLite still has a bug. I just pushed to the try server with 3.6.14, and this unit test fails, along with a few other places ones.
Comment 26•15 years ago
|
||
(In reply to comment #25) > I'm not actually convinced that this fixed us - that, or SQLite still has a > bug. I just pushed to the try server with 3.6.14, and this unit test fails, > along with a few other places ones. Shawn, any update on that topic?
Reporter | ||
Comment 27•15 years ago
|
||
SQLite 3.6.14 had a bug that we were tickling. This if fixed.
Comment 28•15 years ago
|
||
Marking verified fixed based on passed tests, no backouts and comment 27.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment 29•15 years ago
|
||
3.0.x seems to be using sqlite 3.6.7 -- do we still need this fix on the 1.9.0 branch given comment 27?
Reporter | ||
Comment 30•15 years ago
|
||
(In reply to comment #29) > 3.0.x seems to be using sqlite 3.6.7 -- do we still need this fix on the 1.9.0 > branch given comment 27? see comment 0 - linux distros can use a newer sqlite that will break branch
Comment 31•15 years ago
|
||
Comment on attachment 374329 [details] [diff] [review] v2.1 fair enough. Approved for 1.9.0.12, a=dveditz for release-drivers
Attachment #374329 -
Flags: approval1.9.0.12? → approval1.9.0.12+
Reporter | ||
Comment 32•15 years ago
|
||
This needs a branch patch - it doesn't apply now and requires non-trivial changes AFAICT.
Assignee | ||
Comment 33•15 years ago
|
||
The first patch chunk needed to remap to Firefox 3 version of the query, but it's still a simple change. The second chunk was just too much patch context preventing a clean apply, but the actual change part was simple -- a difference of AS vs "no AS". For the test file, I included the seamonkey stuff.. building right now..
Attachment #384969 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #384969 -
Attachment is obsolete: true
Attachment #384969 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 34•15 years ago
|
||
Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.311; previous revision: 1.310 done Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v <-- nsNavHistoryAutoComplete.cpp new revision: 1.61; previous revision: 1.60 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_empty_tags.js,v done Checking in toolkit/components/places/tests/unit/test_empty_tags.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_empty_tags.js,v <-- test_empty_tags.js initial revision: 1.1 done
Keywords: fixed1.9.0.12
Comment 35•15 years ago
|
||
Marking verified for 1.9.0.12 based on passing unit test.
Keywords: fixed1.9.0.12 → verified1.9.0.12
You need to log in
before you can comment on or make changes to this bug.
Description
•