Fix the sqlite errors of content unittests on Android

Fix the sqlite errors of content unittests on Android

Many tests are failed due to sqlite errors on Android.
The reason is that sqlite need a temporary directory to
store internal temporary files. However, Android doesn't
have this kind of directory while Linux has /var/tmp.

To fix this, use in-memory temporary files. It also includes:
1) Add one unit test in sql unittests to reproduce this
issue for Android.
2) Remove those disabled cases for content unitests on
Android.

BUG=
TEST=run_tests.py -s sql_unittests; run_test.py -s net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148064

[yongsheng, 2012-07-19 14:23:02]

This patch is to fix many failed cases caused by sqlite.
The root cause is that sqlite uses temporary directory to store internal
temporary files. It checks below paths in order:
1) the global variable fixed in the patch;
2) env variable
3) /var/tmp
4) /tmp/
5) .
The errors are because sqlite use '.' as the temporary directory. On Android,
sqlite can't create files under this directory.

[yongsheng, 2012-07-19 14:24:34]

the old sql unittests don't cover this scenario, so i create a new unit test for
android. Please help review.

[John Grabowski, 2012-07-19 19:10:41]

Hey Scott, can you review this plz?

[Scott Hess - ex-Googler, 2012-07-19 19:47:21]

How does Android SQLite handle this, then?  Is this the same solution?

Would arranging for sqlite.gyp to add -DSQLITE_TEMP_STORE=3 work as an
alternative?  Or Execute("PRAGMA temp_store = memory") when opening the
connection?  [I think you want SQLITE_TEMP_STORE, though.]

In general, when SQLite creates temporary files, Bad Things Are Happening,
probably because someone is doing a query which would be far less expensive if
they moved some of the SQL into C++.  So even if you do end up having to go
forward with a change like this currently exists, I think you should log a bug
so that we can get those uses fixed so that eventually this is not needed.

http://codereview.chromium.org/10809015/diff/1/sql/connection.cc
File sql/connection.cc (right):

http://codereview.chromium.org/10809015/diff/1/sql/connection.cc#newcode23
sql/connection.cc:23: extern char* sqlite3_temp_directory;
This is in sqlite3.h, why do you need it here?  Also, the way you're using it is
contrary to the comment before the variable in sqlite3.h.

http://codereview.chromium.org/10809015/diff/1/sql/connection.cc#newcode48
sql/connection.cc:48: LAZY_INSTANCE_INITIALIZER;
All the methods and variables are static.  Why do you need this lazy instance at
all?  Just make everything global (w/in the anonymous namespace, of course).

[Scott Hess - ex-Googler, 2012-07-19 20:06:59]

http://crbug.com/138128 to track the shouldn't-create-temp-files problem.

Also, in case it wasn't clear, I was suggesting memory-only only for Android.

[yongsheng, 2012-07-20 01:25:24]

ok, I'll investigate below 3 issues:
1) why generating temp files internally?
2) Android sqlite usage
3) in-memory temp files
thanks for review.

http://codereview.chromium.org/10809015/diff/1/sql/connection.cc
File sql/connection.cc (right):

http://codereview.chromium.org/10809015/diff/1/sql/connection.cc#newcode23
sql/connection.cc:23: extern char* sqlite3_temp_directory;
On 2012/07/19 19:47:21, shess wrote:
> This is in sqlite3.h, why do you need it here?  Also, the way you're using it
is
> contrary to the comment before the variable in sqlite3.h.
should be removed.

http://codereview.chromium.org/10809015/diff/1/sql/connection.cc#newcode48
sql/connection.cc:48: LAZY_INSTANCE_INITIALIZER;
On 2012/07/19 19:47:21, shess wrote:
> All the methods and variables are static.  Why do you need this lazy instance
at
> all?  Just make everything global (w/in the anonymous namespace, of course).
ok.
let's wait for the investigation result about whether using in-memory temp
files.

[yongsheng, 2012-07-20 03:08:35]

On 2012/07/20 01:25:24, yongsheng wrote:
> ok, I'll investigate below 3 issues:
> 1) why generating temp files internally?
See the below call stack.
> 2) Android sqlite usage
Here is the sqlite defines in Android
common_sqlite_flags := -DHAVE_USLEEP=1
-DSQLITE_DEFAULT_JOURNAL_SIZE_LIMIT=1048576 -DSQLITE_THREADSAFE=1 -          
DNDEBUG=1 -DSQLITE_ENABLE_MEMORY_MANAGEMENT=1 -DSQLITE_DEFAULT_AUTOVACUUM=1
-DSQLITE_TEMP_STORE=3 -                  DSQLITE_ENABLE_FTS3
-DSQLITE_ENABLE_FTS3_BACKWARDS -DSQLITE_DEFAULT_FILE_FORMAT=4
> 3) in-memory temp files
yes, it works.
> thanks for review.

