Closed Bug 381549 Opened 13 years ago Closed 12 years ago
Storage Statement ignores return values from SQLite
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.
Assignee: vladimir → nobody
Summary: mozStorage ignores return values from SQLite → mozStorageStatement ignores return values from SQLite
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #267510 - Flags: review?(sspitzer)
I missed two success codes that are kinda important in the last patch.
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 <email@example.com> This seems like the right thing to do when moving existing around to new files.
Attachment #267517 - Flags: review?(sspitzer) → review+
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 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.
Attachment #267593 - Flags: review?(sspitzer) → review+
With nit fixed.
Attachment #267593 - Attachment is obsolete: true
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...
12 years ago
Depends on: 383678
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 on attachment 267669 [details] [diff] [review] v1.4 r=sspitzer
Attachment #267669 - Flags: review?(sspitzer) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.