489897 - implement StatementRow::NewEnumerate

Status: RESOLVED DUPLICATE of bug 1390489

(Toolkit :: Storage, defect, P3)

Description

[David Dahl :ddahl]

The row enumeration in the statement. just like Params

Comment 1

[David Dahl :ddahl]

Attachment: v 0.1

This cannot possibly actually work, Really. Please tell me I fucked up:)

Comment 2

[David Dahl :ddahl]

No - that could not have worked - do I need an execute_soon type call for the executeAnsync call?

Comment 3

[Shawn Wilsher :sdwilsh]

This doesn't get created for the stuff returned from executeAsync, just executeStep.  You can then call stmt.row off of that.  See also https://developer.mozilla.org/en/mozIStorageStatementRow

Comment 4

[David Dahl :ddahl]

Horrors! I thought I was helpoing out with async stuff:)

Comment 5

[David Dahl :ddahl]

Attachment: v 0.2

Comment 6

[David Dahl :ddahl]

Comment on attachment 374534 [details] [diff] [review]
v 0.2

wrong patch, sorry

Comment 7

[David Dahl :ddahl]

Attachment: v 0.2

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 374535 [details] [diff] [review]
v 0.2

>+++ b/storage/test/unit/test_js_helpers_enumerate.js
>+function test_row_enumerate()
>+{
>+  var that = this;
nit: appears to be unused

>+  getOpenedDatabase().createTable('test_row_tbl','name TEXT, car TEXT');
nit: store getOpenedDatabase in a local variable called db or something please?

>+  let sql = "INSERT INTO test_row_tbl (name, car) VALUES ('david', 'Fiat 500')";
>+  let insertStmt = getOpenedDatabase().createStatement(sql);
>+  insertStmt.executeStep();
>+  insertStmt.finalize();
normally you'd want to use something like executeSimpleSQL off of the connection if you have a one-off like this.  However, I'd prefer it if you bound the parameters.

>+  let sql = "SELECT * FROM test_row_tbl";
>+  let selectStmt = getOpenedDatabase().createStatement(sql);
generally we just use the variable name stmt here, and you shouldn't need the sql variable (also, I think you've redeclared sql here).

I'd also just prefer if you modified the table creation run_test to make it have more than one column so we don't have to create another table.

