1747173 - IMAP message flag changes not be seen when changed by another client after inactivity timeout

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[gene smith]

This a regression fix for the changes in bug 1708981 which I think is still in beta at this time and not in 91.

STR:

Start instance 1 of tb
Setup instance 1 to NOT check for email every X minutes.
Setup instance 1 to use imap IDLE (immediate)
Open a folder X and let TB just sit doing nothing for a long enough time for the server to disconnect. Typically this is about 30 minutes.

Start a new tb instance 2 for the same account and open the same folder X.
On instance 2 set a flag on any message in folder X, e.g., read/unread, star etc.
Note that the flag change is seen for the message in instance 2.
Note that the flag change is NOT seen for the message in instance 1 like it should.

This fails because m_imapMailFolderSinkSelected is only set when the URL driving the activity is "select". A "select" URL does occur when you first click on a folder. But after an inactivity time-out and then if you just change a message flag on a visible message, the first URL on the new connection is "addmsgflag" so m_imapMailFolderSinkSelected remains nullptr so the imap idle response is never handled.

The fix is to move the setting of m_imapMailFolderSinkSelected into selectMailbox() where the imap SELECT is actually sent. This works because m_imapMailFolderSinkSelected is only used in the imap selected state and imap SELECT is triggered by other URLs too, like addmsgflag, not just the select URL.

Comment 1

[gene smith]

Attachment: see-flag-changes-after-timeout.patch

I've been running with this fix, along with other in-progress changes, for over a month now. But I haven't run just this change through try server yet.

Comment 2

[Ben Campbell]

Comment on attachment 9256477 [details] [diff] [review]
see-flag-changes-after-timeout.patch

Review of attachment 9256477 [details] [diff] [review]:
-----------------------------------------------------------------

So very satisfying seeing `SetupSinkProxy()` being simplified like that!
Looks pretty good to me - just a question about nullness.

I kicked off a try build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8ad15e809eb8399c8c3d3cb582da3420dff04c04

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +3445,5 @@
>  
> +  // Save the folder sink obtained in SetupSinkProxy() for whatever URL just
> +  // caused this SELECT. Needed so idle and noop responses are using the correct
> +  // folder when detecting changed flags or new messages.
> +  m_imapMailFolderSinkSelected = nullptr;

Don't need this nullptr line - just assigning a new value will be fine.

@@ +3447,5 @@
> +  // caused this SELECT. Needed so idle and noop responses are using the correct
> +  // folder when detecting changed flags or new messages.
> +  m_imapMailFolderSinkSelected = nullptr;
> +  m_imapMailFolderSinkSelected = m_imapMailFolderSink;
> +  MOZ_ASSERT(m_imapMailFolderSinkSelected);

Are there cases where `m_imapMailFolderSink` might be null here? Genuine question - I have no idea what the expectations are or should be :-)
`SetupSinkProxy()` uses the sink supplied by the running URL. Presumably, because we're in `SelectMailbox()` here, the m_RunningURL object always has a folder associated with it, so we'd never find a null sink here.
But the URL holds the sink as a weak reference, so maybe there's a possibility for the sink to disappear before `SetupSinkProxy()` asks for it?

(To be clear, I _love_ asserts - they clarify expectations and assumptions, and that's great. But not if there's any reasonable case for seeing a nullptr sink here)

Comment 3

[gene smith]

• m_imapMailFolderSinkSelected = nullptr;

Don't need this nullptr line - just assigning a new value will be fine.

I thought it might be needed to free memory pointed to by the old value, but I wasn't sure. I was hoping you could confirm or deny this. So you are saying that just assigning a new value is sufficient and no need to null it first so I can remove this line.

I do see the "sink" pointers set to nullptr when the thread exits or dies so I guess it's only needed then to free the memory.

Not sure about the MOZ_ASSERT. I haven't seen it fail but would "feel" better leaving it in if OK by you. It should only affect debug builds, I think.

I see some failures in the "try" build but it looks like other builds also failing the same tests.
Thanks!

Comment 4

[Wayne Mery (:wsmwk)]

Gene, what's next?

Comment 5

[gene smith]

Attachment: 1747173-fix-flag-changes-after-timeout.patch

Sorry for the delay on this.
This just removes the not needed m_imapMailFolderSinkSelected = nullptr; line as pointed out by reviewer Benc. I've been running with this patch since late Dec and haven't seen any problems or asserts so I'll go ahead and set the keyword to request landing since the original review was +.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1072cb94b04e
IMAP flag changes not seen when changed by another client after inactivity timeout. r=benc

Comment 7

[Rob Lemley [:rjl]]

Comment on attachment 9276966 [details] [diff] [review]
1747173-fix-flag-changes-after-timeout.patch

[Triage Comment]
Approved for 101.0beta per wsmwk via Matrix

Comment 8

[Rob Lemley [:rjl]]

Thunderbird 101.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/7005a222ab2c