Callstack to generate internal temp files(you can see zPath is 0):
#0  unixOpen (pVfs=0xad642b84, zPath=0x0, pFile=0x8b02918, flags=8222,
pOutFlags=0x0) at third_party/sqlite/amalgamation/sqlite3.c:29099
#1  0xacfb41fa in sqlite3OsOpen (pPg=0x8b4afc8) at
third_party/sqlite/amalgamation/sqlite3.c:14087
#2  pagerOpentemp (pPg=0x8b4afc8) at
third_party/sqlite/amalgamation/sqlite3.c:39841
#3  openSubJournal (pPg=0x8b4afc8) at
third_party/sqlite/amalgamation/sqlite3.c:40516
#4  subjournalPage (pPg=0x8b4afc8) at
third_party/sqlite/amalgamation/sqlite3.c:40548
#5  0xacfc284b in pager_write (pPg=0x8b4afc8) at
third_party/sqlite/amalgamation/sqlite3.c:41890
#6  0xacfd0bfa in sqlite3PagerWrite (pDbPage=<value optimized out>) at
third_party/sqlite/amalgamation/sqlite3.c:42002
#7  0xacfd49c4 in insertCell (pPage=0x8b4b3ec, i=<value optimized out>,
    pCell=0x8b42e30 "\v\003\033\001version\001lemetameta\002CREATE TABLE
meta(key LONGVARCHAR NOT NULL UNIQUE PRIMARY KEY, value LONGVARCHAR)", sz=12,
pTemp=0x0,
    iChild=0, pRC=0xbfb722bc) at third_party/sqlite/amalgamation/sqlite3.c:52724
#8  0xacfd7994 in sqlite3BtreeInsert (pCur=0x8b15a88, pKey=<value optimized
out>, nKey=11, pData=0xad07fad1, nData=0, nZero=0, appendBias=0, seekResult=0)
    at third_party/sqlite/amalgamation/sqlite3.c:53976
#9  0xacfff7fb in sqlite3VdbeExec (p=<value optimized out>) at
third_party/sqlite/amalgamation/sqlite3.c:66852
#10 sqlite3Step (p=<value optimized out>) at
third_party/sqlite/amalgamation/sqlite3.c:60868
#11 0xad002df0 in sqlite3_step (pStmt=0x8b0aec0) at
third_party/sqlite/amalgamation/sqlite3.c:60933
#12 0xacf4f884 in sql::Statement::Run (this=0xbfb72748) at sql/statement.cc:54
#13 0xacf4ed61 in sql::MetaTable::SetValue (this=0xbfb72abc, key=0xad090138
"version", value=4) at sql/meta_table.cc:94
#14 0xacf4eed5 in sql::MetaTable::SetVersionNumber (this=0xbfb72abc, version=4)
at sql/meta_table.cc:65
#15 0xacf4f14a in sql::MetaTable::Init (this=0xbfb72abc, db=0x8af4928,
version=4, compatible_version=4) at sql/meta_table.cc:53

[yongsheng, 2012-07-20 03:21:35]

refine it according to your comments and sqlite usage in Android OS.
Thanks.

[Scott Hess - ex-Googler, 2012-07-20 12:37:46]

lgtm.

I've poked at it a little bit, and at this point I suspect that it might
actually be an unexpected interaction w/in SQLite.  The construct which causes
it to spill to a temporary file is really trivial, and AFAICT the temporary file
actually shouldn't be necessary in the first place.  Notably, since it's a
temporary file, it can't be involved in any other database reader's consistency.

Regardless, I suspect that your fix might be an appropriate general fix for
Android and probably even appropriate for ChromiumOS, even though ChromiumOS may
not require it as strongly.

[yongsheng, 2012-07-20 14:42:10]

On 2012/07/20 12:37:46, shess wrote:
> lgtm.
thanks for review.  Others, will you help review it?
> I've poked at it a little bit, and at this point I suspect that it might
> actually be an unexpected interaction w/in SQLite.  The construct which causes
> it to spill to a temporary file is really trivial, and AFAICT the temporary
file
> actually shouldn't be necessary in the first place.  Notably, since it's a
> temporary file, it can't be involved in any other database reader's
consistency.
got it, thanks for explanation. actually there are many other places to generate
temporary files. For example, some of the failed cases of appcache are due to
create unique index. 

> Regardless, I suspect that your fix might be an appropriate general fix for
> Android and probably even appropriate for ChromiumOS, even though ChromiumOS
may
> not require it as strongly.
ok, let's use the macro here.
If you think it's appropriate, i'm glad to adopt this fix in the future.

[yongsheng, 2012-07-21 00:18:57]

yaron & jrg, will you review it?

[John Grabowski, 2012-07-23 18:58:38]

Android LGTM

[commit-bot: I haz the power, 2012-07-24 01:10:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10809015/9002

[commit-bot: I haz the power, 2012-07-24 01:10:43]

Failed to apply patch for build/android/gtest_filter/content_unittests_disabled:
While running patch -p1 --forward --force;
patching file build/android/gtest_filter/content_unittests_disabled
Hunk #1 FAILED at 42.
1 out of 1 hunk FAILED -- saving rejects to file
build/android/gtest_filter/content_unittests_disabled.rej

[commit-bot: I haz the power, 2012-07-24 05:15:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yongsheng.zhu@intel.com/10809015/14001

[commit-bot: I haz the power, 2012-07-24 08:25:54]

Change committed as 148064