I don't know what this code is trying to do, but I know it's wrong. SetJournalMode() will return one of the JOURNAL_* constants, which are in the range 0-3. These will always pass the NS_SUCCEEDED check, so this patch shouldn't change behavior.

These two functions are supposed to return bool and PRInt32 respectively, not nsresult. The BindInt64ByName call might conceivably fail, based on code inspection -- in that case, this changes the caller to return false instead of true (since any non-NS_OK nsresult will evaluate to true). Likewise, GetNewSessionID() will now return -1 for the session id instead of something above 0x80000000, if CreateStatement() fails. Tell me if you'd prefer some other way of handling the errors. I could preserve existing behavior if you prefer.

Comment on attachment 651729 [details] [diff] [review] Remove unreached failure path from Database::InitSchema Review of attachment 651729 [details] [diff] [review]: ----------------------------------------------------------------- ugh! Good catch. Actually, we should keep the old error path, just fix the if condition to: if (JOURNAL_WAL != SetJournalMode(mMainConn, JOURNAL_WAL))

ehr, sorry I inverted the condition, should be if (JOURNAL_WAL == SetJournalMode(mMainConn, JOURNAL_WAL)) { // success } else { // fail }

Comment on attachment 651734 [details] [diff] [review] Use more appropriate return types than nsresult Review of attachment 651734 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsNavHistory.cpp @@ +713,5 @@ > nsresult rv = mDB->MainConn()->CreateStatement(NS_LITERAL_CSTRING( > "SELECT session FROM moz_historyvisits " > "ORDER BY visit_date DESC " > ), getter_AddRefs(selectSession)); > + NS_ENSURE_SUCCESS(rv, -1); I'd say to just return 0, that's what we do in other code parts. Mostly cause I didn't audit all the code to check if it properly handles negative session ids (at first glance it does properly, but you know...) Bonus points if you also fix this to use 0 instead of -1 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_async_history_api.js#553 should just be a mere replace.

Try for new series: https://tbpl.mozilla.org/?tree=Try&rev=487170880a34

Comment on attachment 657243 [details] [diff] [review] Don't treat SetJournalMode's return type as nsresult Review of attachment 657243 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Database.cpp @@ +559,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > // Be sure to set journal mode after page_size. WAL would prevent the change > // otherwise. > + if (JOURNAL_WAL != SetJournalMode(mMainConn, JOURNAL_WAL)) { sorry, you copied my typo, this one should be == (SetJournalMode returns the current mode, so we ask to set wal and if it returns wal it succeeded) r=me with this change.

Comment on attachment 657244 [details] [diff] [review] Use more appropriate return types than nsresult, v2 Review of attachment 657244 [details] [diff] [review]: ----------------------------------------------------------------- yep, thank you!

New try with fixed patch: https://tbpl.mozilla.org/?tree=Try&rev=e72260647793

https://hg.mozilla.org/integration/mozilla-inbound/rev/97e4f3d5adbf https://hg.mozilla.org/integration/mozilla-inbound/rev/b5282f388b5e https://hg.mozilla.org/integration/mozilla-inbound/rev/f101caca12a6

https://hg.mozilla.org/mozilla-central/rev/97e4f3d5adbf https://hg.mozilla.org/mozilla-central/rev/b5282f388b5e https://hg.mozilla.org/mozilla-central/rev/f101caca12a6