> 118 inline PRInt64 AsInt64(PRUint32 idx) { > 119 PRInt64 v; > 120 GetInt64(idx, &v); > 121 return v; > 122 } GetInt64 can fail in 2 cases: - idx is invalid - the value in the database is NULL in such cases: - we ignore a valid error - nothing is assigned to v and we could return a random value possible solutions: - deprecate AsXXX methods and replace them with proper GetXXX methods with error checking, doing null check where needed - make AsXXX return a default value (like 0 for int) in case they hit a NULL, abort in case idx is invalid. This would make them proper shortcuts with a slightly different behavior from GetXXX methods. As they are we only save a NS_ENSURE_SUCCESS, making this change would allow to avoid GetIsNull - always abort if GetXXX method fails

actually adw noted that GetInt64 uses sqlite3_value_int64, this method is already doing the conversion NULL to 0 based on the table on this page http://www.sqlite.org/c3ref/column_blob.html So the really remaining issue here is that if someone uses AsIntXX with an invalid column index he will never know and use an unitialized value... I'd say at this point this is a depelopment problem. The right solution could be nsresult rv = GetInt64(idx, &v); NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "Trying to get value for a not existing column");

put up quickly, I think the error is bad enough to cause an abort, not bad enough to not be devs-only

backed out http://hg.mozilla.org/mozilla-central/rev/d688a915d1d4 test_async.js in places is asserting in processfoldernoderow http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavBookmarks.cpp#2319 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1277167723.1277169760.29304.gz

sigh, as I was reporting early this fails if the entry is NULL and we want int, looks like it is not using sqlite3_value_int64 but Variant_base::GetDataType(PRUint16 *_type) that returns an awesome NS_ERROR_CANNOT_CONVERT_DATA; and this is again a scary bug.

> xul.dll!mozilla::storage::Variant_base::GetAsInt64(__int64 * __formal=0x0038d964) Line 84 C++ xul.dll!mozilla::storage::Row::GetInt64(unsigned int aIndex=5, __int64 * _value=0x0038d964) Line 194 C++ xul.dll!mozIStorageValueArray::AsInt64(unsigned int idx=5) Line 125 + 0x16 bytes C++

the column in question is a calculated value (an IFNULL) and its value is NULL. so column_type ends up being null and we put a null variant in the mData array, then when asking for the result we try to convert a null variant to an int64

i'm back to the initial question. Should we trust the caller and return a default value if the conversion fails, or abort and force the caller to check GetIsNull?

sqlite api methods trust the caller (if null and user asking int it is returning 0), so looks like better to follow that. Should we do the same for GetIntXX methods? they have a GetIsNull and looks like AsIntXX are shortcuts that don't care about null.

Doesn't NS_ABORT_IF_FALSE abort even in opt builds?

(In reply to comment #10) > Doesn't NS_ABORT_IF_FALSE abort even in opt builds? nope http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#173

Comment on attachment 453039 [details] [diff] [review] trust the caller I think I prefer version 1's approach of always notifying regardless.

that will complicate our code a bit in all those cases where we use calculated columns that could be null. those column don't have a type definition because are calculated, but I can do that.

(In reply to comment #13) > that will complicate our code a bit in all those cases where we use calculated > columns that could be null. those column don't have a type definition because > are calculated, but I can do that. How so? Wouldn't we want to know if the Get failed? That's the only difference between the patches (at least, that's what interdiff tells me).

well no, the second patch will abort only if index is invalid, but will allow to select as int a null column (like a calculated column). the first patch instead will abort both if index is wrong and if you try to get int from a null column. so if you have a column like IFNULL(a, b) where both A and B are int, but they can be null, when you use asIntXX on it the second patch will return 0, the first patch will abort. To handle it you will have to GetIsNull(&isNull); if (isNull) handleNull; else AsIntXX;

instead if a column exists as type null, is null and you try to fetch it through asIntXX it will return 0... This issue is practically only on calculated columns, we would probably have a different behavior not returning 0. I should probably write a test case to make this clear.

I meant "if a column exists as type int, is null..."

(In reply to comment #15) > well no, the second patch will abort only if index is invalid, but will allow > to select as int a null column (like a calculated column). the first patch > instead will abort both if index is wrong and if you try to get int from a null > column. > > so if you have a column like IFNULL(a, b) where both A and B are int, but they > can be null, when you use asIntXX on it the second patch will return 0, the > first patch will abort. To handle it you will have to GetIsNull(&isNull); if > (isNull) handleNull; else AsIntXX; Oh right. Let's special case null, but abort on all other errors, OK?

This ignores errors if GetXXX succeeded or if (it fails and) the field is null, any other error aborts. I added a cpp test that touches 3 paths, where we fail is with async statements. While sync statements pass though sqlite_get_value functions that do conversion for us, async statements instead use a cached mozIStorageRow, that goes through Variant conversions. I guess this happens because those rows are cached while in sync stmts we directly return values from current active row in sqlite result.

Comment on attachment 455675 [details] [diff] [review] patch v2.0 r=sdwilsh

Comment on attachment 455675 [details] [diff] [review] patch v2.0 asking approval to land, risk is low, but can prevent some unexpected behavior.

small unbitrot for test in makefile.in

forgot tt close http://hg.mozilla.org/mozilla-central/rev/ce14f575aff9