720311 - nsFormHistory.js opens formhistory.sqlite on shutdown in order to close it

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Matthew N. [:MattN]]

If form history is not used and therefore DbConnection is not accessed, the formhistory.sqlite will not be opened.  The bug is that _dbClose() accesses this.dbConnection which ends up opening the database if it wasn't already.

Tail of console output at shutdown if form history was not used (browser.formfill.debug = true):
*** LOG addons.xpi: Calling bootstrap method shutdown on jid0-NgMDcEu2B88AbzZ6ulHodW9sJzA@jetpack version 1.3.5
PwMgr mozStorage: Closing the DB connection.
FormHistory: Opening database at /Users/foo/Library/Application Support/Firefox/Profiles/foobar.default/formhistory.sqlite
FormHistory: Open Database
FormHistory: Initializing Database
NOTE: child process received `Goodbye', closing down
NOTE: child process received `Goodbye', closing down
*** LOG addons.xpi: shutdown

The Async form history replacement in bug 566746 doesn't seem to have this problem and so this bug can probably be resolved once that lands.  

This could be considered a regression from bug 696486 since it started closing the DB connection and caused the connection to be opened on profile-before-change if it wasn't already open.

Comment 1

[Marco Bonardo [:mak]]

yeah the check there should ensure that, I commented about using prototype in https://bugzilla.mozilla.org/show_bug.cgi?id=696486#c7 but likely not loudly enough.

if (this.dbConnection !== undefined) should be changed to check FormHistory.prototype.dbConnection

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Check if dbConnection is a getter

(In reply to Marco Bonardo [:mak] from comment #1)
> if (this.dbConnection !== undefined) should be changed to check
> FormHistory.prototype.dbConnection

AFAICT, that won't help since the getter is on FormHistory.prototype.  Maybe I'm missing something?

My approach checks whether dbConnection is a getter still.  I also considered just adding a new boolean property to indicate whether the DB was opened.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 677244 [details] [diff] [review]
v.1 Check if dbConnection is a getter

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +940,5 @@
>          }
>          this.dbStmts = {};
> +
> +        // If dbConnection is still a getter then it hasn't been initialized.
> +        if ("get" in Object.getOwnPropertyDescriptor(FormHistory.prototype, "dbConnection"))

The more common way in our codebase to do the getter check is:
Object.getOwnPropertyDescriptor(FormHistory.prototype, "dbConnection").value === undefined

That said, the fix looks fine.

Comment 4

[Matthew N. [:MattN]]

Thanks Marco. I changed the code to match your suggestion.

https://hg.mozilla.org/integration/fx-team/rev/f33e99e56d52

Comment 5

[Matthew N. [:MattN]]

Backed out for xpcshell failures. https://tbpl.mozilla.org/?tree=Fx-Team&rev=f33e99e56d52

https://hg.mozilla.org/integration/fx-team/rev/d61718babf20

Comment 6

[Matthew N. [:MattN]]

I added a null check for the cleanup case that the corrupt DB test hit.

Updated try results: https://tbpl.mozilla.org/?tree=Try&rev=6950ae784221

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e7519b835d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87e7519b835d