494124 - SQL for awesome bar's auto complete uses empty IN condition ("IN ()")

Status: RESOLVED WORKSFORME

(Toolkit :: Places, defect)

Description

[Kouhei Sutou]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; ja; rv:1.9.0.10) Gecko/2009042805 Iceweasel/3.0.9 (Debian-3.0.9-1)
Build Identifier: 

Awesome bar execute a SQL for an auto complete. The SQL sometimes includes empty IN condition, "SELECT ... WHERE url IN () ...". The empty IN condition is invalid SQL.


Reproducible: Sometimes

Steps to Reproduce:
1. clear profile (rm -rf ~/.mozilla/fennec/)
2. type "google.com" into awesome bar as fast as possible ;-)

Actual Results:  
Empty IN condition "IN ()" is included in awesome bar's SQL.
If you build SQLite with "-DSQLITE_DEBUG=1" option, SQLite calls assert() and Fennec is aborted.

Expected Results:  
Don't include empty IN condition.

Comment 1

[Kouhei Sutou]

Attachment: suppress needless IN condition.

Comment 2

[Shawn Wilsher :sdwilsh]

I'd be more interested in finding out why it's empty, instead of blindly patching it.

Comment 3

[Ed Lee :Mardak]

Looking at the control flow.. there would be no bindings if prevMatchCount == 0.. and it would go into that block only if autocomplete is enabled and the previous search didn't finish (and happened to find nothing yet.)

