Comment on attachment 568687 [details] [diff] [review] close connections and misc cleanups in toolkit/components/satchel I'm not a good reviewer here. I'm not a peer of this code, and I know very little about SQLite ...

Comment on attachment 568687 [details] [diff] [review] close connections and misc cleanups in toolkit/components/satchel Review of attachment 568687 [details] [diff] [review]: ----------------------------------------------------------------- ok, I can probably review this :) we are almost there! ::: toolkit/components/satchel/nsFormAutoComplete.js @@ +137,2 @@ > self._dbStmts = null; > self.__formHistory = null; please, please, please, could you also remove the prefBranch observer added here :( 76 this._prefBranch.addObserver("", this.observer, false); ::: toolkit/components/satchel/nsFormHistory.js @@ +142,5 @@ > + Services.obs.addObserver(function pbc() { > + self.dbClose(); > + Services.obs.removeObserver(pbc, "profile-before-change"); > + }, "profile-before-change", false); > + // FIXME: we should removeObserver these somewhere. please add followup bug numbers to fixme comments, I briefly discussed with dolske and we both agree we would like to have them. Reason is that we have a bunch of XXX and TODO comments in codebase without filed bugs, and they are often ignored. So like // FIXME (bug 696410): we should... But ideally I hope we can get bug 696410 landed at the same time. Also, I usual prefer removing the observer before and doing actual work later (so if you have to add more work in future they don't get mixed up and you can preserve blame) @@ +845,5 @@ > + * dbClose > + * > + * Finalize the statements. > + */ > + dbClose : function () { methods intended for internal use should be prefixed with _, so _dbClose I know other methods in this component are lacking names, but more mistakes don't make a right, so this should have one. _dbClose : function FH__dbClose() seems fine (using the FH_ prefix, for formHistory) @@ +857,3 @@ > > /* > * dbCleanup In this dbCleanup method I've found another instance of: this.dbStmts = []; please fix it, this is an object, not an array. @@ +877,3 @@ > > // Close the connection, ignore 'already closed' error > + try { this.dbConnection.close(); } catch(e) {} So, I don't see why this is not inside the dbClose() method, can we just move it there? since right now you are not closing the connection on profile-before-change, and the dbClose name is misleading. I think we should also reportError the exception here. IF we fail this removal a reinitialization may fail and the user (And us) would be left unaware of any kind of issue.

Comment on attachment 568770 [details] [diff] [review] close connections Review of attachment 568770 [details] [diff] [review]: ----------------------------------------------------------------- r+ with these fixed, especially the wrong call to dbClose that should be to _dbFinalize. I hope you'll fix the followup bugs too in next days, those should be trivial and I'd be happy to review them. ::: toolkit/components/satchel/nsFormAutoComplete.js @@ +131,5 @@ > } > + } else if (topic == "profile-before-change") { > + // FIXME (bug 696478): > + // also removeObserver on the observer we added in > + // this._prefBranch.addObserver("", this.observer, false); simplify comment here, the description is in the bug. Just // FIXME (bug 696478): also remove the prefBranch observer. ::: toolkit/components/satchel/nsFormHistory.js @@ +142,5 @@ > + Services.obs.addObserver(function pbc() { > + Services.obs.removeObserver(pbc, "profile-before-change"); > + self.dbClose(); > + }, "profile-before-change", false); > + // FIXME (bug 696480): we should removeObserver these somewhere. hm, ok in bug 696480 we'll have to switch to use the observe() method, though, I'd have preferred to do that here... btw, add a note to that bug please. Unfortunately your call to dbClose won't work, since you renamed the method to dbFinalize(). @@ +845,5 @@ > + * dbClose > + * > + * Finalize the statements. > + */ > + _dbFinalize : function FH__dbFinalize() { the comment has the wrong label now... @@ +847,5 @@ > + * Finalize the statements. > + */ > + _dbFinalize : function FH__dbFinalize() { > + // FIXME (bug 696486): close the connection in here. > + // Finalize all statements to free memory, avoid errors later "// Finalize all statements to allow closing the connection correctly." "avoid errors" is a bit generic concept...

PS: note that without bug 696486 you are actually not closing the connection on shutdown, so at least that bug is immportant for the work you are doing.

(In reply to Marco Bonardo [:mak] from comment #5) > PS: note that without bug 696486 you are actually not closing the connection > on shutdown, so at least that bug is immportant for the work you are doing. I know I am not. As I mentioned many times since I started working on this, I am trying to split the patches because starting closing the connections found many problem, statements not being finalized in particular. Being able to fix and test one thing at a time is a *big* improvement.

I will commit it once try is done https://tbpl.mozilla.org/?tree=Try&rev=81a9bf4155a8

ok, we are going to wait here to figure out the live profile switching issue pointed out by honza. If we can switch profiles on the fly this will require some small adjustement.

Comment on attachment 568788 [details] [diff] [review] close connections resettung review flag on me, to take into account honza comments, should not be lot of work.

Comment on attachment 568788 [details] [diff] [review] close connections Review of attachment 568788 [details] [diff] [review]: ----------------------------------------------------------------- If you don't feel like you want to work on this js code, just ping me I can take these patches, but since you're at a step from completing them I'd really not want to steal you the glory ::: toolkit/components/satchel/nsFormAutoComplete.js @@ +88,2 @@ > > + Services.obs.addObserver(this.observer, "profile-before-change", false); ok, let's make this weak owner, by flipping false to true. @@ +129,5 @@ > default: > self.log("Oops! Pref not handled, change ignored."); > } > + } else if (topic == "profile-before-change") { > + // FIXME (bug 696478): also remove the prefBranch observer. either move this fixme comments up where we add the prefBranch observer, or just flip the third argument to true and close that followup bug. This is the related code: this._prefBranch.addObserver("", this.observer, false); @@ +130,5 @@ > self.log("Oops! Pref not handled, change ignored."); > } > + } else if (topic == "profile-before-change") { > + // FIXME (bug 696478): also remove the prefBranch observer. > + Services.obs.removeObserver(self.observer, "profile-before-change"); no more needed @@ +138,1 @@ > self._dbStmts = null; should be set to {}, not to null ::: toolkit/components/satchel/nsFormHistory.js @@ +147,2 @@ > Services.obs.addObserver(function() { self.expireOldEntries() }, "idle-daily", false); > Services.obs.addObserver(function() { self.expireOldEntries() }, "formhistory-expire-now", false); So here is a (I hope clear) explanation of what we want here, please don't kep spawning followups, I'm starting getting lost: in FormHistory.prototype.QueryInterface add nsISupportsWeakRerefence to the interfaces array flip all false to true in the addObserver calls here. replace all three "function() something" with "this". in FormHistory.prototype.observe replace the if/else with a switch (topic), should be: switch (topic) { case "nsPref:changed": this.updatePrefs(); break; case "profile-before-change": this._dbFinalize(); break; case "idle-daily": this.expireOldEntries(); break; case "formhistory-expire-now": this.expireOldEntries(); break; default: this.log("Oops! Unexpected notification: " + topic); break; }

Comment on attachment 573797 [details] [diff] [review] Finalize statements in profile-before-change Review of attachment 573797 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry but I really don't know this code enough even for giving a feedback.

Comment on attachment 573797 [details] [diff] [review] Finalize statements in profile-before-change Review of attachment 573797 [details] [diff] [review]: ----------------------------------------------------------------- it looks good, just check if try is happy. ::: toolkit/components/satchel/nsFormHistory.js @@ +872,5 @@ > + * Finalize the statements. > + */ > + _dbFinalize : function FH__dbFinalize() { > + // FIXME (bug 696486): close the connection in here. > + // Finalize all statements to allow closing the connection correctly. the second comment line should replace the comment you have in the javadoc, it's not useful here.

I am away until Saturday. Could you please check this in?

In my queue with a few other bits that are being sent to try first and then onto inbound :-) https://tbpl.mozilla.org/?tree=Try&rev=eb96679e6888

Bug 687511 included in that try run failed check-sync-dirs, so had to push again: https://tbpl.mozilla.org/?tree=Try&rev=329bc2d383a3 (this changeset is there, is just hidden due to the previous push to try)