792915 - Remove mDatabase from folder object

Status: NEW

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

After spending the last week tracing out folder compact bugs whose root causes were management of the database cache, I have come to the conclusion that we need to revise the way that we keep databases open.

In particular, the database reference in the folder is causing more harm than good. We see core code that routinely resets that to prevent unneeded open databases, and the same is recommended to extensions. We also get crashes when we check for null mDatabase in folder code, then later in the same method crash with null database because some unexpected intervening code nulled it. The concept of letting the folder hold the main db reference is clearly being circumvented routinely, and it is time to just abandon that.

Instead, all references to the database in the folder code should use a local nsCOMPtr to get it. Eliminate mDatabase (at least as a strong reference). Let other code (like msgDBCacheManager.js) and the cache manage the lifetime of database objects.

Comment 1

[Jim Porter (:squib)]

Does this imply removing nsIMsgFolder.msgDatabase? Because I would be very sad if that went away (I use it for Mail Summaries).

Comment 2

[Kent James (:rkent)]

I am proposing to make it read only.

Comment 3

[Jim Porter (:squib)]

Oh, that's ok then.

Comment 4

[Federico]

Should this be considered a blocker for bug 794401?

Comment 5

[Kent James (:rkent)]

(In reply to Federico from comment #4)
> Should this be considered a blocker for bug 794401?

No, I would not do that. If that bug is a db opening problem, then the real blocker is to implement async database opening, which has some initial work done but is not hooked up yet. The effect of this bug on performance would be small compared to that improvement.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #0)
> ...
> In particular, the database reference in the folder is causing more harm
> than good. We see core code that routinely resets that to prevent unneeded
> open databases, and the same is recommended to extensions. We also get
> crashes when we check for null mDatabase in folder code, then later in the
> same method crash with null database because some unexpected intervening
> code nulled it. The concept of letting the folder hold the main db reference
> is clearly being circumvented routinely, and it is time to just abandon that.

So this would improve our stability!?  :)
Is this a big job, smallish, or in between?
Are there any currently active bugs this would help?

Comment 7

[Kent James (:rkent)]

It's not a huge job, but it could be risky, and some extensions probably try to manage the lifetime of the database directly by setting the value, and those would break. It's always hard to know what will or will not increase stability, but the management of database lifetime is a real mess now, so cleaning it up is one of those technical debt issues that we need to address.

At this point in time I would probably rather spend time trying to make the database replaceable by a non-mork representation. That will require quite a bit of rework itself.