696486 - Close dbConnection.

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

in nsFormHistory.js we have a dbFinalize method. It should also close the connection and be called dbClose.

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I did a try push with the current bits to
https://tbpl.mozilla.org/?tree=Try&rev=a4fdcea5e6c1

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

A new push of the wip bits is:
https://tbpl.mozilla.org/?tree=Try&rev=d68a005f0502

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I updated the wip patch with the changes in 702848, removed the strange try..catch and kept the sync close.

https://tbpl.mozilla.org/?tree=Try&rev=93990e770e20

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

rebased and pushed again
https://tbpl.mozilla.org/?tree=Try&rev=a7c4a43a8e1a

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch on top of bug 702848

This bug depends on 702848, but maybe we can do part of the review in parallel.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: _dbFinalize should be _dbClose

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

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 581316 [details] [diff] [review]
_dbFinalize should be _dbClose

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +367,4 @@
>      get dbConnection() {
>          // Make sure dbConnection can't be called from now to prevent infinite loops.
>          delete FormHistory.prototype.dbConnection;
> +        FormHistory.prototype.dbConnection = null;

why do we need to do this? I think there's a better way to detect when a property does not exist than assigning null to it.

Btw, this makes the connection not restartable, but the issue comes from the other bug... So I suppose I'll ignore it...

@@ +880,2 @@
>          for each (let callback in this.closeCallbacks) {
>              callback.complete();

closeCallbacks is an array, you should not use for each on it, since for each is intended for objects.

Rather than walking it and then freeing it later, would be better to
while (this.closeCallbacks.length > 0) {
  (this.closeCallbacks.shift()).complete();
}
and I assume complete() may throw, so should be in a try catch to not interrupt the shutdown process.

@@ +885,4 @@
>              stmt.finalize();
>          }
>          this.dbStmts = {};
> +        this.closeCallbacks = [];

I think would be better to do something different

@@ +904,4 @@
>          let backupFile = this.dbFile.leafName + ".corrupt";
>          storage.backupDatabaseFile(this.dbFile, backupFile);
>  
> +        if (this.dbConnection !== null)

"dbConnection" in FormHistory.prototype maybe?

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: _dbFinalize should be _dbClose

I changed the check for "has this connection been created" to use undefined and changed the loop that calls complete.

What should we do with the exception we catch?

https://tbpl.mozilla.org/?tree=Try&rev=71ba80a3ba44

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: _dbFinalize should be _dbClose

https://tbpl.mozilla.org/?tree=Try&rev=5aff2f24870e

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 581643 [details] [diff] [review]
_dbFinalize should be _dbClose

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

I don't really want to add another interface & more code here. In fact, I want to go in the other direction & eventually stop exposing DBConnection entirely. If we end up removing DBConnection, then we also will end up removing the new stuff you're adding

Adding a new observer topic which would get notified right before we close the connection seems like it would result in less churn (no new APIs and it still works!).

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 581643 [details] [diff] [review]
_dbFinalize should be _dbClose

I'm clearing the request since I can't do anything here till bug 702848 has a decision, so I don't need to track this request for now.  Please re-request review (you may even request to dolske or zpao) once that bug landed.

Comment 12

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Paul O'Shannessy [:zpao] from comment #10)
> Comment on attachment 581643 [details] [diff] [review]
> _dbFinalize should be _dbClose
> 
> Review of attachment 581643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really want to add another interface & more code here. In fact, I
> want to go in the other direction & eventually stop exposing DBConnection
> entirely. If we end up removing DBConnection, then we also will end up
> removing the new stuff you're adding

Note that was the direction I went first (https://bug702848.bugzilla.mozilla.org/attachment.cgi?id=574764). In fact I even reported bug 703400 to track removing DBConnection, but Dolske objected to that direction.

At this point I am happy with any solution that lets me close the connection :-(

> Adding a new observer topic which would get notified right before we close
> the connection seems like it would result in less churn (no new APIs and it
> still works!).

Even this one. Would this be a profile message? How about profile-before-change-prepare or profile-finish-statements?

Comment 13

[Marco Bonardo [:mak]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #12)
> Even this one. Would this be a profile message? How about
> profile-before-change-prepare or profile-finish-statements?

fwiw, we already have profile-change-teardown that is fired before profile-before-change

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Close dbConnection.

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

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 583863 [details] [diff] [review]
Close dbConnection.

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +402,5 @@
>          case "formhistory-expire-now":
>              this.expireOldEntries();
>              break;
>          case "profile-before-change":
> +            this.dbConnection.close();

I don't understand why we finalize statements in change-teardown and close connection in before-change... we should move the database close code from _dbCleanup into _dbFinalize, and in profile-before-change just invoke _dbFinalize. And stop listening to profile-change-teardown

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> I don't understand why we finalize statements in change-teardown and close
> connection in before-change... we should move the database close code from
> _dbCleanup into _dbFinalize, and in profile-before-change just invoke
> _dbFinalize. And stop listening to profile-change-teardown

Because that was the only solution that dolske accepted in 702848. If you want to discuss something different with him, please do, but lets not block this bug on it.

Comment 17

[Marco Bonardo [:mak]]

hm wait, maybe I missed what happened in the dependencies, what's the reason bug 702848 moved finalization to profile-change-teardown?

Comment 18

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Marco Bonardo [:mak] from comment #17)
> hm wait, maybe I missed what happened in the dependencies, what's the reason
> bug 702848 moved finalization to profile-change-teardown?

Yes. Everyone that uses this database should finalize statements in profile-change-teardown.

Comment 19

[Marco Bonardo [:mak]]

Hm wait, I see a reason for form.js to use profile-change-teardown, and that's because it can't preduct if it will run before of after formHistory closes the connection in profile-before-change.
But there is no reason for formHistory to do so since it will finalize and close with predictable ordering, so I think in formHistory my suggestion in comment 15 is the simplest and won't regress anything.

Comment 20

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Close dbConnection.

I guess we don't need to consistent

https://tbpl.mozilla.org/?tree=Try&rev=9ef0441c367e

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 584558 [details] [diff] [review]
Close dbConnection.

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

a couple comments, apart those looks ok.

::: toolkit/components/satchel/nsFormHistory.js
@@ +869,2 @@
>       *
>       * Finalize all statements to allow closing the connection correctly.

This comment needs a minor update to tell this also actually closes the connection

@@ +873,5 @@
>          for each (let stmt in this.dbStmts) {
>              stmt.finalize();
>          }
>          this.dbStmts = {};
> +        this.dbConnection.close();

dbConnection is still sort of memoized , so I'd move here all the following code block, that is currently in dbCleanup. Then in dbCleanup just call this._dbClose().

         if (this.dbConnection !== undefined) {
             try {
                 this.dbConnection.close();

             } catch (e) {
                 Components.utils.reportError(e);
             }
         }

Comment 22

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3aeba1dee836

Comment 23

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/3aeba1dee836