Places depends on a buggy behavior of SQLite's GROUP_CONCAT

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Places
P2
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: Mardak)

Tracking

(Depends on: 1 bug, {verified1.9.0.12, verified1.9.1})

Trunk
mozilla1.9.2a1
verified1.9.0.12, verified1.9.1
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)

(Reporter)

Description

9 years ago
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)

Updated

9 years ago
Blocks: 489442
(Reporter)

Comment 1

9 years ago
Created attachment 373923 [details] [diff] [review]
v1.0

Make sure we don't get bookmarks with empty titles.
Attachment #373923 - Flags: review?(dietrich)
(Reporter)

Comment 2

9 years ago
Note: this behavior changed in http://www.sqlite.org/cvstrac/tktview?tn=3806
(Reporter)

Updated

9 years ago
Whiteboard: [needs review dietrich]
(Assignee)

Comment 3

9 years ago
There's another usage of GROUP_CONCAT to get tags for autocomplete in the BOOK_TAG_SQL #define.
(Reporter)

Comment 4

9 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.
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]
(Assignee)

Comment 8

9 years ago
Created attachment 374176 [details] [diff] [review]
v2

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+
(Assignee)

Updated

9 years ago
Depends on: 489852
(Assignee)

Comment 10

9 years ago
Created attachment 374329 [details] [diff] [review]
v2.1

(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

9 years ago
For what it's worth, drh gave me the sql for >= 1
Whiteboard: [has patch][has reviews]
(Assignee)

Comment 12

9 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
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Flags: in-litmus-
Target Milestone: --- → mozilla1.9.2a1
(Reporter)

Updated

9 years ago
Attachment #374329 - Flags: approval1.9.0.12?
(Assignee)

Comment 13

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f7450eab80a2
Keywords: fixed1.9.1
Whiteboard: [has patch][has reviews]

Comment 14

9 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

9 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

9 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

9 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

9 years ago
wouldn't do_check_eq be more sensible than using an if statement to call do_check_true(false)?

Comment 19

9 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

9 years ago
Created attachment 375341 [details] [diff] [review]
Fix up test to work in SeaMonkey

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

9 years ago
Attachment #375341 - Flags: review? → review?(edilee)
(Assignee)

Comment 21

9 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

9 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

9 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.
(Reporter)

Comment 25

9 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.
(Reporter)

Updated

9 years ago
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?
(Reporter)

Comment 27

9 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
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

9 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 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

9 years ago
This needs a branch patch - it doesn't apply now and requires non-trivial changes AFAICT.
(Assignee)

Comment 33

9 years ago
Created attachment 384969 [details] [diff] [review]
patch against fx3 cvs

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

9 years ago
Attachment #384969 - Attachment is obsolete: true
Attachment #384969 - Flags: review?(sdwilsh)
(Assignee)

Comment 34

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