1067085 - Awesomebar search results don't disappear when a search query that initially showed results doesn't yield results anymore

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

STR:

1) Type "bugzilla" in the Awesomebar assuming that you went to Bugzilla before.
2) Wait until some Awesomebar results are shown.
3) Now type " foo bar" so that "bugzilla foo bar" is the search term.

The old results should now disappear but instead they will stay. By disabling unified autocomplete the old behavior will be restored.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Can't reproduce this in my trunk profile, hm.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

In my main profile, whenever I hit this, I see three occurrences of:

"Error(s) encountered during statement execution. Sqlite.jsm:618"

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

This seems to be caused by the MATCH_ANYWHERE search in case we don't get enough search results. Investigating further.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Hum. That fails for me because of the following SQLite error:

<database disk image is malformed>

Marco, any idea what to do about that? Should we handle those failures? If I wrap the conn.executeCached() calls in UnifiedComplete.js we at least resolve the promise returned by execute() instead of rejecting it and thus clear search results.

Comment 5

[Marco Bonardo [:mak]]

Is your database really malformed, or is that error due to some unexpected error in the match function?
Could you please check the db with "PRAGMA integrity_check" ?

I guess we might indeed handle a failure differently here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#1349

what we do currently is just reporting an error, we should probably invoke this.finishSearch(true) in both cases (success or failure), so we return whatever we got so far (and keep reporting error in the failure case).
Would this help in your case?

Something like:

this.getDatabaseHandle().then(conn => search.execute(conn))
                        .then(undefined, ex => { 
                          dump("Query failed: " + ex + "\n");
                          Cu.reportError(ex);
                        })
                        .then(() => {
                          if (search == this._currentSearch) {
                            this.finishSearch(true);
                        });

Though we should first figure the corruption type.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> Is your database really malformed, or is that error due to some unexpected
> error in the match function?
> Could you please check the db with "PRAGMA integrity_check" ?

wrong # of entries in index moz_places_guid_uniqueindex
wrong # of entries in index moz_places_url_uniqueindex
wrong # of entries in index moz_places_lastvisitdateindex
wrong # of entries in index moz_places_frecencyindex
wrong # of entries in index moz_places_visitcount
wrong # of entries in index moz_places_hostindex
wrong # of entries in index moz_places_faviconindex
rowid 242908 missing from index moz_historyvisits_dateindex
rowid 242908 missing from index moz_historyvisits_fromindex
rowid 242908 missing from index moz_historyvisits_placedateindex
wrong # of entries in index moz_historyvisits_dateindex
wrong # of entries in index moz_historyvisits_fromindex
wrong # of entries in index moz_historyvisits_placedateindex

:(

Comment 7

[Marco Bonardo [:mak]]

just issuing a REINDEX should fix it

now the funny part is that Places Maintenance we run weekly should have fixed this, but it didn't, that means likely your profile is not running idle-daily tasks?

Comment 8

[Marco Bonardo [:mak]]

that could be related to bug 973591...

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #8)
> that could be related to bug 973591...

Hah, just wrote a comment mentioning the exact same bug. Yeah, IRCCloud is always open. Should probably find out what's causing this.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> this.getDatabaseHandle().then(conn => search.execute(conn))
>                         .then(undefined, ex => { 
>                           dump("Query failed: " + ex + "\n");
>                           Cu.reportError(ex);
>                         })
>                         .then(() => {
>                           if (search == this._currentSearch) {
>                             this.finishSearch(true);
>                         });

Tried and WFM.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #7)
> just issuing a REINDEX should fix it

It did, thanks :)

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1067085-Handle-Awesomebar-search-failures-better.patch

Well, that's pretty much what you suggested :)

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8489872 [details] [diff] [review]
0001-Bug-1067085-Handle-Awesomebar-search-failures-better.patch

Review of attachment 8489872 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm!

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/8b581c66bded

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b581c66bded

Comment 16

[Andrei Vaida [:avaida]]

Unfortunately, I was unable to replicate this issue on my end, several attempts were made on different test machines with the affected builds. 

Tim, is there any chance you could confirm this fix on your end?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

I can definitely say that I never saw this again since it was fixed. Is that enough to confirm? :)

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Tim Taubert [:ttaubert] from comment #17)
> I can definitely say that I never saw this again since it was fixed. Is that
> enough to confirm? :)

I believe so, yes. Thank you, Tim! I'll keep an eye on this just in case.