implement StatementRow::NewEnumerate

RESOLVED DUPLICATE of bug 1390489

Status

()

Toolkit
Storage
P3
normal
RESOLVED DUPLICATE of bug 1390489
9 years ago
2 months ago

People

(Reporter: ddahl, Unassigned)

Tracking

unspecified
mozilla1.9.3a1
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

9 years ago
The row enumeration in the statement. just like Params

Updated

9 years ago
Summary: implement mozStorageStatementRow::NewEnumerate → implement StatementRow::NewEnumerate
(Reporter)

Updated

9 years ago
Assignee: nobody → ddahl
(Reporter)

Comment 1

9 years ago
Created attachment 374517 [details] [diff] [review]
v 0.1

This cannot possibly actually work, Really. Please tell me I fucked up:)
Attachment #374517 - Flags: review?
(Reporter)

Updated

9 years ago
Attachment #374517 - Flags: review? → review?(sdwilsh)
(Reporter)

Comment 2

9 years ago
No - that could not have worked - do I need an execute_soon type call for the executeAnsync call?
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
(Reporter)

Comment 4

9 years ago
Horrors! I thought I was helpoing out with async stuff:)
(Reporter)

Comment 5

9 years ago
Created attachment 374534 [details] [diff] [review]
v 0.2
Attachment #374534 - Flags: review?(sdwilsh)
(Reporter)

Updated

9 years ago
Attachment #374517 - Attachment is obsolete: true
Attachment #374517 - Flags: review?(sdwilsh)
(Reporter)

Comment 6

9 years ago
Comment on attachment 374534 [details] [diff] [review]
v 0.2

wrong patch, sorry
Attachment #374534 - Attachment is obsolete: true
Attachment #374534 - Flags: review?(sdwilsh)
(Reporter)

Comment 7

9 years ago
Created attachment 374535 [details] [diff] [review]
v 0.2
Attachment #374535 - Flags: review?(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
Attachment #374535 - Flags: review?(sdwilsh) → review-
(Reporter)

Comment 9

9 years ago
Created attachment 376317 [details] [diff] [review]
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]
Attachment #374535 - Attachment is obsolete: true
Attachment #376317 - Flags: review?(sdwilsh)

Updated

9 years ago
Attachment #376317 - Flags: review?(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
(Reporter)

Comment 11

9 years ago
Created attachment 376373 [details] [diff] [review]
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:)
Attachment #376317 - Attachment is obsolete: true
Attachment #376373 - Flags: review?(sdwilsh)
Created attachment 404132 [details] [diff] [review]
updated hot cup of fail

updated from some bitrot
Attachment #376373 - Attachment is obsolete: true
Attachment #376373 - Flags: review?(sdwilsh)
Created attachment 404154 [details] [diff] [review]
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.
Attachment #404132 - Attachment is obsolete: true
Attachment #404154 - Flags: superreview?(mrbkap)
Attachment #404154 - Flags: review?(ddahl)

Updated

8 years ago
Status: NEW → ASSIGNED
Whiteboard: [needs review ddahl][needs sr mrbkap]
Target Milestone: --- → mozilla1.9.3a1
(Reporter)

Comment 14

8 years ago
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
Attachment #404154 - Flags: review?(ddahl) → review+
(Reporter)

Updated

8 years ago
Whiteboard: [needs review ddahl][needs sr mrbkap] → [needs sr mrbkap]
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.
Attachment #404154 - Flags: superreview?(mrbkap) → superreview+
(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.
(Reporter)

Comment 17

8 years ago
Created attachment 405963 [details] [diff] [review]
v 1.1 Review comments addressed
Attachment #404154 - Attachment is obsolete: true
(Reporter)

Comment 18

8 years ago
I was hoping to get this into 3.6 - probably too late, eh? sadface.
Does this just need to be checked in then?
(Reporter)

Comment 20

8 years ago
I though we were waiting on blake for sr+. Looks like i forgot to flag the patch.
(Reporter)

Updated

8 years ago
Attachment #405963 - Flags: superreview?(mrbkap)
(Reporter)

Updated

8 years ago
Attachment #405963 - Flags: superreview?(mrbkap)
(Reporter)

Comment 21

8 years ago
Oh, the patch says it is already sr+
(Reporter)

Comment 22

8 years ago
sdwilsh:

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

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs sr mrbkap]
http://hg.mozilla.org/mozilla-central/rev/64d3466e3964
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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
(Reporter)

Comment 27

8 years ago
Looks like some storage tests also failed. Weird - as I ran the entire storage test suite while testing this patch.
(Reporter)

Comment 28

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

8 tests fail now:

http://pastebin.mozilla.org/694177
(Reporter)

Comment 29

8 years ago
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?
(Reporter)

Updated

4 years ago
Assignee: ddahl → nobody
Priority: -- → P3
Bug 1390489 fixes this.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago2 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1390489
You need to log in before you can comment on or make changes to this bug.