1252610 - crash in android.database.sqlite.SQLiteException: near ")": syntax error (code 1): , while compiling: INSERT INTO… when suggested sites list is empty

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-fcc19f54-f594-4cf1-b6fe-ebe0c2160301.
=============================================================

New crash today. Fallout from bug 760956.

android.database.sqlite.SQLiteException: near ")": syntax error (code 1): , while compiling: INSERT INTO topsites SELECT * FROM (SELECT _id, url, title, NULL AS history_id, 3 as type FROM (  ) WHERE url NOT IN (SELECT url FROM topsites) AND url NOT IN (SELECT url FROM bookmarks WHERE parent == ?) LIMIT MAX(0, (? - (SELECT COUNT(*) FROM topsites) - (SELECT COUNT(*) FROM bookmarks WHERE parent == ?)))  )
#################################################################
Error Code : 1 (SQLITE_ERROR)
Caused By : SQL(query) error or missing database.
	(near ")": syntax error (code 1): , while compiling: INSERT INTO topsites SELECT * FROM (SELECT _id, url, title, NULL AS history_id, 3 as type FROM (  ) WHERE url NOT IN (SELECT url FROM topsites) AND url NOT IN (SELECT url FROM bookmarks WHERE parent == ?) LIMIT MAX(0, (? - (SELECT COUNT(*) FROM topsites) - (SELECT COUNT(*) FROM bookmarks WHERE parent == ?)))  ))
#################################################################
	at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method)
	at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1093)
	at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:670)
	at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:588)
	at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:59)
	at android.database.sqlite.SQLiteStatement.<init>(SQLiteStatement.java:31)
	at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1812)
	at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1793)
	at org.mozilla.gecko.db.BrowserProvider.getTopSites(BrowserProvider.java:819)
	at org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:894)
	at android.content.ContentProvider.query(ContentProvider.java:1007)
	at android.content.ContentProvider$Transport.query(ContentProvider.java:218)
	at android.content.ContentResolver.query(ContentResolver.java:489)
	at android.content.ContentResolver.query(ContentResolver.java:433)
	at org.mozilla.gecko.db.LocalBrowserDB.getTopSites$5514332a(LocalBrowserDB.java:1667)
	at org.mozilla.gecko.home.TopSitesPanel$TopSitesLoader.loadCursor(TopSitesPanel.java:505)
	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:44)
	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:26)
	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground$532ebdd5(Unknown Source)
	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground$42af7916(Unknown Source)
	at android.support.v4.content.ModernAsyncTask$2.call(Unknown Source)
	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
	at java.lang.Thread.run(Thread.java:818)

Comment 1

[Andrzej Hunt :ahunt]

This seems to be because we're not being provided any suggested sites, resulting in an empty string from suggestedSitesBuilder.toString().

Comment 2

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1252610 - Don't insert suggested sites into topsites table unless they actually exist r=rnewman

Review commit: https://reviewboard.mozilla.org/r/37473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37473/

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8725409 [details]
MozReview Request: Bug 1252610 - Don't insert suggested sites into topsites table unless they actually exist r=rnewman

https://reviewboard.mozilla.org/r/37473/#review34005

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:821
(Diff revision 1)
> +            if (suggestedSitesBuilder.length() > 0) {

Rename `firstClause` to `hasProcessedAnySuggestedSites` and use it here.

File a follow-up to run more efficient SQL altogether if there are no suggested sites.

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8725409 [details]
MozReview Request: Bug 1252610 - Don't insert suggested sites into topsites table unless they actually exist r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37473/diff/1-2/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1252610 - Extract SuggestedSites interface to simplify testing r=liuche

Review commit: https://reviewboard.mozilla.org/r/37485/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37485/

Comment 7

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1252610 - Expand TestTopSites to cover the empty SuggestedSites case r=liuche

There is some duplication of the column names, however we're aiming to remove
all the MatrixCursor stuff in Bug 1249018 anyways.

Review commit: https://reviewboard.mozilla.org/r/37487/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37487/

Comment 8

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/9aefc0d2d3b4e787b94fb88469d04f100ef32754
Bug 1252610 - Don't insert suggested sites into topsites table unless they actually exist r=rnewman

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8725434 [details]
MozReview Request: Bug 1252610 - Extract SuggestedSites interface to simplify testing r=liuche

https://reviewboard.mozilla.org/r/37485/#review34057

This seems to be fine, since we're just moving code. I'd like to see a green try build though.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalSuggestedSites.java:6
(Diff revision 1)
> +package org.mozilla.gecko.db;

You'll need to add this file to moz.build

::: mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java:42
(Diff revision 1)
> +    Cursor get(int limit);

Do these need access modifiers? (public, etc) Generally we avoid package-level implicit access.

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8725435 [details]
MozReview Request: Bug 1252610 - Expand TestTopSites to cover the empty SuggestedSites case r=liuche

https://reviewboard.mozilla.org/r/37487/#review34067

Make sure this also runs on try.

::: mobile/android/base/java/org/mozilla/gecko/db/StubSuggestedSites.java:1
(Diff revision 1)
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-

Make sure to add this to the test moz.build.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit3/background_junit3_sources.mozbuild#24

::: mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestTopSites.java:56
(Diff revision 1)
> +        verifyGetTopSites(mContext, mEmptySuggestedSites);

It looks like the mEmptySuggestedSites that gets passed in gets overwritten by verifGetTopSites anyways.

I don't think you need the member variables anymore and can just create the suggestedSites instance that you want to test within each test method and pass that into verifyGetTopSites.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9aefc0d2d3b4