782616 - Fix still more abuse of nsresult (storage/, toolkit/components/places/)

Status: RESOLVED FIXED

(Core :: General, defect)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Don't return nsresult from main() (storage/)

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Remove unreached failure path from Database::InitSchema

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.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Use more appropriate return types than nsresult

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 4

[Marco Bonardo [:mak]]

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))

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Don't treat SetJournalMode's return type as nsresult

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Use more appropriate return types than nsresult, v2

Try for new series:

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

Comment 9

[Marco Bonardo [:mak]]

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 10

[Marco Bonardo [:mak]]

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!

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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