1952094 - Stop using detached nsIMsgDBHdr objects

Status: ASSIGNED

(MailNews Core :: Database, task)

Description

[Ben Campbell]

Mork supports the concept of detached rows, so you can create a row, populate it with data and then add it to the database.
This is used when adding new message entries to the DB: a detached nsIMsgDBHdr object is created, populated with data, then attached to the message table in the database.
This is rather a pain for a msgDB implemented on top of sqlite (i.e. a globaldb).

So it'd be much better to just have a function that lets us pass in the raw data we want for the message, and return a new, already-populated nsIMsgDBHdr object. This'd still work fine for the existing database, and would make the globaldb much easier.

Comment 1

[Ben Campbell]

Attachment: Bug 1952094 - Add nsIMsgDatabase.addMsgHdr(). r=#thunderbird-back-end-reviewers

This patch adds a raw data struct to let the caller create a message without
using a detached nsIMsgDBHdr.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6b9365a770df
Add nsIMsgDatabase.addMsgHdr(). r=mkmelin

Comment 3

[Ben Campbell]

Turns out that filtering is often applied to detached nsIMsgDBHdr objects.
e.g. For incoming POP3 (and IMAP?) messages: the incoming message is parsed, matched against filters and then added to the database for whichever folder it's been moved to. There's also the issue of the message data being already written into the local message store for the original folder, so a move operation needs to roll that back - this is the core issue in Bug 1936988.

Comment 4

[Magnus Melin [:mkmelin]]

We might want to stop doing that... This kind of pre-addition filtering will always be special and thus prone to bugs.

Comment 5

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #4)

We might want to stop doing that... This kind of pre-addition filtering will always be special and thus prone to bugs.

It could be made to work, but I'd want that made explicit and properly documented - not ad-hoc implied mork-specific weirdness like we currently have :-)
The filter matching is reasonably self contained (if a bit clunky) at the moment, but the filter actions (move, delete copy etc) are all over the place - implemented in different ways by different protocols.

Geoff and I had the insight the other week that the filter matching logic could (should?) be shared with the general message filtering used elsewhere (search, virtual folders etc etc etc). At least for globalDB stuff...

In any case, filtering needs a good rethink.

Comment 6

[Ben Campbell]

Attachment: Bug 1952094 - Add AddMessageBatch() path which avoids detached nsIMsgDBHdrs. r=#thunderbird-back-end-reviewers

This adds an implementation of nsIMsgLocalMailFolder.addMessageBatch() which doesn't require detached nsIMsgDBHdr objects.
Currently only used for the MOZ_PANORAMA code, due to filtering requirements in RSS folders.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/77392d52de80
Add AddMessageBatch() path which avoids detached nsIMsgDBHdrs. r=darktrojan

Comment 8

[Ben Campbell]

Attachment: Bug 1952094 - Fix AddMessageBatch() on Panorama to set .messageSize. r=#thunderbird-back-end-reviewers

Comment 9

[Ben Campbell]

Actually, it's not just filtering.
Detached nsIMsgHdrs are used all over the place. For example, nsParseMailMessageState creates and populates a new (detached) nsIMsgDBHdr, then relies on the calling code to use nsIMsgDatabase.addNewHdrToDB() to actually add it to the database.
It wouldn't be too hard to change over, except that nsParseMailMessageState use tends to be spread out amongst multiple callbacks (eg for message copying), so they all need to be tracked down and modified. Sigh.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9d8a04bd1ed7
Fix AddMessageBatch() on Panorama to set .messageSize. r=darktrojan

Comment 11

[Ben Campbell]

Attachment: Bug 1952094 - Add nsIMsgDatabase.attachHdr() to allow separation of detached and live nsIMsgHdr objects. r=#thunderbird-back-end-reviewers

Comment 12

[Ben Campbell]

Attachment: Bug 1952094 - For localfolder copy, use msgKey from final header in nsIMsgCopyServiceListener.setMessageKey() invocations. r=#thunderbird-back-end-reviewers

The local folder copystate stores the nsMsgKey of the new msgHdr at time of
creation, and later sends that key out to the listener. This is fine for the
legacy (mork) code, where the key is assigned before the msgHdr is added to
the database and doesn't change.
But it's no good for panorama, which assigns the key later, when the row is
created in the database. This patch ensures the final key value - for either
database system - is sent out via nsIMsgCopyServiceListener.setMessageKey().

Comment 13

[Ben Campbell]

Attachment: Bug 1952094 - Convert localfolder to use nsIMsgDatabase.attachHdr(). r=#thunderbird-back-end-reviewers

Comment 14

[Ben Campbell]

Attachment: Bug 1952094 - Convert nsParseNewMailState to use nsIMsgDatabase.attachHdr(). r=#thunderbird-back-end-reviewers

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5fb3d455becc
Add nsIMsgDatabase.attachHdr() to allow separation of detached and live nsIMsgHdr objects. r=darktrojan,mkmelin
https://hg.mozilla.org/comm-central/rev/3f050062410d
For localfolder copy, use msgKey from final header in nsIMsgCopyServiceListener.setMessageKey() invocations. r=darktrojan
https://hg.mozilla.org/comm-central/rev/df0f3876e084
Convert localfolder to use nsIMsgDatabase.attachHdr(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/3ebf5d679e88
Convert nsParseNewMailState to use nsIMsgDatabase.attachHdr(). r=darktrojan

Comment 16

[Martin Giger [:freaktechnik]]

Backed out this latest stack because just about all windows tests failed with application crashed [@ nsMsgDatabase::AttachHdr]. Random example log (from a debug build): https://treeherder.mozilla.org/logviewer?job_id=511542418&repo=comm-central&lineNumber=3449

Comment 17

[Ben Campbell]

Doh!
How odd... there's no platform-specific code there. It'll be something stupidly simple. Investigating.

Comment 18

[Ben Campbell]

The breakage was because in AttachHdr(), using the same nsCOMPtr<> for both input and output causing the input to appear as null on windows.
i.e.:

nsCOMPtr<nsIMsgDBHdr> hdr = GetMSgHdrFromSomeWhere();
rv = db->AttachHdr(hdr, false, getterAddRefs(hdr));

hdr would be null, Inside AttachHdr(), but only on windows.
Still no idea why this only happens on windows, but it's a bit moot - it's a bad pattern anyway, as AttachHdr() should be viewed as taking a detached msghdr object and returning a new "live" msghdr, even if those two objects are the same in the legacy (mork) code.
I've updated the patches to use different pointer objects (adding a temp nsCOMPtr<> if needed) and it all works fine on windows now.

try run including those patches here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=cddaca39165d0f87848f843632720da556a247aa

Comment 19

[Ben Campbell]

Another try run, with just this bugs patches:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=153e1d4d6682456cc3caffce33109f8445859bc6

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bb42e1ea85d7
Add nsIMsgDatabase.attachHdr() to allow separation of detached and live nsIMsgHdr objects. r=mkmelin,darktrojan
https://hg.mozilla.org/comm-central/rev/a1432784925b
For localfolder copy, use msgKey from final header in nsIMsgCopyServiceListener.setMessageKey() invocations. r=darktrojan
https://hg.mozilla.org/comm-central/rev/8c5d682fd52d
Convert localfolder to use nsIMsgDatabase.attachHdr(). r=thunderbird-back-end-reviewers,darktrojan
https://hg.mozilla.org/comm-central/rev/cd58840d7d70
Convert nsParseNewMailState to use nsIMsgDatabase.attachHdr(). r=darktrojan