533530 - junk training data not saved if plugin leaks, need a quit observer

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Kent James (:rkent)]

While doing some testing for my extensions, I noticed that training data was not getting saved on shutdown. Data is supposed to be saved in the destructor for nsBayesianFilter, but that destructor was not getting called (presumably because of some sort of shutdown leak) which I confirmed with printf statements in the destructor.

While the shutdown leak seems to be caused by my extensions, still saving of training data is too important to rely on no leaks. We should add a quit observer to make sure that the data gets saved.

Comment 1

[David :Bienvenu]

I don't know if the issue is that the destructor isn't called. Even if it is called, it's too late to do things like NS_GetSpecialDirectory so training data won't get saved. I'm going to fix this, because I'm trying to fix the memory leaks from the xpcshell tests and test_junkingWhenDisabled leaks a lot of stuff.

Comment 2

[David :Bienvenu]

Attachment: proposed fix

I need to add an Init method because you can't add observers in the constructor, because there are no references to the service yet, and it gets deleted out from under me when I add an observer. Init also has the nice advantage that you can actually return errors, though I haven't taken advantage of that.

Comment 3

[David :Bienvenu]

Comment on attachment 546949 [details] [diff] [review]
proposed fix

clearing review request because I think this patch exposes issues w/ other tests

Comment 4

[David :Bienvenu]

Attachment: minimal fix

minimal fix - I tried moving more stuff into Init() but got into a world of hurt that I don't feel like revisiting...

Comment 5

[Kent James (:rkent)]

Comment on attachment 547109 [details] [diff] [review]
minimal fix

The patch as listed does not work:

+NS_GENERIC_FACTORY_CONSTRUCTOR(nsBayesianFilter, Init)

should be

+NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsBayesianFilter, Init)

With that change, I tested this with printf's and with and without artificial leaks, and it seems to work.

r=me with that error fixed.

Comment 6

[David :Bienvenu]

thx for catching that! That would explain the weirdness I saw. Though I would have expected even more...

http://hg.mozilla.org/comm-central/rev/1bf028c0289b