api.js doesn't meet Closure compiler requirement which effectively results in Statement memory never freed

[saaj]

If I take sql.js 1.5, say from CDNJS, and take https://cdnjs.cloudflare.com/ajax/libs/sql.js/1.5.0/sql-wasm.js, and un-minify it, this particular part of Statment's prototype is interesting:

a.prototype.reset = function() {
    return 0 === Dc(this.Ra) && 0 === Cc(this.Ra)
};
a.prototype.freemem = function() {
    for (var h; void 0 !== (h = this.lb.pop());) na(h)
};
a.prototype.free = function() {
    var h = 0 === Ec(this.Ra);
    delete this.db.fb[this.Ra];
    this.Ra = 0;
    return h
};

Now if I take the same part of source code (same with compiled without --closure 1) it looks like this:

Statement.prototype["reset"] = function reset() {
    this.freemem();
    return (
        sqlite3_clear_bindings(this.stmt) === SQLITE_OK
        && sqlite3_reset(this.stmt) === SQLITE_OK
    );
};
Statement.prototype["freemem"] = function freemem() {
    var mem;
    while ((mem = this.allocatedmem.pop()) !== undefined) {
        _free(mem);
    }
};
Statement.prototype["free"] = function free() {
    var res;
    this.freemem();
    res = sqlite3_finalize(this.stmt) === SQLITE_OK;
    delete this.db.statements[this.stmt];
    this.stmt = NULL;
    return res;
};

What's the difference? It is that Statment.prototype.freemem calls have disappeared (there's in fact only 1 occurrence of "freemem" in the minified version, and it's the defintion) hence, unless it's called manually, Statement memory is never freed, even on Database.prototype.close. Hence a memory leak.

It's likely related to unmet Closure compilers advanced optimisation code requirements (though I didn't check whether emcc actually uses the advanced mode) and probably inconsistent method naming in api.js.

Solution: Be Consistent in Your Property Names

This solution is pretty simple. For any given type or object, use dot-syntax or quoted strings exclusively. Don't mix the syntaxes, especially in reference to the same property.

Also, when possible, prefer to use dot-syntax, as it supports better checks and optimizations. Use quoted string property access only when you don't want Closure Compiler to do renaming, such as when the name comes from an outside source, like decoded JSON.

This PR can used for reproduction of the memory leak lana-k/sqliteviz#70. It's how I discovered the problem.

[lovasoa]

Thank you very much for the bug report and the detailed analysis ! This is a good find.
The behavior of the closure compiler is quite surprising, I would have expected it to raise an error or not attempt minimization when it encounters a call to a method it doesn't know.

I think the fix is pretty simple: we need to declare the methods unquoted in order to be able to use them unquoted :

Statement.prototype.free = function free() { ... };
Statement.prototype["free"] = Statement.prototype.free;

this way we will still benefit from the minimization, and get access to the full method names.

@saaj : Are you interested in opening a PR ?

[saaj]

I can make a PR that removes --closure 1 from the Makefile. That's plus ~50KB to sql-wasm.js, which isn't a big deal in my opinion (e.g. to make a quick fix).

But I don't have an idea how to make changes in api.js robust against Closure compiler's code requirements, and I'm not interested to figure out.

[lovasoa]

The bug report in itself is already a great contribution. I'll make the change myself when I'll have the time, then :)

[lovasoa]

This shoud be fixed in the latest version. Thanks again, @saaj !