381549 - mozStorageStatement ignores return values from SQLite

Status: RESOLVED FIXED

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

Description

[Colin Barrett [:cbarrett]]

http://mxr.mozilla.org/mozilla/source/storage/src/mozStorageStatement.cpp#287
http://www.sqlite.org/capi3ref.html#sqlite3_bind_text

We also do completely unnecessary checks that the implementation does for us. Filed on behalf of dwitte and sdwilsh.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

I missed two success codes that are kinda important in the last patch.

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 267517 [details] [diff] [review]
v1.1

two minor nits:

1)

sean tells me the reason he didn't remove the "if (aParamIndex < 0 || aParamIndex >= mParamCount)" check before
sqlite3_bind_parameter_name() and sqlite3_column_name() is on bad values, they will return null, which we aren't treating as an error case.

sean, can you add a comment to that effect to those two places?


2)

+ * The Original Code is mozStorage Code.
+ *
+ * The Initial Developer of the Original Code is
+ *   Mozilla Corporation

To be fair, while you did extend it, I think Oracle should still get credit for ConvertResultCode(), and not Mozilla
and the Initial Developer should be Vladimir Vukicevic <[e-mail]>

This seems like the right thing to do when moving existing around to new files.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

I actually missed mozStorageStatement::Clone in the last patch, so I did the changes to that in this one, as well as addressed the review comments.

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 267593 [details] [diff] [review]
v1.2

r=sspitzer

nit:  s/Sicne/Since

also, please run through the mozStorage unit tests and the places unit tests before landing.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

With nit fixed.

Comment 7

[Shawn Wilsher :sdwilsh]

There's a slight issue with this patch still - in fact, it's the same as Bug 383673.  We only assert if we are out of bounds when everyone I've asked agrees that it should fail.

New patch coming to fix that...

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

Seth, I know we talked about keeping the assertions, but I think this error message is clear enough to not also have the assertion.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 267669 [details] [diff] [review]
v1.4

r=sspitzer

Comment 10

[Shawn Wilsher :sdwilsh]

Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.16; previous revision: 1.15
Checking in storage/src/mozStorageConnection.cpp;
new revision: 1.17; previous revision: 1.16
Checking in storage/src/mozStorage.h;
initial revision: 1.1