939072 - Consider auto-closing Sqlite.jsm databases

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Now that we have support for post-mortem finalization in JS, we can consider auto-closing Sqlite.hsm databases upon garbage-collection.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

mak, gps, do you think this would be useful/interesting?

Comment 2

[Gregory Szorc [:gps]]

We have support for finalization in JS now?! Details!

When I originally wrote Sqlite.jsm, I had ideas for having connections automatically close after N seconds of inactivity so memory could be regained, etc. I'm all for implementing this!

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gregory Szorc [:gps] from comment #2)
> We have support for finalization in JS now?! Details!

Well, not officially :)
But we have http://dxr.mozilla.org/mozilla-central/source/toolkit/components/finalizationwitness/nsIFinalizationWitnessService.idl

> When I originally wrote Sqlite.jsm, I had ideas for having connections
> automatically close after N seconds of inactivity so memory could be
> regained, etc. I'm all for implementing this!

Ok, I'll try and work on this when I find some time.

Comment 4

[Marco Bonardo [:mak]]

as gps said, it was already an intended feature.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

I won't have time to handle this in the near future. If anyone wants to pick this bug, they're welcome.

Comment 6

[Michael Hoffmann]

Attachment: sqlitefinalize.patch

Ok, I'll take a shot at this.  I've attached a patch, is it a step in the right direction?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8369315 [details] [diff] [review]
sqlitefinalize.patch

Review of attachment 8369315 [details] [diff] [review]:
-----------------------------------------------------------------

I believe that this can work. However, this is probably hard-to-maintain.

