Closed Bug 720311 Opened 10 years ago Closed 9 years ago

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

Categories

(Toolkit :: Form Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Keywords: regression, Whiteboard: [snappy])

Attachments

(1 file)

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.
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
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Assignee: mnoorenberghe+bmo → nobody
Status: ASSIGNED → NEW
(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.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Attachment #677244 - Flags: review?(mak77)
Summary: nsFormHistory.js opens formhistory.sqlite in order to close it on shutdown → nsFormHistory.js opens formhistory.sqlite on shutdown in order to close it
Whiteboard: [snappy]
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.
Attachment #677244 - Flags: review?(mak77) → review+
Thanks Marco. I changed the code to match your suggestion.

https://hg.mozilla.org/integration/fx-team/rev/f33e99e56d52
Whiteboard: [snappy] → [snappy][fixed-in-fx-team]
Backed out for xpcshell failures. https://tbpl.mozilla.org/?tree=Fx-Team&rev=f33e99e56d52

https://hg.mozilla.org/integration/fx-team/rev/d61718babf20
Whiteboard: [snappy][fixed-in-fx-team] → [snappy]
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
https://hg.mozilla.org/mozilla-central/rev/87e7519b835d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.