So probably you type something (probably long, so there's no results) then you type another character.

Comment 4

[Kouhei Sutou]

Edward's description is right.

The else closure codes assumes that previous search is in-progress or was finished but it has no result:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#722

      // We either have a previous in-progress search or a finished search that
      // has more than 0 results. We can continue from where the previous
      // search left off, but first we want to create an optimized query that
      // only searches through the urls that were previously found

But this problem case is "previous search is in-progress search and it has no result".

We can meat the problem case if we type more characters before previous search has any results as said by Edward:
(In reply to comment #3)
> So probably you type something (probably long, so there's no results) then you
> type another character.


This case may be occurred on Firefox but I didn't try it yet. Sorry.

Comment 5

[Kouhei Sutou]

I confirm this problem with Firefox (Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2a1pre) Gecko/20090525 Minefield/3.6a1pre) on Linux too.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

looks like this does not need to stay unconfirmed

Comment 7

[Shawn Wilsher :sdwilsh]

This is pretty easy to reproduce with the patch in bug 494826.  Also makes it impossible to land that bug without this being fixed.

This could actually be a reason for us not getting fast results too, since SQLite doesn't choose the best index here.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 378792 [details] [diff] [review]
suppress needless IN condition.

>+      nsCString bindingsConditionWhere;
>+      nsCString bindingsConditionAnd;
use nsCAutoString here please, and do both declarations on one line.

>+      if (!bindings.IsEmpty()) {
>+        nsCString bindingsCondition;
nsCAutoString please

>+        bindingsCondition += NS_LITERAL_CSTRING("url IN (");
You should use AppendASCII on the string instead of appending an NS_LITERAL_CSTRING

>+        bindingsCondition += bindings;
>+        bindingsCondition += NS_LITERAL_CSTRING(")");
ditto

>+
>+        bindingsConditionWhere += NS_LITERAL_CSTRING(" WHERE ");
ditto

>+        bindingsConditionWhere += bindingsCondition;
>+
>+        bindingsConditionAnd += NS_LITERAL_CSTRING(" AND ");
ditto

r=sdwilsh with these fixed.  Please upload a new patch and have edilee@gmail review it.

Comment 9

[Kouhei Sutou]

Attachment: use nsCAutoString instead of nsCString

Comment 10

[Kouhei Sutou]

In the same file, other nsCAutoString use Append() instead of AppendASCII().
e.g. AutoCompleteStatementCallbackNotifier::HandleError()

Should we use Append() instead of AppendASCII() in the patch too?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Should we use Append() instead of AppendASCII() in the patch too?
Because this is an nsACString, it'll be slightly (probably optimized out though) to use Append, so go ahead and use that.

Comment 12

[Kouhei Sutou]

Attachment: use Apped() instead of AppendASCII()

(In reply to comment #11)
> (In reply to comment #10)
> > Should we use Append() instead of AppendASCII() in the patch too?
> Because this is an nsACString, it'll be slightly (probably optimized out
> though) to use Append, so go ahead and use that.

Thanks for your comment. I've updated a patch.

Comment 13

[Ed Lee :Mardak]

Comment on attachment 380340 [details] [diff] [review]
use Apped() instead of AppendASCII()

If there were no results from before, the previous query will return nothing. The original query was doing something like WHERE url IN (), and that's going to give nothing.

We should then make a query that doesn't end up with IN () but still gets us 0 results.

So we can just detect if prevMatchCount is 0 and query for something like.. SELECT 0 WHERE 0

Comment 14

[Kouhei Sutou]

It seems that I couldn't understand your comment. Can I confirm your comment?

(In reply to comment #13)
> (From update of attachment 380340 [details] [diff] [review])
> If there were no results from before, the previous query will return nothing.

Yes. It's this case, isn't it?
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#709

> The original query was doing something like WHERE url IN (), and that's going
> to give nothing.

We want to avoid "WHERE url IN ()" query because the query isn't valid SQL.
So we should avoid that the original query includes "WHERE url IN ()". And the attached patch does it.

> We should then make a query that doesn't end up with IN () but still gets us 0
> results.

In that case, we still have processing query? (!mAutoCompleteFinishedSearch)


I'll update my patch after I understand your comment.

Comment 15

[Ed Lee :Mardak]

(In reply to comment #14)
> > We should then make a query that doesn't end up with IN () but still gets us 0
> > results.
> In that case, we still have processing query? (!mAutoCompleteFinishedSearch)
Correct. Right now we detect if we should continue from where the last search stopped when mDBPreviousQuery is not-null. Perhaps we need a separate way to indicate that we want to continue searching but not use a previous query because it will result in no results anyway.

Comment 16

[Kouhei Sutou]

(In reply to comment #15)
> Correct. Right now we detect if we should continue from where the last search
> stopped when mDBPreviousQuery is not-null. Perhaps we need a separate way to
> indicate that we want to continue searching but not use a previous query
> because it will result in no results anyway.

OK. I understand your comment.
But can we detect "no results" case before a query finishes?

Comment 17

[Kouhei Sutou]

Attachment: More simple solution but it still uses mDBPreviousQuery.

If searching query doesn't get any results yet, we just drop the searching query and recreate a new query with new search term. It seems that we don't have any benefits to reuse the searching query if the searching query doesn't get any results yet.

What about this solution?

Or should I replace mDBPreviousQuery mechanism with something new mechanism?

Comment 18

[Ed Lee :Mardak]

Comment on attachment 382463 [details] [diff] [review]
More simple solution but it still uses mDBPreviousQuery.

This won't quite work as right now we depend on a non-null mDBPreviousQuery to decide where to continue searching (mPreviousChunkOffset)

Comment 19

[Kouhei Sutou]

(In reply to comment #18)
> This won't quite work as right now we depend on a non-null mDBPreviousQuery to
> decide where to continue searching (mPreviousChunkOffset)

I confirmed the patch solves this problem because else clause of its condition resets mDBPreviousQuery as null.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#768

Comment 20

[Shawn Wilsher :sdwilsh]

I'm told that newer SQLite's actually handle an empty IN clause.  Do we still need to fix this?

Comment 21

[Shawn Wilsher :sdwilsh]

In fact, I cannot reproduce this on trunk anymore (we've recently upgraded SQLite).

Comment 22

[Kouhei Sutou]

I cannot reproduce this on my environment, too.

Comment 23

[Shawn Wilsher :sdwilsh]

I guess this is WORKSFORME now.  For what it is worth, I've been told that if we ever hit a SQLite assertion, it is a bug in SQLite and we should report it upstream.