Let me suggest a refactoring that would have equivalent semantics but that I believe would be clearer:
- every instance of OpenedConnection should contain a field |_data| (or something such) that contains all the fields that are currently directly in that instance (you'll need to rewrite lots of code to make this happen, so that probably deserves one patch by itself);
- in |OpenedConnections|, save a reference to |_data|;
- add a field |_witness| or something such, directly in the instance, for the FinalizationWitness;
- for auto-closing, reconstruct a simili-|OpenedConnection| using |_data| and call |close()|.

::: toolkit/modules/Sqlite.jsm
@@ +157,5 @@
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connection = OpenConnections.extract(aValue);
> +
> +  // Close the connection
> +  // There probably should be some kind of warning logged here...

Good point.

@@ +161,5 @@
> +  // There probably should be some kind of warning logged here...
> +  try {
> +    connection._close();
> +  } catch (ex) {
> +    Cu.reportError("Error when auto-closing sqlite connection: " + ex);

Take a look at how the other functions in this module log errors. It's more powerful than just Cu.reportError.

Comment 8

[Michael Hoffmann]

Attachment: Refactor OpenedConnection to store information in |_data|

Thanks for the suggestions.  Here I have attached one patch just for the refactoring part.

And here's the try results (just with the old commit name)
https://tbpl.mozilla.org/?tree=Try&rev=2d50ae74fb3b

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8371740 [details] [diff] [review]
Refactor OpenedConnection to store information in |_data|

Review of attachment 8371740 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Looking at the code, I realize that you should create a constructor ConnectionData for _data and the companion prototype. That prototype should define all the methods needed for closing the connection and the methods of OpenedConnection should dispatch to _data's methods for these methods.

Also, please document why you need this. Once this is done, ask :gps for a review, he's the owner of this code.

Comment 10

[Michael Hoffmann]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #9)
> Comment on attachment 8371740 [details] [diff] [review]
> Refactor OpenedConnection to store information in |_data|
> 
> Review of attachment 8371740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. Looking at the code, I realize that you should create a
> constructor ConnectionData for _data and the companion prototype. That
> prototype should define all the methods needed for closing the connection
> and the methods of OpenedConnection should dispatch to _data's methods for
> these methods.
> 
> Also, please document why you need this. Once this is done, ask :gps for a
> review, he's the owner of this code.

Thanks, I'll do that.  Just to make sure I got this right, I need only to move the methods needed for closing into the ConnectionData prototype, so that I can easily auto-close it later if needed?  That is, I keep the other not-used-for-closing methods as they currently are in my patch, inside OpenedConnection and referring to instance data with this._data.<stuff>?

And the ConnectionData constructor should just encapsulate the initialization that's currently inside OpenedConnection constructor?

Comment 11

[David Teller [:Yoric] - still alive but not very active]

(In reply to Michael Brennan from comment #10)
> Thanks, I'll do that.  Just to make sure I got this right, I need only to
> move the methods needed for closing into the ConnectionData prototype, so
> that I can easily auto-close it later if needed?

Exactly. The idea is to auto-close the ConnectionData rather than the OpenedConnection. Once your work on this bug is complete, an instance of OpenedConnection will contain both an instance of ConnectionData (_data) and an instance of FinalizationWitness (_finalizer). Both the OpenedConnection and the _finalizer (with the help of your OpenedConnections data structure) will be able to trigger closing of the instance of ConnectionData. Keeping _finalizer out of _data ensures that we can keep a reference to _data without preventing _finalizer from being garbage-collected.

>  That is, I keep the other
> not-used-for-closing methods as they currently are in my patch, inside
> OpenedConnection and referring to instance data with this._data.<stuff>?

This seems like the simplest way to do it. :gps might ask you to rework this a little, we shall see.

> And the ConnectionData constructor should just encapsulate the
> initialization that's currently inside OpenedConnection constructor?

Indeed.

Comment 12

[Michael Hoffmann]

Attachment: Refactor OpenedConnection to use a OpenedData object

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8375326 [details] [diff] [review]
Refactor OpenedConnection to use a OpenedData object

Review of attachment 8375326 [details] [diff] [review]:
-----------------------------------------------------------------

I would have called it this._connection, this._conn, or even this._c instead of this._data. But that's a bikeshed.

I like how you've encapsulated all the connection state/data into a self-contained object. That makes it much easier to throw state away and start with a new connection.

I didn't perform line-by-line verification that the supposedly copied code was copied without modifications. Should I be looking for any changes?

While I'm granting r+, I'd like to throw out a slightly alternative proposal. Instead of having OpenedConnection proxy methods via OpenedData, have you considered dynamically changing the .prototype of the connection? When the connection is open, the active prototype's execute() is defined to do the right thing. When the connection is closed, execute() will open a new connection, swap in the appropriate prototype (this.prototype = OpenedConnection), and call this.execute() (hitting the new prototype this time). I /think/ the current patch is pretty close to that - you just need to change some constructors around and set up prototype inheritance.

Again, that's just an idea. I'm still granting r+ (on the condition lines didn't change as part of the refactor).

::: services/metrics/tests/xpcshell/test_metrics_storage.js
@@ +98,5 @@
>  
>  add_task(function test_checkpoint_apis() {
>    let backend = yield Metrics.Storage("checkpoint_apis");
>    let c = backend._connection;
> +  let count = c._data._statementCounter;

The fact this file is accessing internal APIs and had to be changed as part of this patch is unfortunate. We should make statementCounter a public/proxied API. Follow-up bug.

::: toolkit/modules/Sqlite.jsm
@@ +597,5 @@
>     *
>     * @return Promise<>
>     */
>    close: function () {
> +    return this._data.close();

What if this._data is not defined? I guess I'll wait for a future patch to see!

Comment 14

[Michael Hoffmann]

(In reply to Gregory Szorc [:gps] from comment #13)

Thanks for the review!

> I would have called it this._connection, this._conn, or even this._c instead
> of this._data. But that's a bikeshed.

There's no problem changing it. But _connection is already in use in the code, and it could be confusing to have them too similar. Do you want me to change the name of _connection to something else if I would change the name of _data, to _conn for instance?

> I like how you've encapsulated all the connection state/data into a
> self-contained object. That makes it much easier to throw state away and
> start with a new connection.
> 
> I didn't perform line-by-line verification that the supposedly copied code
> was copied without modifications. Should I be looking for any changes?

No, it's not necessary. I've only copied the code.

> While I'm granting r+, I'd like to throw out a slightly alternative
> proposal. Instead of having OpenedConnection proxy methods via OpenedData,
> have you considered dynamically changing the .prototype of the connection?
> When the connection is open, the active prototype's execute() is defined to
> do the right thing. When the connection is closed, execute() will open a new
> connection, swap in the appropriate prototype (this.prototype =
> OpenedConnection), and call this.execute() (hitting the new prototype this
> time). I /think/ the current patch is pretty close to that - you just need
> to change some constructors around and set up prototype inheritance.

Sounds good to try to avoid the proxying and simply use the correct prototype instead. But I got a bit confused from the example. You say that if we have a connection that is closed and call execute(), then it should open a new connection. Is that to add new functionality to be able to reopen an already closed connection? Or by opening a new connection, do you mean to create a new object on which we can change the prototype?

In either case, that would mean that we would only have one object for the connection, right? Where would we later put the finalization witness? The reference that we have to maintain in order to close on finalization would have to point to that object, and effectively prevent the connection object from being garbage collected. But I might just have misunderstood the idea here.

Comment 15

[Marco Bonardo [:mak]]

I also think _data and openedData looks too generic and confusing. openedConnection or connectionHandler may be fine... the internal _connection may be _dbConn, _rawConn or similarly named.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

(In reply to Michael Brennan from comment #14)
> Sounds good to try to avoid the proxying and simply use the correct
> prototype instead. But I got a bit confused from the example. You say that
> if we have a connection that is closed and call execute(), then it should
> open a new connection. Is that to add new functionality to be able to reopen
> an already closed connection? Or by opening a new connection, do you mean to
> create a new object on which we can change the prototype?

Whatever you do, please make sure that you end up with the most readable solution. Going higher-order with prototypes just to avoid ~20loc is probably not a good investment.

Comment 17

[Gregory Szorc [:gps]]

We have the following states:

1) A "ready connection"
2) An "open connection"
3) A "closed connection"

A "ready connection" holds the necessary parameters to establish a connection.

I argue a "closed connection" can be modeled as an "open connection" that has transitioned into the "ready" state and is thus the same as a "ready connection." Although, we may wish to explicitly track a state that refuses to allow a closed connection to be reopened (say after shutdown has been initiated).

I think we can make things look like the following:

openConnection(options) -> ReadyConnection

ReadyConnection = function (options) {
  this._connOptions = options;
  this._connection = null;
  this._openAllowed = true;
}

ReadyConnection.prototype.execute = function (sql, params=null, onRow=null) {
  return this.open().then(() => {
    return this.execute(sql, params, onRow);
  });
};

ReadyConnection.prototype.open = function () {
  let deferred = Promise.defer();

  Services.storage.openAsyncDatabase(this._file, this._options, (status, connection) => {
    this._connection = connection;
    this.prototype = OpenedConnection;

    deferred.resolve(this);
  });

  return deferred;
};

OpenedConnection.prototype.execute = function (sql, params=null, onRow=null) {
  // Existing execute() function.
};

OpenedConnection.prototype.close = function () {
  // Existing close code.
  this._connection = null;
  this.prototype = ReadyConnection;

  return deferred;
};

We can either group all the "opened connection" variables inside a new object (like the existing patch), or we can leave them as member variables on the base object. I don't think it matters too much if you are doing dynamic prototypes.

I think this approach is more end-user friendly because it abstracts the complex logic of connection state management away from the user. They have a simple-to-use object that can be opened or closed (for performance reasons) without their knowledge or requirement to do anything special. That's extremely powerful and empowers us to change Sqlite.jsm without auditing all the callers.

What do you think of this approach?

Comment 18

[Michael Hoffmann]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #16)
> (In reply to Michael Brennan from comment #14)
> > Sounds good to try to avoid the proxying and simply use the correct
> > prototype instead. But I got a bit confused from the example. You say that
> > if we have a connection that is closed and call execute(), then it should
> > open a new connection. Is that to add new functionality to be able to reopen
> > an already closed connection? Or by opening a new connection, do you mean to
> > create a new object on which we can change the prototype?
> 
> Whatever you do, please make sure that you end up with the most readable
> solution. Going higher-order with prototypes just to avoid ~20loc is
> probably not a good investment.

I'll do my best :)

Comment 19

[Michael Hoffmann]

I'm trying out your proposal and I'll attach a patch to show how far I got with it.

(In reply to Gregory Szorc [:gps] from comment #17)
> We can either group all the "opened connection" variables inside a new
> object (like the existing patch), or we can leave them as member variables
> on the base object. I don't think it matters too much if you are doing
> dynamic prototypes.

I think it would be a lot easier to have this data together like it is now to be able to auto-close it with the help of a finalization witness later.

> I think this approach is more end-user friendly because it abstracts the
> complex logic of connection state management away from the user. They have a
> simple-to-use object that can be opened or closed (for performance reasons)
> without their knowledge or requirement to do anything special. That's
> extremely powerful and empowers us to change Sqlite.jsm without auditing all
> the callers.
> 
> What do you think of this approach?

Sounds like a nice idea, but I don't see why dynamic prototypes are needed for this. If you look at my patch, it currently switches between the prototypes when opening and closing connections. So the set of methods exposed changes and thus the interface towards the user changes. To be sure to call the right methods the client would still have to track the state of the connection object, which we don't want. So maybe ReadyConnection is also intended to inherit from OpenedConnection? To keep the interface constant.

Right now there's three objects in play: ReadyConnection, OpenedConnection and OpenedData. OpenedData could renamed to make its intent more clear, but currently an instance of it is stored in _openedConnection when the connection is really open (the old _data member). I don't know if your suggestion also mean that OpenedData should be merged into OpenedConnection, to limit this to only two objects. But if that is the case, then I suppose we would have an instance of OpenedConnection stored in _openedConnection but at the same time the containing object would have a prototype of OpenedConnection, which I think would be quite confusing.

Perhaps I'm missing something vital here, but to me it seems easier to provide one interface towards the user in just a regular object, and keep the connection state management private. Then we could still have the connection management abstracted away from the user. What do you think?

Lastly, doesn't it look a bit weird to allow something like the following?

connection = openConnection({ path: somePath });
connection.open();  // wasn't it open already?

If we relieve the user from keeping track of the connection's state, is it too prohibitive to disallow the client from calling open? Otherwise, maybe some renaming could help?

Comment 20

[Michael Hoffmann]

Attachment: Tryout with dynamic prototypes

Added ReadyConnection. Renamed _connection to _dbConn, and _data to _openedConnection.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8393047 [details] [diff] [review]
Tryout with dynamic prototypes

Review of attachment 8393047 [details] [diff] [review]:
-----------------------------------------------------------------

At a cursory glance, this looks good to me.

::: toolkit/modules/Sqlite.jsm
@@ +462,3 @@
>  }
>  
> +function ReadyConnection(options, path, openedOptions) {

Documentation needed.

@@ +479,5 @@
> +  open: function () {
> +    let deferred = Promise.defer();
> +    let log = Log.repository.getLogger("Sqlite.ConnectionOpener");
> +
> +    let file = FileUtils.File(this._path);

Actually, we don't need |file|. Just passing |this._path| to |openAsyncDatabase| should work nicely.

@@ +626,5 @@
>     *
>     * @return Promise<>
>     */
>    close: function () {
> +    var promise = this._openedConnection.close();

Please use let instead of var.

@@ +628,5 @@
>     */
>    close: function () {
> +    var promise = this._openedConnection.close();
> +    this._openedConnection = null;
> +    this.__proto__ = ReadyConnection.prototype;

This needs careful documentation.

Comment 22

[Jeff Walden [:Waldo]]

So, prototype mutation like this is Bad.  It's slow initially, and you forever taint property access speed on that object, and objects along its prototype chain.  Don't do it!

In this case, if I read the patch correctly, you have connection objects.  They contain a _dbConnection field that's (presumably) connection info.  And that's it.  And you have a prototype with methods that act on that connection info.  And the only reason -- I think -- that you have prototype mutation here, is because you think it gets rid of overhead to test whether the connection is open on each method-call?

First, I would question whether the overhead of such a test is so bad.  If it's just checking the value of a single integer/boolean property of this._dbConnection or so, it's not going to amount to enough win overall to justify other approaches.  It's not going to overcome the cost of prototype mutation, both initially and on an ongoing basis.

If the test really is sort of bad -- which, honestly, seems unlikely when there's such a clear state-change here -- I'd probably have a single field that's a "connection implementation" object.  Then your Connection.prototype.foo() method could just forward to |this._impl.foo()|, and you wouldn't have an extra check.  The method-forwarding shouldn't cost much once the JITs touch this...I hope.  I'm not sure that totally-plain forwarding of arguments and method calls will translate with utmost optimality.

All things considered, I'd just have the connection methods check a field to know current state and leave it at that.  Prototype mutation isn't worth it for this.

Comment 23

[Gregory Szorc [:gps]]

Jeff: What you say is true. But I thought the discussion on dev-platform that brought you here eventually concluded that the overhead is not significant. It's on the order of 20 cycles or something. It really only impacts heavily JIT optimized code. Etc. Chrome JS probably won't feel it. Am I wrong?

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8393047 [details] [diff] [review]
Tryout with dynamic prototypes

Review of attachment 8393047 [details] [diff] [review]:
-----------------------------------------------------------------

I like the approach!

Comment 25

[David Teller [:Yoric] - still alive but not very active]

I believe that prototype mutation:
- makes things harder to follow for most potential contributors;
- will cause a warning to be displayed, which is bad for debugging.

I therefore vote against prototype mutation.

Comment 26

[Michael Hoffmann]

How shall I continue with this issue?
If I were to choose I'd skip mutating the prototype mostly because of readability, but I am fine continuing with that approach also, if we decide to go on with it.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

For the first version, don't mutate the prototype. If gps believe that mutating it would really improve code quality, let's evaluate that in a followup bug.

Comment 28

[Michael Hoffmann]

Attachment: sqlitefinalize-part1, refactorization

Essentially same as previous version, except some updated names and merged with newer changes in the tree.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8422737 [details] [diff] [review]
sqlitefinalize-part1, refactorization

I believe that gps is on sabbatical. Taking over the review.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8422737 [details] [diff] [review]
sqlitefinalize-part1, refactorization

Review of attachment 8422737 [details] [diff] [review]:
-----------------------------------------------------------------

Quick remark: a non-trivial patch to Sqlite.jsm has landed yesterday. It affects opening and closing. You should make sure to update your patch so that it still works after these changes.

Comment 31

[Michael Hoffmann]

Attachment: sqlitefinalize-part2, auto-close

I haven't added any tests. I could add a test that sends a notification and asserts that the connection is really closed, but I believe that I then need to add a promise that is resolved when the database is auto-closed, and also expose some internal data to the test.

I tried commenting out a close() in firefox and ran it. Quite often I get a crash since the database doesn't seem to close before it's too late. I suppose we could add an async shutdown blocker to solve that, but maybe it's easy to run into some condition where the blocker waits but the database doesn't get garbage collected.

What do you think about the above two issues, are they worth implementing?

Comment 32

[Michael Hoffmann]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 8th-12th) from comment #30)
> Comment on attachment 8422737 [details] [diff] [review]
> sqlitefinalize-part1, refactorization
> 
> Review of attachment 8422737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Quick remark: a non-trivial patch to Sqlite.jsm has landed yesterday. It
> affects opening and closing. You should make sure to update your patch so
> that it still works after these changes.

Oh, thanks for the heads up. I'll take a look at it.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8422745 [details] [diff] [review]
sqlitefinalize-part2, auto-close

Review of attachment 8422745 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with minor nits. Now, I'd like tests!

::: toolkit/modules/Sqlite.jsm
@@ +33,5 @@
>  let connectionCounters = new Map();
>  
> +// Used by finalization witnesses to be able to close opened
> +// connections on garbage collection.
> +let connIdToConnData = new Map();

Please document the keys and values.

@@ +35,5 @@
> +// Used by finalization witnesses to be able to close opened
> +// connections on garbage collection.
> +let connIdToConnData = new Map();
> +
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {

Some more comments would be useful.

@@ +38,5 @@
> +
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connectionIdentifier = aValue;
> +  let connectionData = connIdToConnData.get(connectionIdentifier);
> +  connIdToConnData.delete(aValue);

That would be connectionIdentifier.

@@ +41,5 @@
> +  let connectionData = connIdToConnData.get(connectionIdentifier);
> +  connIdToConnData.delete(aValue);
> +
> +  dump("Warning: Sqlite connection '" + connectionIdentifier +
> +       "' was not properly closed. Auto-close triggered by garbage collection.\n");

Let's use `Services.console` instead of `dump`. This will make it easier to test the property, as we can add observers.

@@ +43,5 @@
> +
> +  dump("Warning: Sqlite connection '" + connectionIdentifier +
> +       "' was not properly closed. Auto-close triggered by garbage collection.\n");
> +
> +  connectionData.close();

This will fail if it gets executed too late during shutdown. We will need to find a solution or at least handle this case through better error reporting.

@@ +614,5 @@
>    // field without preventing garbage collection of
>    // OpenedConnection. On garbage collection, we will still be able to
>    // close the database using this extra reference.
> +  this._openedConnection = new ConnectionData(connection, basename,
> +                                              number, options);

Let's not repaginate needlessly.

Comment 34

[Michael Hoffmann]

Attachment: sqlitefinalize-part1, refactorization v2

Updated and merged with updates in tree.

Comment 35

[Michael Hoffmann]

Attachment: sqlitefinalize-part2, auto-close v2

Tests added and changes suggested from feedback applied.

Comment 36

[Michael Hoffmann]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 16th-23th) from comment #33)
> @@ +43,5 @@
> > +
> > +  dump("Warning: Sqlite connection '" + connectionIdentifier +
> > +       "' was not properly closed. Auto-close triggered by garbage collection.\n");
> > +
> > +  connectionData.close();
> 
> This will fail if it gets executed too late during shutdown. We will need to
> find a solution or at least handle this case through better error reporting.
> 

I have not done anything for this one. Should we try to solve it here or leave it for a follow up bug?

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8428330 [details] [diff] [review]
sqlitefinalize-part1, refactorization v2

Review of attachment 8428330 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with the minor changes below.
As far as I'm concerned, if tests pass, this can get r+. mak, do you want to perform an additional review?

::: toolkit/modules/Sqlite.jsm
@@ +102,5 @@
>  /**
> + * Connection data with methods necessary for closing the connection.
> + *
> + * To support auto-closing in the event of garbage collection, this
> + * type contains all the connection data of an opened connection and

Nit: It's not a type, it's a data structure or a constructor.

@@ +154,5 @@
> +    this._deferredClose.promise
> +  );
> +}
> +
> +ConnectionData.prototype = Object.freeze({

I haven't checked every single line of this proto. Can you confirm that this is a simple cut-paste from OpenConnection.prototype?

@@ +678,5 @@
> +  // witness. This enables us to store an additional reference to this
> +  // field without preventing garbage collection of
> +  // OpenedConnection. On garbage collection, we will still be able to
> +  // close the database using this extra reference.
> +  this._openedConnection = new ConnectionData(connection, basename, number, options);

It's a bit weird to have a field `_openedConnection` in a constructor `OpenedConnection`. I'd call this `_connectionData`.

@@ +752,5 @@
>     *        the original connection.
>     *
>     * @return Promise<OpenedConnection>
>     */
>    clone: function (readOnly=false) {

I don't really like to see us hitting in a private field of another constructor (here, `_log`). I would forward this to a method `ConnectionData.prototype.clone`.

@@ +828,2 @@
>     */
>    executeCached: function (sql, params=null, onRow=null) {

I would forward this to a method `ConnectionData.prototype.executeCached`.

@@ +884,5 @@
>    /**
>     * Whether a transaction is currently in progress.
>     */
>    get transactionInProgress() {
> +    return this._openedConnection._open && !!this._openedConnection._inProgressTransaction;

Same here and for other methods below.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8428331 [details] [diff] [review]
sqlitefinalize-part2, auto-close v2

Review of attachment 8428331 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.
We'll need to also handle the case in which garbage-collection takes place too late, after the shutdown barrier, in which case we end up with a shutdown freeze/crash. That can wait for a followup bug, though.

::: toolkit/modules/Sqlite.jsm
@@ +45,5 @@
> + *
> + * Key: _connectionIdentifier of ConnectionData
> + * Value: ConnectionData object
> + */
> +let connIdToConnData = new Map();

For the sake of encapsulation, I would make this a field of constructor `ConnectionData` itself. This will let you rename it to something a little more readable, e.g. `byId`.

@@ +119,5 @@
> + *
> + * The observer is passed the connection identifier of the database
> + * connection that is being finalized.
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {

This call to `Services` would force Services.jsm to be imported during startup, which is something we want to avoid. Could you make this call to `addObserver` in the initialization of `Barrier`, for instance? Alternatively, make field `ConnectionData.byId` a lazy getter and add this observer during the initialization of `byId`.

Also, nit: in this file, we don't prefix arguments with "a". That's a C++-ism that we try to avoid in recent JS code (not always successfully).

@@ +120,5 @@
> + * The observer is passed the connection identifier of the database
> + * connection that is being finalized.
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connectionIdentifier = aValue;

Nit: Let's just call the argument `connectionIdentifier`, in that case.

@@ +122,5 @@
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> +  let connectionIdentifier = aValue;
> +  let connectionData = connIdToConnData.get(connectionIdentifier);
> +  connIdToConnData.delete(connectionIdentifier);

Is there a path that could lead to `connectionIdentifier` not being in the map?

@@ +126,5 @@
> +  connIdToConnData.delete(connectionIdentifier);
> +
> +  Services.console.logStringMessage(
> +    "Warning: Sqlite connection '" + connectionIdentifier +
> +    "' was not properly closed. Auto-close triggered by garbage collection.\n");

Could you make this a script error message?
See e.g. http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonLogging.jsm#89-96

And let's follow that example and put back the `dump` I asked you to remove, as a fallback.

Also, could you add a comment somewhere mentioning that `connectionIdentifier` contains the human-readable name of the database?

@@ +618,5 @@
>      throw new TypeError("Connection must be a valid Storage connection.");
>    }
>  
>    if (isClosed) {
> +    throw new Error("Sqlite.jsm has been shutdown. Cannot clone connection to: " + source.database.path);

Ah, thanks for the typo fix.

@@ +718,5 @@
> +  // Store the extra reference in a map with connection identifier as
> +  // key. Then create the finalization witness.
> +  connIdToConnData.set(this._openedConnection._connectionIdentifier,
> +                       this._openedConnection);
> +  this._witness = FinalizationWitnessService.make(

This field deserves more documentation.

@@ +780,5 @@
>    close: function () {
> +    // An entry in the map indicates we have an active witness. Since
> +    // we are closing, tell the witness to forget us. This condition
> +    // should always evaluate to true unless close is called more than
> +    // once.

I'd prefer a slightly shorter line. Can you call the id `id`?

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +928,5 @@
> +  let warningLogged = false;
> +  let listener = {
> +    observe: function(aMessage) {
> +      let messageText = aMessage.message;
> +      if (messageText.indexOf("Warning: Sqlite connection") !== -1) {

Could you make sure that the connectionIdentifier also appears in the message?

@@ +948,5 @@
> +
> +  do_check_true(warningLogged);
> +});
> +
> +add_task(function test_forget_witness_on_close() {

function*

@@ +963,5 @@
> +
> +  yield c.close();
> +  // After close, witness should have forgotten the connection
> +  do_check_true(forgetCalled);
> +});

Now, I'd like a test about garbage-collection. Since gc is not an exact science, I would go for something along the lines of:

add_task(function* foo() {

  let deferred = Promise.defer();

  for (let i = 0; i < 100; ++i) {
    let c = yield getDummyDatabase(...);

    // Release `deferred` once one of the databases has been closed by the gc
    // (any of them will do).
    c._openedConnection._deferredClose.promise.then(deferred.promise.resolve); 
  }

  Components.utils.forceGC();
  yield deferred.promise;
});

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 8428330 [details] [diff] [review]
sqlitefinalize-part1, refactorization v2

Review of attachment 8428330 [details] [diff] [review]:
-----------------------------------------------------------------

I think the review job has been very good here so far, I don't have any important points to bring out, so I'll let the final pass to David who greatly managed it so far.

I'm just pointing out some nits about very simple cleanup that we may do while we are in the process of movi this code (some or most are not related to the changes we are making, but sounds like a good time for trivial cleanup)

::: toolkit/modules/Sqlite.jsm
@@ +119,5 @@
> +function ConnectionData(connection, basename, number, options) {
> +  this._log = Log.repository.getLoggerWithMessagePrefix("Sqlite.Connection." + basename,
> +                                                        "Conn #" + number + ": ");
> +
> +  this._log.info("Opened");

nit: remove the newline above this.

@@ +258,5 @@
> +    this._open = false;
> +
> +    this._log.debug("Calling asyncClose().");
> +    this._dbConn.asyncClose({
> +      complete: function () {

please, while moving this, make it:

this._dbConn.asyncClose(() => {
  ...
});

first of all mozIStorageCompletionCallback has the function decorator (so it doesn't need to be an object with a complete method), second arrow function allows you to avoid the .bind(this)

@@ +286,5 @@
> +    let onFinished = function () {
> +      this._anonymousStatements.delete(index);
> +      statement.finalize();
> +      this._startIdleShrinkTimer();
> +    }.bind(this);

also, please make this an arrow function, so you can remove .bind(this)

@@ +297,5 @@
> +          onFinished();
> +          deferred.resolve(rows);
> +        }.bind(this),
> +
> +        function onError(error) {

as well as here we may have

rows => {...},
error  => {...}

(regardless I don't see why we are using .bind(this) on these 2 functions, this is not even used and onFinished is already bound)

@@ +315,5 @@
> +    this._log.info("Shrinking memory usage.");
> +
> +    let onShrunk = this._clearIdleShrinkTimer.bind(this);
> +
> +    return this.execute("PRAGMA shrink_memory").then(onShrunk, onShrunk);

nit: I don't see why there are so many newlines here :)

Comment 40

[Marco Bonardo [:mak]]

Comment on attachment 8428331 [details] [diff] [review]
sqlitefinalize-part2, auto-close v2

Review of attachment 8428331 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from David's comments that look sane:

::: toolkit/modules/Sqlite.jsm
@@ +119,5 @@
> + *
> + * The observer is passed the connection identifier of the database
> + * connection that is being finalized.
> + */
> +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {

I think the topic is expected just once, so the observer should be removed on first use?

Comment 41

[Michael Hoffmann]

(In reply to David Rajchenbach Teller [:Yoric] from comment #37)
> @@ +154,5 @@
> > +    this._deferredClose.promise
> > +  );
> > +}
> > +
> > +ConnectionData.prototype = Object.freeze({
> 
> I haven't checked every single line of this proto. Can you confirm that this
> is a simple cut-paste from OpenConnection.prototype?

Yes, the methods are unmodified with the exception of some renamings of `_connection` to `_dbConn`.

> @@ +678,5 @@
> > +  // witness. This enables us to store an additional reference to this
> > +  // field without preventing garbage collection of
> > +  // OpenedConnection. On garbage collection, we will still be able to
> > +  // close the database using this extra reference.
> > +  this._openedConnection = new ConnectionData(connection, basename, number, options);
> 
> It's a bit weird to have a field `_openedConnection` in a constructor
> `OpenedConnection`. I'd call this `_connectionData`.

Ah, I found this myself some time ago and thought it was weird. I also thought I had fixed it, but apparently not :-) Thanks for noticing!

> 
> @@ +828,2 @@
> >     */
> >    executeCached: function (sql, params=null, onRow=null) {
> 
> I would forward this to a method `ConnectionData.prototype.executeCached`.
> 
> @@ +884,5 @@
> >    /**
> >     * Whether a transaction is currently in progress.
> >     */
> >    get transactionInProgress() {
> > +    return this._openedConnection._open && !!this._openedConnection._inProgressTransaction;
> 
> Same here and for other methods below.


While fixing the remaining methods by forwarding methods calls to `ConnectionData` I ran into a problem where I need some help.

According to the documentation comment, `OpenedConnection.prototype.executeTransaction` takes a callback which, when called, will be passed the connection instance, which here is the `OpenedConnection` instance. So if this method is moved into `ConnectionData.prototype.executeTransaction` it will need a reference to the `OpenedConnection` instance for it to be able to call the callback with the right argument.

But passing `this` when calling `ConnectionData.prototype.executeTransaction` sounds really bad. We don't want a reference back to the `OpenedConnection` preventing garbage collection. Maybe the lifetime of the closure is short enough to pose no problem, I haven't tried yet, but to me this looks like something we should really try to avoid.

What are your opinion on this?

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Good question.
Indeed, we don't want to pass the instance of `ConnectionData` nor to have a backref. So we need to make sure that `func` knows about the instance of `OpenedConnection` by the time it reaches `ConnectionData.prototype.executeTransaction`, as follows:

OpenedConnection.prototype.executeTransaction = function(func, ...) {
  return this._connectionData.executeTransaction(() => func(this), ...);
}

Comment 43

[Michael Hoffmann]

(In reply to Marco Bonardo [:mak] from comment #40)
> Comment on attachment 8428331 [details] [diff] [review]
> sqlitefinalize-part2, auto-close v2
> 
> Review of attachment 8428331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apart from David's comments that look sane:
> 
> ::: toolkit/modules/Sqlite.jsm
> @@ +119,5 @@
> > + *
> > + * The observer is passed the connection identifier of the database
> > + * connection that is being finalized.
> > + */
> > +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> 
> I think the topic is expected just once, so the observer should be removed
> on first use?

It will be called for every connection that gets garbage-collected, so it should stay until the end. But thanks for the reminder, I've added a removeObserver at shutdown.

Comment 44

[Michael Hoffmann]

Attachment: sqlitefinalize-part1, refactorization v3

Comment 45

[Michael Hoffmann]

Attachment: sqlitefinalize-part2, auto-close v3

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8438166 [details] [diff] [review]
sqlitefinalize-part2, auto-close v3

Review of attachment 8438166 [details] [diff] [review]:
-----------------------------------------------------------------

I like that patch a lot. I'll be happy to r+ it once you have finished the other part.

::: toolkit/modules/Sqlite.jsm
@@ +50,5 @@
> +  consoleMessage.init(message, stack.fileName, null, stack.lineNumber, 0,
> +                      Ci.nsIScriptError.errorFlag, "component javascript");
> +  Services.console.logMessage(consoleMessage);
> +
> +  // Always dump errors, in case the Console Service isn't listening yet

Nit: "or isn't listening anymore".

@@ +119,5 @@
>        // Now, wait until all databases are closed
>        yield Barriers.connections.wait();
> +
> +      // Everything closed, no finalization events to catch
> +      Services.obs.removeObserver(finalizationObserver, "sqlite-finalization-witness");

Good idea.

@@ +938,5 @@
>     * @return Promise<>
>     */
>    close: function () {
> +    // Unless cleanup has already been done by a previous call to
> +    // `close', delete the database entry from map and tell the

Nit: Maybe `close` (rather than `close'), for consistency?

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +996,5 @@
> +  // references. This is needed at the moment, otherwise
> +  // garbage-collection takes place after the shutdown barrier and the
> +  // test will timeout. Once that is fixed, we can remove this line
> +  // and be fine as long as at least one connection is
> +  // garbage-collected.

Really? That's sad.

Comment 47

[Michael Hoffmann]

Attachment: sqlitefinalize-part1, refactorization v3

Thank you David.
I've applied changes from feedback. Adding review flag to both patches and hope everything looks alright.

Comment 48

[Michael Hoffmann]

Attachment: sqlitefinalize-part2, auto-close v4

Comment 49

[Michael Hoffmann]

https://tbpl.mozilla.org/?tree=Try&rev=1bbfea2bf116

Comment 50

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8438738 [details] [diff] [review]
sqlitefinalize-part1, refactorization v3

Review of attachment 8438738 [details] [diff] [review]:
-----------------------------------------------------------------

Could you double-check the `func` (see below)? Other than that, this seems good to me, with minor nits.

::: toolkit/modules/Sqlite.jsm
@@ +271,5 @@
>        this._startIdleShrinkTimer();
>        throw ex;
>      }
>  
>      return deferred.promise;

All this block (from `let deferred` to the final `return`) could be rewritten as follows:

return Task.spawn(function*() {
  try {
    return yield this._executeStatement(sql, statement, params, onRow);
  } finally {
    this._startIdleShrinkTimer();
  }
}.bind(this));

Which would also provide better stack backtraces in case of error. Your call, you can either do it now or keep it for a followup bug.

@@ +311,5 @@
>        onFinished();
>        throw ex;
>      }
>  
>      return deferred.promise;

Same here.

@@ +337,5 @@
>        yield this.execute("BEGIN " + type + " TRANSACTION");
>  
>        let result;
>        try {
> +        result = yield Task.spawn(func);

Shouldn't this be `func()`?

@@ +342,5 @@
>        } catch (ex) {
>          // It's possible that a request to close the connection caused the
>          // error.
> +        // Assertion: close() will unset
> +        // this._inProgressTransaction when called.

Please don't repaginate.

@@ +510,5 @@
>        },
>  
>        handleError: function (error) {
> +        self._log.info("Error when executing SQL (" +
> +                       error.result + "): " + error.message);

Please don't repaginate.

Comment 51

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8438740 [details] [diff] [review]
sqlitefinalize-part2, auto-close v4

Review of attachment 8438740 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +986,5 @@
> +
> +add_task(function* test_close_database_on_gc() {
> +  let deferred = Promise.defer();
> +
> +  for (let i = 0; i < 100; ++i) {

Can you add a comment that we do it 100 times to encourage the gc to collect at least one of the instances?

Comment 52

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8438740 [details] [diff] [review]
sqlitefinalize-part2, auto-close v4

Review of attachment 8438740 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +51,5 @@
> +                      Ci.nsIScriptError.errorFlag, "component javascript");
> +  Services.console.logMessage(consoleMessage);
> +
> +  // Always dump errors, in case the Console Service isn't listening anymore
> +  dump("*** " + message + "\n");

Last minute idea.
Instead of dump(...), you should call `Promise.reject(new Error(message));`. In addition to displaying the error message, it will also cause unit tests to fail if we reach this line.

Note: If this idea causes tests to fail immediately, we can keep it for a followup bug.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #38)
> > +Services.obs.addObserver(function observe(aSubject, aTopic, aValue) {
> 
> This call to `Services` would force Services.jsm to be imported during
> startup, which is something we want to avoid.

In general this is good advice, but Services.jsm specifically is likely already loaded before anyone invokes Sqlite.jsm, so I doubt this matters in practice, right?

Comment 54

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Gavin Sharp (email [e-mail]) from comment #53)
> In general this is good advice, but Services.jsm specifically is likely
> already loaded before anyone invokes Sqlite.jsm, so I doubt this matters in
> practice, right?

Well, the two reasons are:
1. we have rewritten some of Sqlite.jsm specifically to import as little as possible during startup, so let's not waste the effort;
2. bug 859912.

But yes, here, it's not a big deal.

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Any chance you could wrap this up quickly, Michael?

Comment 56

[Michael Hoffmann]

Attachment: Folded sqlitefinalize patch

I've folded the patches with the suggestions from the review added, including changing `func` to `func()` in the task spawning.

However, `Promise.reject(new Error(message));` caused the tests to fail so I left the `dump` unchanged.

Shall I add the flag for checkin-needed?

Comment 57

[David Teller [:Yoric] - still alive but not very active]

Yes, please do.
Don't forget to paste a link to your latest (successful) Try run.

Comment 58

[Michael Hoffmann]

https://tbpl.mozilla.org/?tree=Try&rev=1a2650fd2437

Those Marionette-tests that fail don't seem to have been there on my previous Try run. I don't think they are related to my change, but they don't seem intermittent either. How should I proceed here?

Comment 59

[David Teller [:Yoric] - still alive but not very active]

Looks like an infrastructure problem, indeed. Let's mark this bug as checkin-needed.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Those Marionette failures weren't there on your previous Try run because they were only recently enabled on Windows. I've retriggered them, but that sure looks suspicious to me.

And FWIW, consistent failures across all opt/debug builds of the same platform generally doesn't say "infrastructure" to me ;)

Comment 61

[Ryan VanderMeulen [:RyanVM]]

My only thought on the failures is that maybe you pushed against an old head (rather than mozilla-central tip) and you picked up an incidental test failure along the way (since there were some failures that needed fixing before Mn could be enabled on Windows). A good reason to always make sure your Try pushes are on top of m-c tip, FWIW ;).

Anyway, please do a push on top of m-c tip with the below syntax to verify that things are OK:
try: -b do -p win32 -u marionette -t none

Comment 62

[Michael Hoffmann]

https://tbpl.mozilla.org/?tree=Try&rev=81f4553679b5

Comment 63

[Michael Hoffmann]

Attachment: Folded sqlitefinalize patch, latest on top of m-c

Comment 64

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/b018c57be4cb
https://hg.mozilla.org/integration/fx-team/rev/7ccab6cb126f

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b018c57be4cb
https://hg.mozilla.org/mozilla-central/rev/7ccab6cb126f