1720047 - Remove nsIMsgDBHdr.messageOffset

Status: ASSIGNED

(MailNews Core :: Backend, task)

Description

[Ben Campbell]

The message offset is a mailstore-specific detail and only used for mbox storage.
The offset is replaced by the "storeToken" value (for mbox, storeToken just holds the offset).

Searching for .messageOffset/GetMessageOffset()/SetMessageOffset() is a good way to find remaining mbox-centric code.

Comment 1

[Ben Campbell]

Attachment: Bug 1720047 - Remove unused nsIMsgDBHdr.messageOffset access in nsParseNewMailState. r?mkmelin

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/243f03b1ed01
Remove unused nsIMsgDBHdr.messageOffset access in nsParseNewMailState. r=mkmelin

Comment 3

[Ben Campbell]

Huh. Also, the current implementation (nsMsgHdr::GetMessageOffset()) (source) returns the message key if the offset is missing from the DB.
Very curious. I wonder what the original intention there was. It predates the mercurial history.

This is causing havoc over in Bug 1705824 - under certain circumstances it is using message keys as offsets into mbox files. Hilarity ensues :-)

Comment 4

[:aceman]

In the past the key indeed was equal to the offset in the file, but that is no longer true for a few years. That code must be removed.

Comment 5

[Ben Campbell]

(In reply to :aceman from comment #4)

In the past the key indeed was equal to the offset in the file, but that is no longer true for a few years.

Ahh - thanks! That makes sense. I knew there'd be a valid original reason behind it.

That code must be removed.

I plan to remove the whole .messageOffset attribute very soon anyway. It's mbox specific and redundant (it's encoded in the mbox storeToken).
I did think about changing this aspect in the meantime, but there's not really any sensible return value to use if the offset is missing, and there's no error checking. So the best we could do is assert (and preferably crash hard :-)

Comment 6

[Ben Campbell]

Attachment: Bug 1720047 - Remove unused nsImapMailDatabase::UpdateFolderFlag(). r=mkmelin

Filing this under the nsIMsgDBHdr.messageOffset removal bug. It's not directly
related, but it was obscuring another UpdateFolderFlag() function which does
use .messageOffset.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e881ea1bf01b
Remove unused nsImapMailDatabase::UpdateFolderFlag(). r=mkmelin

Comment 8

[Geoff Lankow (:darktrojan)]

Setting the milestone to that of the first patch.

Comment 9

[Ben Campbell]

Attachment: Bug 1720047 - Remove unused nsIMsgDBHdr.messageOffset access in nsMailboxProtocol. r=mkmelin

Comment 10

[Ben Campbell]

Attachment: Bug 1720047 - Show warning message in debug if unset nsMsgHdr.messageOffset is read. r=mkmelin

Previously, unset .messageOffset reads would return the message key, and NS_OK.
This behaviour is no longer useful.
In release builds, unset .messageOffset now comes back as -1, with a rv of NS_ERROR_FAILURE.
Very few callers check the result, so this way we'll hear about it in debug builds at least.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2a60fe381b37
Remove unused nsIMsgDBHdr.messageOffset access in nsMailboxProtocol. r=mkmelin

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3b3ba6e833b0
Show error in debug if unset nsMsgHdr.messageOffset is read. r=mkmelin

Comment 13

[Magnus Melin [:mkmelin]]

This error is now showing on CI: https://treeherder.mozilla.org/logviewer?job_id=358828588&repo=comm-central&lineNumber=5406
Hit MOZ_CRASH([Parent 29215, Main Thread] ###!!! ASSERTION: missing .messageOffset (key=1, storeToken=''):

Comment 14

[Magnus Melin [:mkmelin]]

And on many others: https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=dJepD1cqQlaaRokSWrrMfQ.0&revision=3b3ba6e833b0732f59f5bc11098397d9dff6207e

Comment 15

[Magnus Melin [:mkmelin]]

Backout: https://hg.mozilla.org/comm-central/rev/ce6364ecf82402cfbb51ceeb18675c96f5992021

Comment 16

[Ben Campbell]

This one is infuriating. I've been digging into it.
For reference, my original patch asserted if an unset .messageOffset was read, as there have been a couple of bugs made a lot harder to track down because it currently returns the message key in such cases (which used to make sense, but no longer does).

There are a couple of things in play.

1. A lot of the unit tests rely on being able to read .messageOffset, even if it's unset. For example. Assert.equal() accesses all the attributes in the 'got' and 'expected' objects to produce an error message. It does this regardless of the success of the test. This is reasonable, but annoying, as it doesn't allow us to check for the unset state in code.

2. There are a few places which rely on .messageOffset even if the value returned is useless or disregarded. For example, nsImapMailFolder::SetPendingAttributes() copies over the messageOffset and storeToken during message copies, even though those values have no meaning outside the original mailstore (certainly they don't mean anything for a non-offline message on an IMAP server!).
   I did have a go at removing them... but there are a couple of tests which seem to rely on those values being copied over, even if they are rubbish.
   (see https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=65d9f3236ed777a59320695d60aaae142668264d )

I've wasted enough time on this now, so I'm just going to post a revised version of the patch which reduces the error to a warning, doesn't fail, and returns an unset .messageOffset as 12345678 rather than the message key. The key was too easy to mistake for something plausable - hopefully the new value will raise eyebrows if it's spotted anywhere.
There's a try run with the new patch here, which looks good to me: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f08cf9f304f1baa24bb319e1d5b1b40413e333cf

Hopefully, that'll hold things until I finish this bug and excise .messageOffset competely.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8c44f53caece
Show warning message in debug if unset nsMsgHdr.messageOffset is read. r=mkmelin

Comment 18

[ISHIKAWA, Chiaki]

(Oops. It is not |--serialize| but |--sequential|. I was writing from memory. So I am fixing this comment.)
In Bug 1774952, I have listed tests from C-C TB xpcshell test suite that print the warnings when the test is run using
the FULL DEBUG version of C-C TB LOCALLY with "--sequential--verbose".
Note that unless "--sequential --verbose" is passed to xpcshell-test the warning lines are hidden if the tests are deemed to have succeeded.
I have no idea how to run xpcshell-test on treeherder servers with "--sequential --verbose" and thus cannot point at them easily on the shared server.

Comment 19

[Takanori MATSUURA]

The leave-open is set but has not been updated in years.
Are additional patches planned?

Comment 20

[Ben Campbell]

Yes, definitely.
There are only about 3 xpcshell tests, 1 mochitest and a few places in C++ which still reference .messageOffset.
So it shouldn't be tooooooo hard to finally get rid of it. Just a matter of finding the time to do it.