559678 - Better errors for Storage transactions misuse

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0

return NS_ERROR_UNEXPECTED (Shawn prefers this one so we don't have to rev uuid) if a transaction does not exist on COMMIT or ROLLBACK, fix idls (were never updated) and WARN the sqlite error code if it's unknown.

I guess Andrew's review is enough here, feel free to forward if i'm wrong please.
Thanks.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 439370 [details] [diff] [review]
patch v1.0

Should you be using NS_LITERAL_CSTRING for your error message?

Assuming Shawn has signed off on the error changes, my review probably works?  In general I think you want to direct all reviews to sdwilsh, though.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> Assuming Shawn has signed off on the error changes, my review probably works? 
> In general I think you want to direct all reviews to sdwilsh, though.
You are a peer ;)

Comment 3

[Marco Bonardo [:mak]]

(In reply to comment #1)
> (From update of attachment 439370 [details] [diff] [review])
> Should you be using NS_LITERAL_CSTRING for your error message?

good question, i see in code we use both forms, i guess Append has overloads, i'm unsure what's the "cheapest" one, not that i care much since this is debug only and pretty rare.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Gavin suggested AppendLiteral, that works too, since he's more experienced, i'll just go for it.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.2

maybe attach the right patch, eh? :)

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.3

yay! Looks like Storage had a test for this exception, fixed it to check NS_ERROR_UNEXPECTED.

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/7a19f732a070

Comment 8

[Marco Bonardo [:mak]]

i'll push a followup to use better (less scarying) wording in the warning.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> i'll push a followup to use better (less scarying) wording in the warning.
Thanks mak!

Comment 10

[Marco Bonardo [:mak]]

Better warning
http://hg.mozilla.org/mozilla-central/rev/131c1b4b9d2c