Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError in success callback #409

Closed
robwithhair opened this issue Jan 14, 2016 · 7 comments
Closed

TypeError in success callback #409

robwithhair opened this issue Jan 14, 2016 · 7 comments

Comments

@robwithhair
Copy link

I am getting the following error on iOS while using the plugin via pouchdb.

Error in Success callbackId: SQLitePlugin664112984 : TypeError: undefined is not an object (evaluating 'r.type')

The error is being triggered by the following line in sqlite plugin

    mycb = function(result) {
      var j, last, q, r, ref, res, type;
      last = result.length - 1;
      for (i = j = 0, ref = last; 0 <= ref ? j <= ref : j >= ref; i = 0 <= ref ? ++j : --j) {
        r = result[i];
        type = r.type; // <----  This line is triggering the exception because result is empty array (line 413)
        res = r.result;
        q = mycbmap[i];
        if (q) {
          if (q[type]) {
            q[type](res);
          }
        }
      }
    };

The error seems to be caused by cordova returning an empty array for result. When I bypass this code so the code continues to run it doesn't seem that there is a callback in the mycbmap, which also appears to be empty.

I'm submitting this issue now to see if anyone has experienced anything like this before or can offer any pointers. Right now I'm going to step through the iOS code line by line to attempt to find the database query which is throwing the exception. I'll add more details once I'm able to find the line but any help or advice on this issue would be much appreciated.

@robwithhair
Copy link
Author

When this issue occurs I can confirm result = [] and mycbmap = {}

@brodycj
Copy link
Contributor

brodycj commented Jan 15, 2016

@sstraus (Stefano Straus) reported the same issue in #410 was introduced in 0.7.12. Looking at the changes to CoffeeScript/Javascript, I think this is caused by using readTransaction with no SQL statements. Possible workarounds:

  • do a normal db.transaction instead
  • add a SELECT 1 sql statement to your readTransaction function

The following change to the test suite seems to reproduce the problem:

diff --git a/spec/www/spec/legacy.js b/spec/www/spec/legacy.js
index c044b50..3b18ba4 100755
--- a/spec/www/spec/legacy.js
+++ b/spec/www/spec/legacy.js
@@ -951,31 +951,36 @@ var mytests = function() {
               ok(false, 'should have rolled back');
             });
           });

         });

         // NOTE [BUG #230]: this is now working if we do not depend on a valid sqlite_master table
         // XXX TODO: test with and without transaction callbacks, also with empty db.readTransaction()
         test_it(suiteName + 'empty transaction (no sql statements) and then SELECT transaction', function () {

           stop(2);

           var db = openDatabase("Database-Undefined", "1.0", "Demo", DEFAULT_SIZE);

           try {
-            db.transaction();
+            // OK:
+            //db.transaction();
+            // OK:
+            //db.transaction(function(tx) {});
+            // BREAKING:
+            db.readTransaction(function(tx) {});
             ok(false, 'expected a synchronous error');
           } catch (err) {
             ok(!!err, 'got error like we expected');
           }

           // verify we can still continue
           db.transaction(function (tx) {
             tx.executeSql('SELECT 1', [], function (tx, res) {
               equal(res.rows.item(0)['1'], 1);

               start();
             });
           }, function (error) {
             // XXX [BUG #230] iOS, Windows, and WP(8) versions of the plugin fail here:
             ok(false, 'transaction failed ' + error);

@sstraus
Copy link

sstraus commented Jan 15, 2016

Just a note, this issue happens on any platform, not only IOS.

@brodycj
Copy link
Contributor

brodycj commented Jan 15, 2016

This issue also breaks PouchDB compatibility (see storesafe/cordova-sqlcipher-adapter/issues/19).

A quick fix is to edit www/SQLitePlugin.js as follows:

$ git diff -U7 www
diff --git a/www/SQLitePlugin.js b/www/SQLitePlugin.js
index c9e2c88..e0a00b8 100644
--- a/www/SQLitePlugin.js
+++ b/www/SQLitePlugin.js
@@ -118,15 +118,15 @@
   };

   SQLitePlugin.prototype.readTransaction = function(fn, error, success) {
     if (!this.openDBs[this.dbname]) {
       error(newSQLError('database not open'));
       return;
     }
-    this.addTransaction(new SQLitePluginTransaction(this, fn, error, success, false, true));
+    this.addTransaction(new SQLitePluginTransaction(this, fn, error, success, true, true));
   };

   SQLitePlugin.prototype.startNextTransaction = function() {
     var self;
     self = this;
     nextTick((function(_this) {
       return function() {

@robwithhair
Copy link
Author

Great, I'll try the quick fix and see if it helps. Currently using an older fork of this project when it was passing pouch tests and it's working ok. Lots of out of memory issues on iOS that I am now debugging but don't know if that is pouch or the old fork of this project. Probably pouch sync trying to allocate too much. @brodybits might using your lower memory version of this project (under a different licence) help? Suppose it couldn't hurt to try. Will probably also require this patch?

@brodycj brodycj mentioned this issue Feb 12, 2016
brodycj pushed a commit to brodycj/Cordova-sqlite-storage-common-dev that referenced this issue Feb 12, 2016
Move & fix existing empty tx test
Add more empty tx tests
Put db name check test in its own describe block
brodycj pushed a commit to brodycj/Cordova-sqlite-storage-common-dev that referenced this issue Feb 12, 2016
@brodycj
Copy link
Contributor

brodycj commented Feb 12, 2016

A workaround fix is published.

@brodycj brodycj closed this as completed Feb 12, 2016
@kellyChap
Copy link

I found that reverting to 0.8.2 version of the plugin allows it to work on Android as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants