Places depends on a buggy behavior of SQLite's GROUP_CONCAT

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
P2
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: Mardak)

Tracking

({verified1.9.0.12, verified1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
wanted1.9.0.x ?
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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?
Blocks: 489442
Posted patch v1.0 (obsolete) — Splinter Review
Make sure we don't get bookmarks with empty titles.
Attachment #373923 - Flags: review?(dietrich)
Note: this behavior changed in http://www.sqlite.org/cvstrac/tktview?tn=3806
Whiteboard: [needs review dietrich]
There's another usage of GROUP_CONCAT to get tags for autocomplete in the BOOK_TAG_SQL #define.
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.
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.
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!
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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)
Whiteboard: [needs review dietrich]
Posted patch v2 (obsolete) — Splinter Review
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 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+
Depends on: 489852
Posted patch v2.1Splinter Review
(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
For what it's worth, drh gave me the sql for >= 1
Whiteboard: [has patch][has reviews]
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: 10 years ago
Resolution: --- → FIXED
Flags: in-litmus-
Target Milestone: --- → mozilla1.9.2a1
Attachment #374329 - Flags: approval1.9.0.12?
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f7450eab80a2
Keywords: fixed1.9.1
Whiteboard: [has patch][has reviews]
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
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.
(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?
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..
wouldn't do_check_eq be more sensible than using an if statement to call do_check_true(false)?
(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...
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?
Attachment #375341 - Flags: review? → review?(edilee)
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+
If the check fails, you don't need to call do_test_finished, so I'm not sure why that matters.
Oh, I guess I was thinking that the test runs until it hits a do_test_finished. Or maybe those are mochi tests.
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.
Blocks: 493560
(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?
SQLite 3.6.14 had a bug that we were tickling.  This if fixed.
Marking verified fixed based on passed tests, no backouts and comment 27.
Status: RESOLVED → VERIFIED
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?
(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 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+
This needs a branch patch - it doesn't apply now and requires non-trivial changes AFAICT.
Posted patch patch against fx3 cvs (obsolete) — Splinter Review
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)
Attachment #384969 - Attachment is obsolete: true
Attachment #384969 - Flags: review?(sdwilsh)
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
Marking verified for 1.9.0.12 based on passing unit test.
You need to log in before you can comment on or make changes to this bug.