>+  while(selectStmt.executeStep()){
You only have one row, so you should just do_check_true(stmt.executeStep())

>+    let name = selectStmt.row.name;
>+    do_check_eq(name,'david');
>+    let car  = selectStmt.row.car;
>+    do_check_eq(car,'Fiat 500');
This isn't actually testing enumeration.  You'll want to do a for (let name in stmt.row) and check for the column names similar to how test_params_enumerate does it for stmt.params

> let tests = [
>   test_params_enumerate,
>+  test_row_enumerate
nit: trailing comma please

Comment 9

[David Dahl :ddahl]

Attachment: WIP patch, fail!

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/ddahl/code/moz/mozilla-central/mozilla/storage/src/mozStorageStatementRow.cpp, line 233
[Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_js_helpers_enumerate.js :: test_row_enumerate :: line 78"  data: no]

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 376317 [details] [diff] [review]
WIP patch, fail!

>+++ b/storage/src/mozStorageStatementRow.cpp
>+      // Get column count.
>+      mStatement->GetColumnCount(&mColumnCount);
we should actually get and set this in the constructor

>+++ b/storage/src/mozStorageStatementRow.h
>@@ -56,16 +56,16 @@ public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_MOZISTORAGESTATEMENTROW
>   NS_DECL_NSIXPCSCRIPTABLE
> 
>   StatementRow(mozStorageStatement *aStatement);
> protected:
> 
>   mozStorageStatement *mStatement;
>-
>+  PRUint32 mColumnCount;
nit: newline before friend declatarion

>+++ b/storage/test/unit/test_js_helpers_enumerate.js
>+  let stmt = db.createStatement("INSERT INTO test (driver, car) VALUES (:driver, :car)");
you can actually just call createStatement (global function)

>+  stmt.params['driver'] = 'David';
>+  stmt.params['car'] = 'Fiat 500';
stmt.params.driver and stmt.paramd.car please

>+  stmt.executeStep();
just execute

>+  stmt.finalize();
and this should be in a finally block and execute should be in a try block

>+  let stmt = db.createStatement("SELECT driver FROM test WHERE driver = :driver");
redeclared stmt

>+  stmt.params['driver'] = 'David';
same as above

>+  stmt.executeStep();
do_check_true this

>+  let expected = ["driver",];
I'd like to have more than one column here, just to test more than one thing for an enumeration

>+  let index = 0;
>+  for (let colName in stmt.row){
>+    do_check_eq(colName,expected[index]);
>+    index++;
you can just do do_check_eq(colName, execpted[index++]) and lose the bracing.  To debug this, I'd add a dump statement indicating what colName is that you are getting.

>+  stmt.finalize();
needs to be in a finally block, and everything starting with executeStep needs to be in a try block

>   getOpenedDatabase().executeSimpleSQL(
>     "CREATE TABLE test (" +
>-      "id INTEGER PRIMARY KEY " +
>+      "id INTEGER PRIMARY KEY, driver VARCHAR(32) NULL, car VARCHAR(32) NULL" +
each column on a new line please

Comment 11

[David Dahl :ddahl]

Attachment: Another hot cup of fail

Everyone's favorite error:

[Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_js_helpers_enumerate.js :: test_row_enumerate :: line 79"  data: no]

Seems like a pretty fundamental JS error. Out of my league:)

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: updated hot cup of fail

updated from some bitrot

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Changes to David's patch here are pretty minimal, but I'm going to have him take a look anyway.  sr for the API change.

Comment 14

[David Dahl :ddahl]

Comment on attachment 404154 [details] [diff] [review]
v1.0

Looks good to me. I like the new clean style of storage. I had a bunch of failed tests reported, but i am sure it is a borked obj dir, rebuilding now just to make sure

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 404154 [details] [diff] [review]
v1.0

>     nsDependentCString jsid(::JS_GetStringBytes(JSVAL_TO_STRING(aId)));
> 
>+    // If we are trying to get the __iterator__ property, return early.  Our
>+    // NewEnumerate function will handle that.
>+    if (0 == strcmp(jsid.get(), "__iterator__"))

Given that jsid is already an nsDependentCString, can you just call EqualsLiteral here?

>@@ -147,6 +155,70 @@ StatementRow::GetProperty(nsIXPConnectWr
>+      JSString *jsname = ::JS_NewStringCopyN(aCtx, &(name.get()[1]),
>+                                             name.Length());
>+      NS_ENSURE_TRUE(jsname, NS_ERROR_OUT_OF_MEMORY);

I don't understand using &(name.get()[1]) but keeping the length. Does that not include a trailing null character or something? Either way, this needs a comment.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> I don't understand using &(name.get()[1]) but keeping the length. Does that not
> include a trailing null character or something? Either way, this needs a
> comment.
This looks like a copy/paste error.  Should just be using name.get() here.

Comment 17

[David Dahl :ddahl]

Attachment: v 1.1 Review comments addressed

Comment 18

[David Dahl :ddahl]

I was hoping to get this into 3.6 - probably too late, eh? sadface.

Comment 19

[Shawn Wilsher :sdwilsh]

Does this just need to be checked in then?

Comment 20

[David Dahl :ddahl]

I though we were waiting on blake for sr+. Looks like i forgot to flag the patch.

Comment 21

[David Dahl :ddahl]

Oh, the patch says it is already sr+

Comment 22

[David Dahl :ddahl]

sdwilsh:

do you think this can land? this will be quite helpful for the placesQueryApi refactor

Comment 23

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/64d3466e3964

Comment 24

[Reed Loden [:reed]]

I backed this out in http://hg.mozilla.org/mozilla-central/rev/6f2093ee4cb9 due to test failures. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262237726.1262239889.17949.gz

Comment 25

[Justin Dolske [:Dolske]]

Fascinating.

The failing test code is inside pwmgr_common.js#131, where we remove all logins, add 1 test login, and then sanity check that it actually got added. Surprisingly, the sanity check fails.

With signon.debug on in a local test run, I see getAllLogins() failing with "PwMgr cryptoSDR: Failed to decrypt string:  (NS_ERROR_FAILURE)". We can't decrypt the login that just got added, so we return nothing. A dump of signons.sqlite at this point looks like it should be OK, so I dunno why this patch triggers a failure.

Next step would be to look at if the encrypted value is getting mangled somewhere, and why NSS thinks it suddenly can't decrypt things.

Comment 26

[Justin Dolske [:Dolske]]

So, the data in the DB is the same as the data saved by addLogin.

The problem is that when we query the DB, we get back one row (as expected), but all the values (eg stmt.row.hostname) are |undefined|. And that's why the decryption fails.

Looks like there are other xpcshell test failures associated with this landing too... http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262239166.1262242381.12184.gz

Comment 27

[David Dahl :ddahl]

Looks like some storage tests also failed. Weird - as I ran the entire storage test suite while testing this patch.

Comment 28

[David Dahl :ddahl]

really strange. did another storage patch make this one toxic?  

8 tests fail now:

http://pastebin.mozilla.org/694177

Comment 29

[David Dahl :ddahl]

I attempted to run the xpcshell tests that cover this patch via gdb and set a breakpoint in Statement::GetColumnIndex, but failed. check-interactive was not helpful. The breakpoint was not set, but would be set if gdb detected the libaray loading.

anyway... we seem to be failing due to a type issue:

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_js_helpers_enumerate.js :: test_row_enumerate :: line 81"  data: no]

which points to this method:

NS_IMETHODIMP
Statement::GetColumnIndex(const nsACString &aName,
                          PRUint32 *_index)
{
  if (!mDBStatement)
      return NS_ERROR_NOT_INITIALIZED;

  // Surprisingly enough, SQLite doesn't provide an API for this.  We have to
  // determine it ourselves sadly.
  for (PRUint32 i = 0; i < mResultColumnCount; i++) {
    if (mColumnNames[i].Equals(aName)) {
      *_index = i;
      return NS_OK;
    }
  }

  return NS_ERROR_INVALID_ARG;
}


I guess the NS_ERROR_INVALID_ARG is the same as  the ILLEGAL_VALUE above?

Comment 30

[Jan de Mooij [:jandem]]

Bug 1390489 fixes this.