Bugzilla

Alex: nice work in comment #33 !

(In reply to Magnus Melin [:mkmelin] from comment #34)
> (In reply to Alexander Bergmann from comment #33)
>  Ben, what do you want to do ^^^

It kind of reinforces my opinion that the old code was damn lucky to have worked at all...

The only thing patch 8c44f53caece8bfd13295337d23deba36dbc92ca does is to return an *obviously* wrong value when an unset .messageOffset is read, rather than a *subtly* wrong value (it returns the message key rather than offset, which is never going to be valid, but is way more likely to still be within the extent of the mbox file).
Any code that relies on that behaviour is going to cause trouble at some point, no matter what.

So. Practical steps forward in the short term:

- Back out 8c44f53caece8bfd13295337d23deba36dbc92ca
- Back out D157071
- Back out D156269 (but maybe not - see below)
- Separate out the unit test in https://phabricator.services.mozilla.com/D157071 into it's own patch.
- Extend that unit test to ensure the message data stays intact, especially when the message key starts getting higher (I'm concerned that the the borked messageOffset might be causing the start of the message to be clipped off).
- Write up a bug for either implementing all this stuff properly and robustly, or just ensure IMAP folders are treated as read-only when in offline mode.


But first... Alex:
Does it work if you back out 8c44f53caece8bfd13295337d23deba36dbc92ca and
https://phabricator.services.mozilla.com/D157071?

but keep in https://phabricator.services.mozilla.com/D156269 ?

(D156269 includes some long-overdue cleanup which will probably have to happen at sometime anyway)




https://phabricator.services.mozilla.com/D156269

Alex: nice work in comment #33 !

(In reply to Magnus Melin [:mkmelin] from comment #34)
> (In reply to Alexander Bergmann from comment #33)
>  Ben, what do you want to do ^^^

It kind of reinforces my opinion that the old code was damn lucky to have worked at all...

The only thing patch 8c44f53caece8bfd13295337d23deba36dbc92ca does is to return an *obviously* wrong value when an unset .messageOffset is read, rather than a *subtly* wrong value (it returns the message key rather than offset, which is never going to be valid, but is way more likely to still be within the extent of the mbox file).
Any code that relies on that behaviour is going to cause trouble at some point, no matter what.

So. Practical steps forward in the short term:

- Back out 8c44f53caece8bfd13295337d23deba36dbc92ca
- Back out D157071
- Back out D156269 (but maybe not - see below)
- Separate out the unit test in D157071 into it's own patch.
- Extend that unit test to ensure the message data stays intact, especially when the message key starts getting higher (I'm concerned that the the borked messageOffset might be causing the start of the message to be clipped off).
- Write up a bug for either implementing all this stuff properly and robustly, or just ensure IMAP folders are treated as read-only when in offline mode.


But first... Alex:
Does it work if you back out 8c44f53caece8bfd13295337d23deba36dbc92ca and
D157071?

but keep in D156269 ?

(D156269 includes some long-overdue cleanup which will probably have to happen at sometime anyway)