Closed Bug 696486 Opened 8 years ago Closed 8 years ago

Close dbConnection.

Categories

(Toolkit :: Form Manager, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 5 obsolete files)

in nsFormHistory.js we have a dbFinalize method. It should also close the connection and be called dbClose.
Summary: _dbFinilize should be _dbClose → _dbFinalize should be _dbClose
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
Attached patch patch on top of bug 702848 (obsolete) — Splinter Review
This bug depends on 702848, but maybe we can do part of the review in parallel.
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #581291 - Flags: review?(mak77)
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?
Attachment #581316 - Flags: review?(mak77)
Attached patch _dbFinalize should be _dbClose (obsolete) — Splinter Review
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
Attachment #581316 - Attachment is obsolete: true
Attachment #581458 - Flags: review?(mak77)
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 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.
Attachment #581643 - Flags: review?(mak77)
(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?
(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
Summary: _dbFinalize should be _dbClose → Close dbConnection.
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
Attachment #583863 - Flags: review?(mak77) → review-
> 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.
hm wait, maybe I missed what happened in the dependencies, what's the reason bug 702848 moved finalization to profile-change-teardown?
(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.
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.
I guess we don't need to consistent

https://tbpl.mozilla.org/?tree=Try&rev=9ef0441c367e
Attachment #583863 - Attachment is obsolete: true
Attachment #584558 - Flags: review?(mak77)
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);
             }
         }
Attachment #584558 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/3aeba1dee836
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.