1956408 - ThreadSanitizer: data race /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp:1278:23 in nsImapProtocol::TellThreadToDie(bool)

Status: ASSIGNED

(MailNews Core :: Networking: IMAP, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: ishikawa [at] yk.rim.or.jp
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=500909289&repo=try-comm-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Dxm5VwIXQoeSPZoh-a8A_g/runs/0/artifacts/public/logs/live_backing.log

---

Seems to be reproduced consistently in repeated jobs.
treeherder bug filer has a feature that seems to be a bug to me.: I filed a bugzilla a few minute ago, and somehow "Report this as a security issue" is automatically turned on (!?), and it seems I cannot access the bug I submitted using this "Intermittent Bug Filer" GUI. Grr.

Comment 2

[ISHIKAWA, Chiaki]

main thread :

Write of size 8 at 0x726c00005000 by main thread:
[task 2025-03-30T14:54:12.056Z] 14:54:12     INFO -  PID 16107 |     #0 assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:66:13 (libxul.so+0xb2a4db4) (BuildId: 65253332a173aed13d2a0a05669bdd2e7cfa027b)
[task 2025-03-30T14:54:12.057Z] 14:54:12     INFO -  PID 16107 |     #1 operator= /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:180:5 (libxul.so+0xb2a4db4)
[task 2025-03-30T14:54:12.057Z] 14:54:12     INFO -  PID 16107 |     #2 nsImapProtocol::SetupWithUrl(nsIURI*, nsISupports*) /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp:808:24 (libxul.so+0xb2a4db4)

https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#808
  m_imapMailFolderSink = nullptr;

Thread T23:

[task 2025-03-30T14:54:12.073Z] 14:54:12     INFO -  PID 16107 |   Previous write of size 8 at 0x726c00005000 by thread T23:
[task 2025-03-30T14:54:12.073Z] 14:54:12     INFO -  PID 16107 |     #0 assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:66:13 (libxul.so+0xb2adc3e) (BuildId: 65253332a173aed13d2a0a05669bdd2e7cfa027b)
[task 2025-03-30T14:54:12.074Z] 14:54:12     INFO -  PID 16107 |     #1 assign_with_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:61:5 (libxul.so+0xb2adc3e)
[task 2025-03-30T14:54:12.074Z] 14:54:12     INFO -  PID 16107 |     #2 operator= /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:187:5 (libxul.so+0xb2adc3e)
[task 2025-03-30T14:54:12.074Z] 14:54:12     INFO -  PID 16107 |     #3 nsImapProtocol::ProcessCurrentURL() /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp:2097:26 (libxul.so+0xb2adc3e)

``
https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#2097
m_imapMailFolderSink = imapMailFolderSink;

It seems we need to protect the access to |m_imapMailFolderSink| at line 2097.
We are missing |ReentrantMonitorAutoEnter mon(mMonitor);| at appropriate place before 2097.

Comment 3

[ISHIKAWA, Chiaki]

Hmm, now I am not sure which |...Monitor| variable is used to protect which variable(s).
Is there a documentation about the correspondence?

Comment 4

[ISHIKAWA, Chiaki]

Hmm, adding protection with |ReentrantMonitorAutoEnter mon(mMonitor);| for the access to |m_imapMailFolderSink| on 2097 now triggers
another error for unprotected access to |m_nextUrlReadyToRun| on line 1521.
See:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9545eb4d406f6cec8038567063ad40755859df65
https://treeherder.mozilla.org/logviewer?job_id=501605263&repo=try-comm-central&lineNumber=4745

Somebody familiar with IMAP ought to look into this.

Comment 5

[Magnus Melin [:mkmelin]]

I'm afraid there is no documentation.
Yes, there is likely more than one variable that needs more protection. That another one popped up is not that unexpected. What you want to be careful about is not ending up in deadlock. It's a real mess.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: WIP: Bug 1956408 - Serialize writing/reading of |m_nextUrlReadyToRun|, etc. to fix TSAN race in nsImapProtocol::TellThreadToDie(). r=#thunderbird-reviewers

protect |m_threadShouldDie|, too.
protect the call to TellThreadToDie() in nsImapIncomingServer.cpp via monitor
should we or should we not serialize the access to |m_inThreadShouldDie| in threadShouldDie()?.
We should because |m_threadDeathMonitor| is private.

Comment 7

[ISHIKAWA, Chiaki]

Still debugging.

Somebody who is familliar with the code ought to check the patch,
but I think we are going in the right direction as far as the existing tests are concerned.

I say some careful review is in order because I see this comment in nsImapProtocol.cpp.

  // We're using PR_CEnter/ExitMonitor because Monitors don't like having
  // us to hold one monitor and call code that gets a different monitor. And
  // some of the methods we call here use Monitors.

(In reply to ISHIKAWA, Chiaki from comment #6)

Created attachment 9475661 [details]
Bug 1956408 - Protect writing of |m_nextUrlReadyToRun|, etc. r=#thunderbird-reviewers

protect |m_threadShouldDie|, too.
protect the call to TellThreadToDie() in nsImapIncomingServer.cpp via monitor
No, it was not necessary. I would remove the comments still left there.

should we or should we not serialize the access to |m_inThreadShouldDie| in threadShouldDie()?.
We should because |m_threadDeathMonitor| is private.

Any comments welcome.

We still need to handle the following bug 1956583.

It looks that we may have to introduce another monitor to serialize the access to the variable/field set by SET_FLAG().
The fix for that bug may be better merged here.

Comment 8

[ISHIKAWA, Chiaki]

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

I'm afraid there is no documentation.
Yes, there is likely more than one variable that needs more protection. That another one popped up is not that unexpected. What you want to be careful about is not ending up in deadlock. It's a real mess.

I want to be careful not to deadlock. For now, the test on tryserver is guidance. Any timeout that results in would be a suspect.
But so far all the timeout errors seem to have been recognized as intermittent errors before, and thus had other root causes.
At the same time, I notice a few TSAN issues pop up and so it is going to be a while before the dust settles down.

This nsImapProtocol.cpp is a mess. :-(
It is very hard to understand.

It may be wise to rewrite this using a protocol analyzer, I mean, like writing the
IMAP command parser using BISON (or original YACC) so that the syntactic part is handled by the parsing engine, and
we focus on error recovery and semantic processing.
Just a thought. I would have written a parser for IMAP command in that manner.
Such a coding style for command parser makes it rather easy to handle command syntax change, etc. I understand GMAIL extends the IMAP command syntax one way or the other. Such extension is much easier to accommodate and easier to understand in my approach, but
that is computer science student in me talking.

Comment 9

[ISHIKAWA, Chiaki]

I realize that if I make a stupid mistake, the compiler is clever enough to catch simplistic errors such as follows.

[task 2025-03-31T11:51:04.141Z] 11:51:04     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/comm/mailnews/imap/src'
[task 2025-03-31T11:51:04.144Z] 11:51:04     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang-cl -fms-compatibility-version=19.39 -std:c++17 -Xclang -ivfsoverlay -Xclang /builds/worker/fetches/vs/overlay.yaml -FonsImapProtocol.obj -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -guard:cf -D_FORTIFY_SOURCE=0 -Xclang -fno-common -DNDEBUG=1 -DTRIMMED=1 -DWINAPI_NO_BUNDLED_LIBRARIES -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DMOZ_SUPPORT_LEAKCHECKING -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/comm/mailnews/imap/src -I/builds/worker/workspace/obj-build/comm/mailnews/imap/src -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/netwerk/base -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -MD -DMOZILLA_CLIENT -FI /builds/worker/workspace/obj-build/mozilla-config.h -fsanitize-blacklist=/builds/worker/checkouts/gecko/build/sanitizers/asan_blacklist_win.txt -fsanitize=address -Zc:sizedDealloc- -Gy -Zc:inline -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -TP -GR- -D_HAS_EXCEPTIONS=0 -fcrash-diagnostics-dir=/builds/worker/artifacts -fcrash-diagnostics-dir=/builds/worker/artifacts -fcrash-diagnostics-dir=/builds/worker/artifacts -gline-tables-only -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -Oy- -Werror -W3 -Wbitfield-enum-conversion -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wenum-compare-conditional -Wenum-float-conversion -Wvolatile -Wno-deprecated-anon-enum-enum-conversion -Wno-deprecated-enum-enum-conversion -Wno-deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wno-unknown-pragmas -Wno-ignored-pragmas -Wno-deprecated-declarations -Wno-microsoft-enum-value -Wno-microsoft-include -Wno-invalid-noreturn -Wno-inconsistent-missing-override -Wno-implicit-exception-spec-mismatch -Wno-microsoft-exception-spec -Wno-unused-local-typedef -Wno-ignored-attributes -Wno-used-but-marked-unused -Wno-psabi -Wthread-safety -Wno-error=builtin-macro-redefined -Wno-vla-cxx-extension -Wno-unknown-warning-option -fno-strict-aliasing -Xclang -ffp-contract=off  -Xclang -MP -Xclang -dependency-file -Xclang .deps/nsImapProtocol.obj.pp -Xclang -MT -Xclang nsImapProtocol.obj   /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp
[task 2025-03-31T11:51:04.144Z] 11:51:04    ERROR -  /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp(5670,31): error: acquiring reentrant monitor 'm_SetFlag' that is already held [-Werror,-Wthread-safety-analysis]
[task 2025-03-31T11:51:04.145Z] 11:51:04     INFO -   5670 |     ReentrantMonitorAutoEnter serializeFlag(m_SetFlag);
[task 2025-03-31T11:51:04.145Z] 11:51:04     INFO -        |                               ^
[task 2025-03-31T11:51:04.145Z] 11:51:04     INFO -  /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp(5667,29): note: reentrant monitor acquired here
[task 2025-03-31T11:51:04.146Z] 11:51:04     INFO -   5667 |   ReentrantMonitorAutoEnter serializeFlag(m_SetFlag);
[task 2025-03-31T11:51:04.146Z] 11:51:04     INFO -        |                             ^
[task 2025-03-31T11:51:04.146Z] 11:51:04    ERROR -  /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp(5733,31): error: acquiring reentrant monitor 'm_SetFlag' that is already held [-Werror,-Wthread-safety-analysis]
[task 2025-03-31T11:51:04.146Z] 11:51:04     INFO -   5733 |     ReentrantMonitorAutoEnter serializeFlag(m_SetFlag);
[task 2025-03-31T11:51:04.146Z] 11:51:04     INFO -        |                               ^
[task 2025-03-31T11:51:04.147Z] 11:51:04     INFO -  /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp(5730,29): note: reentrant monitor acquired here
[task 2025-03-31T11:51:04.147Z] 11:51:04     INFO -   5730 |   ReentrantMonitorAutoEnter serializeFlag(m_SetFlag);
[task 2025-03-31T11:51:04.147Z] 11:51:04     INFO -        |                             ^
[task 2025-03-31T11:51:04.147Z] 11:51:04     INFO -  2 errors generated.
[task 2025-03-31T11:51:04.147Z] 11:51:04    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:674: nsImapProtocol.obj] Error 1
[task 2025-03-31T11:51:04.148Z] 11:51:04     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/comm/mailnews/imap/src'

So I am reasonably confident that when my patch builds and pass the mochitest and xpcshell-test, the code will be safe and works.

Comment 10

[ISHIKAWA, Chiaki]

I need to serialize the access to |m_runningUrl|, too. :-(
I am going to eradicate the issue one by one...

https://treeherder.mozilla.org/jobs?repo=try-comm-
central&tier=2%2C3&test_paths=builds%2Fworker%2Fcheckouts%2Fgecko%2Fcomm%2Fmailnews%2Fbase%2Fsrc&revision=c433b1fceedb8aa78d799dc2699c7c84b9055c50&selectedTaskRun=VR73-6xKTD2MeB5ZGJ2bUA.0

https://treeherder.mozilla.org/logviewer?job_id=501661169&repo=try-comm-central&lineNumber=5278

Comment 11

[ISHIKAWA, Chiaki]

Debugging monitor deadlock is fun, seriously.
TSAN build and test points out thread issues.
Debug build reports potential deadlock issues.
So I can first insert as many locks to eradicate thread issues and then try to find where I have inserted too many monitor locks, that result in potential and real deadlocks, and remove such lock entries.
I need to repeat the process. The compiler even can suggest statically discovered issues.

There is a problem: Bug 1957897
I cannot figure out the stack of where POTENTIAL issue of entering monitor twice is observed. This is a serious issue.
Only the real deadlock situation prints out the stack trace for now.

I think TSAN tests ought to have been executed periodically and then the problem we see would have been eradicated long ago.
But that is what maybe TB council can institutionalize.
(For that matter possible TSAN, and my holy grail, valgrind run as periodic tests. With these extended test runs, I can trust TB more.)

Comment 12

[Intermittent Failures Robot]

1 failures were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Table

	no_variant
linux1804-64-tsan-qr--/opt	1

For more details, see:

https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1956408&startday=2025-04-07&endday=2025-04-13&tree=all

Comment 13

[Intermittent Failures Robot]

1 failures were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Table

	no_variant
linux1804-64-tsan-qr--/opt	1

For more details, see:

https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1956408&startday=2025-04-28&endday=2025-05-04&tree=all

Comment 14

[Intermittent Failures Robot]

1 failures were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Table

	no_variant
linux1804-x86_64/tsan	1

For more details, see:

https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1956408&startday=2025-05-05&endday=2025-05-11&tree=all

Comment 15

[ISHIKAWA, Chiaki]

Those of you who build TSAN version of C-C TB on your local linux PC, and noticed build failures in the last 3-4 weeks, here is a potential fix.
https://bugzilla.mozilla.org/show_bug.cgi?id=1966570#c1

Comment 16

[Intermittent Failures Robot]

1 failures were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Table

	no_variant
linux1804-x86_64/tsan	1

For more details, see:

https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1956408&startday=2025-06-09&endday=2025-06-15&tree=all

Comment 17

[ISHIKAWA, Chiaki]

Attachment: WIP: Bug 1956408 - Fix dara race issues (some fatal) in C-C TB using TSAN, r=#thunderbird-reviewers

This is still being debugged.: I fix one race and then another appears.
Serialize access to |m_threadShouldDie|, too.
Should we or should we not serialize the access to |m_inThreadShouldDie| in threadShouldDie()?.
We SHOULD because |m_threadDeathMonitor| is private and cannot be done
out side nsImapProtocol.cpp.
Serialize access to |m_flag| via SetFlag/TestFlag/ClearFlag.
Serialize access to |m_runningUrl|.
Serialize processing in ProgressEventFunctionUsingNameWithString().
Invocation of monitor.Notify must be in the same scope where monitor
is alive.
Use mMonitor to serialize processing in ProgressEventFunctionUsingNameWithString
I think we probably need to use something on the target folder?
remove the possibility of hung in DeathSignalReceived.

Comment 18

[Intermittent Failures Robot]

1 failures were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Table

	no_variant
linux2404-x86_64/tsan	1

For more details, see:

https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1956408&startday=2026-03-02&endday=2026-03-08&tree=all

Comment 19

[ISHIKAWA, Chiaki]

I have made a real progress.

I have observed many data races during my local TSAN run.
( I could not build TSAN version of TB due to strange binary toolchain issues for like three months. But careful reinstallation of
binary toolchain finally allowed me to build TSAN version locally. ).

Now, I removed almost all of them. A few remain, but they arefrom GC and JIT in Javascript engine, and not TB per se.

During removing data races, I noticed that there were a few places where a bogus pointer (an allocated area, which MAY HAVE BEEN freed by other thread !) is copied into.
This definitely is a serious bug and could be cause of a crash.

To fix the data race issues while avoiding deadlock, I needed to introduce a few monitor variables to protect some data, and needed to introduce a version of a function that does NOT acquire a lock to avoid deadlock in a particular execution path.
Patch is not that difficult to understand on an individual basis, but pervasive in imap subdirectory.

I am testing the local patch using xpcshell test.
Most of the issues experienced by IMAP users (random crash) may be related to the data race issues and xpcshell test would catch such usage hopefully.
Once I verify the TSAN patch are harmless using mochitest, I will upload the patches.
It is just that my local TSAN patches were created after other I/O error clean up patches, I probably have to reorder the patches so that
TSAN patch can land first (since it fixes real crash-class bug).
That rearrangement of local patch order may take a bit of time.

In the meantime, I probably want to file the data race issues in GC + JIT in Javascript engine.

This is a heads-up for the eventual comprehensive TSAN patch.

Comment 20

[ISHIKAWA, Chiaki]

I am trying to update the attachment to a final version to request review.

But I could not upload it.
Does anyone know the cause of the error?

Updating revision D286113:
47939:714b566ee87a Bug 1956408 - Fix data race issues (some fatal) in C-C TB using TSAN, r=#thunderbird-reviewers
Phabricator Error: Undefined index: data

The patch fixes all the data races and deadlocks in IMAP code found so far locally and on tryserver using xpcshell tests.

Comment 21

[ISHIKAWA, Chiaki]

Without the patch to be uploaded here, the xpcshell tests show many data races.
See X6-X10 logs of tsan run.:
[1]
https://treeherder.mozilla.org/jobs?repo=try-comm-central&fromchange=22d34306f9b0df301e36f09c55f79efc787c0737&selectedTaskRun=U9QSOTZuQgOhwpEkR8v76g.0

OTOH, with the patch that will be posted here, we don't see any data races from imap module.
[2]
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=dteRxg-nTv-boXG4APHXqA.0&revision=e86c0c989038e24f0c050fb76c2c5b9ccbd6c5dc

I needed to shuffle patches around and so a few crufts might have slipped in.

Wait a second, why did the run in [2] show smaller number of xpcshell tests? Darn.
treeherder must have gone through a change. The number of ASAN tasks could add changed from a smaller # of jobs to a large # of jobs, but
it may miss crucial tests for TB???

Comment 22

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #21)

Without the patch to be uploaded here, the xpcshell tests show many data races.
See X6-X10 logs of tsan run.:
[1]
https://treeherder.mozilla.org/jobs?repo=try-comm-central&fromchange=22d34306f9b0df301e36f09c55f79efc787c0737&selectedTaskRun=U9QSOTZuQgOhwpEkR8v76g.0

OTOH, with the patch that will be posted here, we don't see any data races from imap module.
[2]
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=dteRxg-nTv-boXG4APHXqA.0&revision=e86c0c989038e24f0c050fb76c2c5b9ccbd6c5dc

I needed to shuffle patches around and so a few crufts might have slipped in.

Wait a second, why did the run in [2] show smaller number of xpcshell tests? Darn.
treeherder must have gone through a change. The number of ASAN tasks could add changed from a smaller # of jobs to a large # of jobs, but
it may miss crucial tests for TB???

The place where imap xpcshell tests are executed moved...
inux 24.04 asan run, X[tier2]'s X2 is where the imod xpcshell tests are run.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=agiEcORZQRGgVz5noSgRsA.0
Look at the live.log file there.
https://firefoxci.taskcluster-artifacts.net/agiEcORZQRGgVz5noSgRsA/0/public/logs/live_backing.log
Near the bottom we find lines like:

[task 2026-03-15T06:55:45.772+00:00] 06:55:45     INFO - TEST-START | xpcshell-maildir.toml:comm/mailnews/imap/test/unit/test_autosync_date_constraints.js
[task 2026-03-15T06:55:46.235+00:00] 06:55:46     INFO - TEST-PASS | xpcshell-maildir.toml:comm/mailnews/imap/test/unit/test_autosync_date_constraints.js | took 462ms

and all the tests ran successfully. (No error is shown. Usually, data race is reported as in the case of [1] above.

I still cannot upload the patch via phabricator:

Updating revision D286113:
47939:714b566ee87a Bug 1956408 - Fix data race issues (some fatal) in C-C TB using TSAN, r=#thunderbird-reviewers
Phabricator Error: Undefined index: data

If you are in dire need to see if the data races are gone, you may need to copy it from here.:
https://hg-edge.mozilla.org/try-comm-central/rev/8ec3b2591c130f1a99553688a8bfbe4d9b5c0241

Comment 23

[ISHIKAWA, Chiaki]

Attachment: 1956408.patch (phabricator does not work for the moment)

This is the patch that was used to create the TSAN-free IMAP moduel in
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e86c0c989038e24f0c050fb76c2c5b9ccbd6c5dc&selectedTaskRun=dteRxg-nTv-boXG4APHXqA.0
Since for some strange reason, moz-phab refused to work with

47941:1ea1adca745a Bug 1956408 - Fix data race issues (some fatal) in C-C TB using TSAN, r=#thunderbird-reviewers
Phabricator Error: Undefined index: data

I am uploading this for people who might want to try this out.

The patch includes a clean shutdown whereby task was not properly woken up before shutdown to enter cleanup phase.
So this may even fix Bug 1524247 and friends.
Someone who experience the problem under linux can test the binary there and if TSAN-binary (available only for linux) produces runtime error can paste the run-time warning here so that I can add more fixes.

Comment 24

[Intermittent Failures Robot]

1 failures were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Table

	no_variant
linux2404-x86_64/tsan	1

For more details, see:

https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1956408&startday=2026-03-09&endday=2026-03-15&tree=all

Comment 25

[ISHIKAWA, Chiaki]

Attachment: updated patch (phabricator does not work for me for the moment :-(

I have not been able to upload my local patch due to a strange error.
So for people who want to test the patch, I am uploading it as an attachment.

This was used to submit the job:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9735df0b7313ce64255f4a1c33745ec5293a3ae2

Please note that there no Xpcshell test errors in linux 24.04 asan (!)
This is remarkable.
For one test, test_imapChunks.js, I had to set the timeout (10 seconds) to 30 seconds because
ASAN binary runs very slow (5-20x) and 10 seconds timeout was too short.
Otherwise, known observed data race issues, thread races during shutdown, etc. are all
addressed and removed as far as imap xpcshell tests could find for imap module on my local PC and treeherder..

There are legitimate races in GC and I had to white list them and a few data races reported in
DEBUG-only code.

I THINK many imap crash issues are solved with this patch.

In the last 24 hours, I removed strange issues at shutdown. No proper cleanup for shutdown took place for IMAP thread if it was sleeping.
This and other issues (running I/O on non-main thread!) I found for the last few days changed the patch significantly, and I assume similar problems are dormant in the code, in code paths not executed by xpcshell tests.
But I think it is a milestone that TSAN could run imap test without any errors as shown on the treeherder, and
I hope someone can review this and land soon.

Now, I wonder why moz-phab stopped working on my PC.
(I had a file system overflow problem a few days ago, and I wonder if that was the cause. But a source code control system and tools related with it are hardened against such I/O errors, I presume.)

Comment 26

[ISHIKAWA, Chiaki]

Attachment: Fix data race issues (some fatal) in C-C TB using TSAN.

IMAP is a rabbit hole of bugs. I fix one race and then another appears, or a deadlock.

However, I have finally come to the step where no TSAN warnings are observed and no IMAP-related deadlocks are reported during xpcshell tests for IMAP locally. (There are legitimate errors related to M-C code and others as observed on treeherder.)

--- Background and History ---

Initially, I started to serialize access to |m_threadShouldDie|. I had to add many locks and retract when deadlocks occur.

Should we or should we not serialize the access to |m_inThreadShouldDie| in threadShouldDie()? We SHOULD because |m_threadDeathMonitor| is private and cannot be done outside nsImapProtocol.cpp. (Handling this turned out to be tricky, but it is done now).

Technical changes included:

• Serialized access to |m_flag| via SetFlag/TestFlag/ClearFlag.
• Serialized access to |m_runningUrl|.
• Serialized processing in ProgressEventFunctionUsingNameWithString.
• Invocation of monitor.Notify must be in the same scope where monitor is alive.

Now almost all data races in C-C are gone. TSAN adds much overhead; if I run all xpcshell tests (M-C and C-C), it does not finish in more than 12 hours inside my linux instance. After proper fixes, the C-C portion now runs in about 1 hour.

--- Final Changes Summary ---

1. Sink Proxy Synchronization (The CC Crash Fix)
   RefPtr proxies (Folder, Message, Server sinks) were being read/cleared across threads without protection. Standardized mMonitor for these. Every assignment is now wrapped.

2. Lifecycle Flag Unification (The TSan Fix)
   Discovered a "Mismatched Monitor" where running status used an obsolete NSPR monitor. Purged PR_CEnterMonitor(this) and unified with mMonitor.

3. Notification Signaling (The Treeherder Hang Fix)
   IMAP thread was "sleeping" through shutdown signals. Added NotifyAll() to TellThreadToDie so it immediately wakes up and exits instead of timing out.

4. Code Audit of Monitors
   Verified m_threadDeathMonitor, m_pseudoInterruptMonitor, and m_mockChannelMonitor architecture.

--- Final Technical Summary ---

I resolved two major issues causing Data Races and Deadlocks:

• The Deadlock: Switched from NS_DispatchAndSpinEventLoopUntilComplete to NS_DispatchToMainThread to make cancellation asynchronous, preventing circular dependencies.
• The Data Races: Implemented a "Capture, Null, and Release" pattern using mMonitor to prevent the Main Thread from seeing variables while the IMAP thread is still processing them.

Current status: imap xpcshell tests passed 3516 checks (133 tests) with zero unexpected results. The TSan run is officially Clean.

--- Notes ---
I am fixing data races/deadlocks inside C-C TB. There remain some races in GC/JIT/Network cache during shutdown which are outside the scope of this module.

Comment 27

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #26)

Created attachment 9553965 [details]
Fix data race issues (some fatal) in C-C TB using TSAN.

I had to use a manual submission web page because moz-phab (and actually arc command that is called behind it as well) were
out of date with the lately updated python version on my Debian GNU/Linux environment.
I may not be able to update the patch as often as I improved the patch locally.

Comment 28

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Fix %x format specifier mismatch in nsAutoSyncState.cpp. r=#thunderbird-reviewers,mkmelin,BenC

IMAP is a rabbit hole of bugs. I fix one race and then another appears, or a deadlock.

However, I have finally come to the step where no TSAN warnings are observed and no IMAP-related deadlocks are reported during xpcshell tests for IMAP locally. (There are legitimate errors related to M-C code and others as observed on treeherder.)

--- Background and History ---

Initially, I started to serialize access to |m_threadShouldDie|. I had to add many locks and retract when deadlocks occur.

Should we or should we not serialize the access to |m_inThreadShouldDie| in threadShouldDie()? We SHOULD because |m_threadDeathMonitor| is private and cannot be done outside nsImapProtocol.cpp. (Handling this turned out to be tricky, but it is done now).

Technical changes included:

• Serialized access to |m_flag| via SetFlag/TestFlag/ClearFlag.
• Serialized access to |m_runningUrl|.
• Serialized processing in ProgressEventFunctionUsingNameWithString.
• Invocation of monitor.Notify must be in the same scope where monitor is alive.

Now almost all data races in C-C are gone. TSAN adds much overhead; if I run all xpcshell tests (M-C and C-C), it does not finish in more than 12 hours inside my linux instance. After proper fixes, the C-C portion now runs in about 1 hour.

--- Final Changes Summary ---

1. Sink Proxy Synchronization (The CC Crash Fix)
   RefPtr proxies (Folder, Message, Server sinks) were being read/cleared across threads without protection. Standardized mMonitor for these. Every assignment is now wrapped.

2. Lifecycle Flag Unification (The TSan Fix)
   Discovered a "Mismatched Monitor" where running status used an obsolete NSPR monitor. Purged PR_CEnterMonitor(this) and unified with mMonitor.

3. Notification Signaling (The Treeherder Hang Fix)
   IMAP thread was "sleeping" through shutdown signals. Added NotifyAll() to TellThreadToDie so it immediately wakes up and exits instead of timing out.

4. Code Audit of Monitors
   Verified m_threadDeathMonitor, m_pseudoInterruptMonitor, and m_mockChannelMonitor architecture.

--- Final Technical Summary ---

I resolved two major issues causing Data Races and Deadlocks:

• The Deadlock: Switched from NS_DispatchAndSpinEventLoopUntilComplete to NS_DispatchToMainThread to make cancellation asynchronous, preventing circular dependencies.
• The Data Races: Implemented a "Capture, Null, and Release" pattern using mMonitor to prevent the Main Thread from seeing variables while the IMAP thread is still processing them.

Current status: imap xpcshell tests passed 3516 checks (133 tests) with zero unexpected results. The TSan run is officially Clean.

--- Notes ---
I am fixing data races/deadlocks inside C-C TB. There remain some races in GC/JIT/Network cache during shutdown which are outside the scope of this module.

Comment 29

[ISHIKAWA, Chiaki]

I think my moz-phab uploaded succeeded and thus I am abandoning the previous patch which was posted using web-based manual entry (I did not know such a beast existed. I found it out of desperarion.) I am not sure how to hide it from listing on this page, though.

My moz-phab did not work after a python upgrade :-(
I wish it had a better warning regarding version mismatch of expected python interpreter.
It took me a long time to figure this out and during the investigation, I corrupted my repository by mistake, even. :-(

Comment 30

[ISHIKAWA, Chiaki]

In the phabricator page:
‘‘‘
D288367: Fix data race issues (some fatal) in C-C TB using TSAN.

mkmelin commented on this revision
View comments

    In D288367#10001661, @Chiaki wrote:

Try ./mach install-moz-phab -f

‘‘‘
Thank you for the tips.

I am not sure if what I did was done by the above command.
The way things were going, there was a mismatche of the python version on my Debian GNU/Linux after a recent package upgrade and the version expected by moz-phab and arc command, and until I figure it out, moz-phab failed to execute very well.
I wish moz-phab had a version check at the beginning of the execution so that unsuspecting user will know what has gone wrong. (Maybe I should file an enhancement request for this.)
Anyway, I created a virtual environment that is used for mozilla commands and all seems to be well at this moment (until something breaks again soon, I suppose.)

Thank you again for the tips. Next time, I will try the above command to see if it solves my situation.

Debian GNU/Linux won't allow a system-wide package update/upgrades/downgrade/installation of python interpreter/packages and that is also a cause of headache.
But since many system tools use python interpreter, for a particular version of Debian GNU/Linux, I understand that the developers needed to pin the version used by system utilities, and the users need to use virtual environment to experiment with a newer or older version of the python interpreter.
To be honest, how I ended up with incorrect version is a bit mystery because I run |./mach bootstrap| occasionally, and I notice that
during |./make configure| a virtual environment or two seem to be created for compilation task. Maybe the test repository of Debian GNU/Linux simply tried to install a bleeding edge version which mozilla does not use for now.

Comment 31

[ISHIKAWA, Chiaki]

I think I need a couple of days at least to get the local repositories in a good shape to post the patch using moz-phab.
In the meantime, here is the major break down of what I did.

Point 1: In mozilla code, data race of a reference counted pointer can lead to a crash in GC later. So we need to
eradicate them as much as possible.
Point 2: avoid deadlock, of course. TSAN is clever to detect deadlock as long as we use Mutex for synchronization.
So, use Mutex to make sure to avoid data races and let TSAN detect deadlocks. This approach worked well.
However, I noticed that there were PR_CEnter/PR_CExit calls in IMAP module. These ought to be removed since PR_CEnter/PR_CExit is legacy, and TSAN cannot know about them during runtime. I did remove a few deadlock cases and many data races using TSAN only after I replaced the use of PR_CEnter, etc. with proper Mutex variables
I have created the proper order the use of Mutexe in 4 level tiers to make sure the multi-acquisition follows order of tiers from the top to the bottom.

Changes in files:

• nsImapFlagAndUidState.cpp— Replaced all PR_CEnterMonitor(this) with a proper m_flagStateLock (Tier 4 leaf Mutex), and added mLock for hash tables/user flags. Clean two-tier locking, I left comments.

• nsImapIncomingServer.cpp — Replaced all PR_CEnterMonitor(this) with m_serverLock (RecursiveMutex, Tier 1). Important fix in CloseConnectionForFolder: TellThreadToDie now called outside the lock to prevent GC SEGV.

• nsImapProtocol.cpp — Very extensive:

• PR_CEnterMonitor(this) purged from TellThreadToDie, replaced with mMonitor held through entire teardown

• protocolSink->CloseStreams() called outside lock (Rule #4. Well Mutex usage rules are spelled out in nsImapProtocol.h)

• m_threadDeathMonitor.NotifyAll() added so IMAP thread wakes immediately on shutdown

• ImapThreadMainLoop restructured to use m_urlReadyToRunMonitor consistently

• Sink proxy assignments (m_imapMailFolderSink, m_imapMessageSink, m_imapServerSink) now under mMonitor

• GetImapHostName/GetImapUserName return by value (safe copy under lock)
  The above was the first possible crash issue I realized early, but it was only the tip of an iceberg.

• CanHandleUrl wrapped in mMonitor with parser state access via AcquireParserStateLock()

• nsImapSaveMessageToDisk cleanup dispatched asynchronously to avoid deadlock

• m_SetFlag (ReentrantMonitor) serializes SetFlag/TestFlag/ClearFlag

• m_ProgressEventFunctionUsingNameWithString monitor added

• nsImapServerResponseParser.cpp — Added GetSelectedMailboxNameLocked() for use when mLock already held.

• New test test_imapTermination.js — Tests idle cleanup, active stream interruption, and teardown resilience.
  This will help us if future developers tinker with the teardown sequence of IMAP module.

• Updated lock hierarchy in nsImapProtocol.h — Now 4-tier: m_serverLock → mMonitor → m_threadDeathMonitor → m_flagStateLock.
  I documented the layered use of locks so that fixing lock inversion problems has a clear guideline for future developers.

I also added proper locks to access array variable consistently.
This is a FUNNY THING which makes me suspicious.
The failure to properly protect the array variable access was one of the FIRST things which TSAN pointed out when TB was compiled using GCC for TSAN test in late 2014- early 2025 time frame Now I use Clang for TSAN test since due to a minor link issue I experienced with GCC-15 which has been solved a couple onths ago.
But this time, it was one of the LAST things the debug session with TSAN uncovered.
It is possible that with earlier introduction of Mutexes in other key places, the variable access under no Mutex protection was not noticed and only became visible again now that all other issues are fixed.
However, different compilers may create code with slightly different timings and I may be able to find more subtle data races, runnable thread starvation, etc. using GCC. So I may try GCC-15 TSAN creation to decide that the patch here is complete for now.

Note: The runnable thread starvation occurred and is now fixed more or less. I think the original design was too flaky to reach this runnable thread starvation state. Now with proper locks and so on, during the teardown , both MAIN and IMAP threads proceed too fast and both think there is nothing to do, MAIN threading waiting to see IMAP thread to die. But in the original design, which I don't want to re-build from scratch (more on this later) by the time MAIN thread tells IMAP thread to die, it may have gone into a wait (with 60 seconds timeout) and won't notice the news from the MAIN thread to exit. When both IMAP and MAIN threads are waiting for something in this manner, IMAP thread will time out in 60 seconds wait, and proceed. In the legacy code, there were deadlocks, or data races that would result in crash during GC. At least, these are removed now.

All in all, I BELIEVE the code will feel a bit quicker to run since now small wait here and there seem to have been cleaned and not encountered that often. And this 60 seconds wait by IMAP thread at close time will happen hopefully less often than before.
I may come back on this last issue in future patch. Before I started on this work, I only knew there were threads (main and imap). Now
I have a better view of how two threads interact (before in a very flaky manner), and now I am aware of the very serious restraint of reference counted pointers allocated in main thread needing to be freed (the reference count becomes 0) in MAIN thread. The violation of this restrained ccurred quite a few times during debugging (and ending in SEGV. it is checked by mozilla runtime) and I understood the precarious state of the legacy design.

MY plan for now is to as follows.
1 .I would upload the current patch I am using locally first so that some devs who got bitten by IMAP crashes can test a version with that patch.
But this will take a few days. : I need to somehow restore my local repositories which were messed up when moz-phab (and I now realize possibly hg itself) did not work very well due to python version mismatch.
2. I would clean it up as suggested in the review board.
I will repeat the cycle of "edit and compile/test locally and on treeherder ->upload via moz-phab -> more review" until it is acceptable.

To be honest, I have doubt if someone can understand the issues I needed to solve and why/how the particular style was adopted to solve. them
Major issues were data races and deadlocks. : I needed to use TSAN to spot them and then need fix them. That was the basic approach.
The mechanism I could use was proper use of Mutex variables, and "proper" signalling via variables under protection of
Mutex. That is a principle all right. However, there are gory details such as what one would normally use is not implemented in the classes used in IMAP modules, so I had to resort to the nulling of some pointer variables to signal that something is no longer available, etc. Or closing streams that no communication channel exists any more and thus, a thread must die, etc. Thread teardown, especially during shutdown is stll messy.

Usually, a patch is produced when no major issue is left. I could not reach that stage until this patch. Thus so many changes in one big patch.
The reason was simple. I had to add many changes before imap xpcshell tests will not produce flurry of errors including deadlocks.
I could not produce a meaningful intermediate state because one fix leads to another data race / deadlock with no end in sight.

So "understanding" some of the changes here may take time. Actually, it may be impossible to understand why until you run the code by the change removed. But there is no simple chunk to remove. Many changes here interact with each other in many subtle manners and the problem happen only on certain timings (there are issues which did not happen on my PC until I spot it on treeherder and repeat the test locally a few times more).

More on the teardown time issues: Often TB does not quit soon enough. There is a long pause.
Some classes used in IMAP module does not implement some nice functions which the base class would use and thus
to avoid the crashes/deadlocks/signalling (after many trials and errors, of course), eventually I had to use a sledgehammer approach of closing stream(s )which TB uses to talk to an IMAP server and, depending on the timings, / and now due to the efficient locking/unlocking and deadlock removal, it is more likely the code closes the streams before BYE or LOGOUT is issued (again this depends on particular timing).
This may have a practical consequences that the fake IMAP server did not seem to realize what happened on TB side (that is quitting earlier than before in cases of early interrupt, etc.), and fakeserver may wait for "LOGOUT" or "BYE" which would never come, and xpcshell test harness may need to wait until a short timeout to finish the test. TB binary will have quit by then already.
Also, I may have uncovered a few test programs that hold implicit assumptions about this timing sequence during shutdown. (This will be taken care of by follow-up when the issues in test programs are understood in more detail.)
TB is now much quicker to shutdown. There were times 60 seconds wait could kick in. The frequency of 60 second wait should be much less.

NEW DESIGN
For these type of protocol handling, I will create a protocol handler engine using
YACC/Bison and embed the code to handle the particular situation in which the protocol handler engine is. in YACC/Bison rule handler. That is much neater and cleaner solution to handle communication protocol. I have done that a few times, and have been disappointed that such approach did not become widely adopted in the industry.
By this approach, one can clearly see how the error is handled, etc., and managing the change in the original protocol much easier
Today, it is not clear how the error in the IMAP protocol handled in the IMAP code. Believe it or not, I don't know. I simply relied on xpcshell tests to test that out, and I only paid attention to data races and deadlocks using TSAN.
But such a rewrite or rebuild would need a much more effort and I am not even sure if IMAP will last for another 10 years or more.
If anyone is interested in paying me for that, :-)
I will taken on that task.
The code in IMAP feels so legacy in today's world, and yet so many people seem to depend on that.
For example, the use of PR_Enter/PR_Exit.
According to searchfox, there are 42 appearances of PR_CEnter. A few are definitions and in comment line.
https://searchfox.org/comm-central/search?q=PR_CENTER&path=&case=false&regexp=false
I am removing 23 such real usages (not on comment line) in IMAP-related files. I think that is a good move, but there are still 8 more usages in mail/mailnews code.
They need to be converted to Mutex for better TSAN debugging.
But as far as I can tell, IMAP code is in MUCH BETTER shape from the viewpoint of TSAN as shown by green sign of xpcshell tests.
I am not sure how the usage in M-C code side may have an impact on TB, though.
It is outside the scope of this patch, and probably felt during UI handling.

EDIT: Fixed typos and some obscure expressions.
EDIT2: A few more fixes for typos and obscure expressions. One can tell that this was written by a human.

Comment 32

[ISHIKAWA, Chiaki]

Now I realize the appearances of PR_CEnter in M-C code portion are definitions/declarations and not usage.
So maybe PR_CEnter/PR_CExit pairs are no longer used in M-C, and C-C is possibly the last users of them (!).

Comment 33

[ISHIKAWA, Chiaki]

I switched the local compiler from Clang to GCC to see if the subtle speed difference of compiler generated code and their own runtime can trigger hither-to-unknown issues.
Bingo (!)

I could find a data race and a hung during the xpcshell tests under imap/test
But from there, I needed to spend a few more days to eradicate the issues that popped up after each fix/compile/test runs.
imap code IS a legacy code.

Then finally, believe it or not, I could ascertain that, during the whole runs of imap xpcshell tests, only one or two GC-related issues are eported deep within incremental GC, which is beyond the scope of imap improvement.
(I changed the amount of output from MOZ_LOG so that I have a different timing from that end. Now, GCC, CLANG version both passed the tests. Huge improvement.)

But then I find out one problem on treeherder.

On treeherder, clang compiles files and during compilation, it checks for some code unsafe patterns to make sure
no new bugs are introduced with the careless use of certain code patterns.
My modification to handle pointer releasees during shutdown were checked with this constraint checker, and did not compile.
(It does and runs fine using clang on my PC.)

So I had to edit a few parts and have to wait for the compilation to abort of finish on treeherder.
My effort to install the compiler plugin locally failed. (Building it interferes with the TSAN build environment and I don't want to break the development environment as of now.)

So it may need a few more days before I can submit the phabricator patch.

My current effort to get a clean build after a few tries.

https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=JPUktuV1Qkip51KxBmti7g.0&revision=37d4f1dd5e750bcaf4e0bfed3201207aed1c2172

Comment 34

[Magnus Melin [:mkmelin]]

If you didn't have it already, add this. It's on for CI anyway.
ac_add_options --enable-clang-plugin?

Comment 35

[ISHIKAWA, Chiaki]

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

If you didn't have it already, add this. It's on for CI anyway.
ac_add_options --enable-clang-plugin?

I have tried various settings. But the compiler spit out errors.
I tried various settings for path setting in mozconfig and setting of CXX* envionnmental variables, but I gave up.
It seems that the particular version of rustc setup (pinned down to a particular version) interferes with the compilation somehow.
I simply don't understand the details.

For now, there are NOT going to be major rewrite (Famos last words) , only minor straightforward rewerites to make sure that proper variable/member accesses occur under forgotten or misplaced locks. In that case, I don't have to use arcane syntax/functions of refcnt pointers and worry about which threads are going to touch it and destroy it during shutdown phase.
That won't be contested by mozilla's strickt checker on treeherder, I think.

Comment 36

[ISHIKAWA, Chiaki]

Attachment: IMAP Threading Stabilization: Data Race & Deadlock Elimination (April 2026)

Comment 37

[ISHIKAWA, Chiaki]

I had to modify the patch after I noticed a couple of data races on treeherder.
That led to MANY MORE fixes in the phabricator patch I updated.

I noticed a problem. Due to a corrupt local repository I experienced, I needed to resurrect the patch from previous copy, and I obviously lost some comment changes I had done. I will fix them.
(I think "hg refresh -e" is the culprit. It seems to place the repository and HG MQ in a very strange sate. I still use HG MQ locally)

I uploaded a summary of what I have done for the last few weeks in a previous comment.

Comment 38

[ISHIKAWA, Chiaki]

I also notice similar comments (for future maintainers) without removing the previous copy. I will fix them.

Comment 39

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #38)

I also notice similar comments (for future maintainers) without removing the previous copy. I will fix them.

One example is as follows.
I obviously left three similar but different copies at different times.
The original three copies are to be replaced with a simpler one.
I record the older comments here for the benefit of future maintainers.
I wanted to leave as many comments as possible about what I found out about the code statically and dynamically during tests, and thus
often left the previous comment in haste thinking that I have removed the older comment in my Emacs editor buffer.

---

OLD (three comments).
This comment blocks are just before GetDatabase() in
nsImapMailFolder.cpp:

/**

• HISTORICAL NOTE ON THREAD SAFETY AND NOTIFICATIONS:
• The legacy version of GetDatabase() "got away" with a non-thread-safe
• implementation due to two factors that no longer apply:
• • 1. Accidental Timing: Without ReentrantMonitors, the database
• initialization and listener attachment happened fast enough on a
• single thread that the "silent window" for notifications was rarely hit.
• 2. Evolution of Tests: Modern xpcshell tests (e.g.,
• test_trustSpamAssassin.js) use Promise-based notification listeners that are
• far less tolerant of missed SummaryChanged events than older timeout-based
• tests.
• THE FIX:
• We now perform a "Late Attachment" within a locked monitor. We MUST:
• • Assign mDatabase (publish it) BEFORE calling UpdateSummaryTotals so that
• any recursive calls to GetDatabase() exit early.
• • Attach the listener AFTER publishing mDatabase but BEFORE the update
• calls so we don't miss the initialization signals.
• • Skip attachment for \Noselect or Dummy folders to avoid the protocol
• hangs seen in test_dontStatNoSelect.js.
  */

/**

• THE GETDATABASE INITIALIZATION ATTRACTOR:
• Handles synchronization between IMAP threads, UI, and async unit tests.
• This function handles a high-stakes synchronization point between the
• IMAP protocol thread, the UI, and Promise-based unit tests.
• --- THE "LOOP/SLEEP" MISSING LINK (CRITICAL): ---
• Previous iterations used a generic "if busy, return" guard which failed
• because it could not distinguish WHO was busy:
• THE "LOOP vs. SLEEP" RESOLUTION:
• 1. RECURSION (The Storm): If UpdateSummaryTotals triggers GetDatabase,
• mInitThreadCount > 0 identifies this thread is the owner. We return
• NS_OK to break the infinite recursion loop.
• 2. RACE (The Sleep): If Thread B enters while Thread A is in Step 4,
• Thread B must mon.Wait(). If it returned NS_OK immediately (old flaw),
• it would see a "half-baked" folder and hang or miss notifications.

• ---

• RECONSTRUCTION OF THE "SILENT WINDOW" AND NOSELECT HANG (Detective Log):
• 1. THE PROBLEM: Modern Promise-based tests (test_trustSpamAssassin.js)
• timed out because they missed the initial 'SummaryChanged' notifications.
• 2. THE ATTEMPTED FIX: Moved AddListener() into GetDatabase() to ensure
• attachment before UpdateSummaryTotals() fired.
• 3. THE REGRESSION (test_dontStatNoSelect.js): Introduced a permanent
     `* hang in the IMAP thread at ReleaseUrlState for \Noselect folders.
• 4. THE DIAGNOSTIC FAILURE (The "Silent Flags" Mystery): mFlags is 0
• during the VERY FIRST call; bitmask checks fail here.
  /
  /*
• THE GETDATABASE INITIALIZATION ATTRACTOR (Deadlock & Recursion Guard)
• This function provides a strictly synchronized "Cold Start" for the folder
• database. It is designed to bridge the gap between the Main Thread's need for
• instant metadata and the IMAP thread's asynchronous protocol handshake.
• HISTORICAL CONTEXT & ARCHITECTURAL NECESSITY:
• 1. THE RECURSION STORM: Step 4 (Post-Initialization) calls
• UpdateSummaryTotals, which can synchronously re-enter GetDatabase. Without
• mInitThreadCount, this causes an infinite recursion hang on the Main Thread.
• 2. THE t65.log DEADLOCK (Wait-Bypass): A circular dependency was observed
• where the Main Thread is in Step 4 (holding 'Owner' status) and the IMAP
• thread enters GetDatabase. If the IMAP thread sleeps in mon.Wait() while
• the Main Thread waits for an IMAP response to finish the summary update,
• the process deadlocks.
• FIX: Case 2 allows secondary threads to bypass the wait if mDatabase is
• already published, as the pointer is stable enough for protocol tasks.
• 3. THE SILENT WINDOW (test_dontStatNoSelect.js): Promise-based tests often
• assert immediately after an action. Step D ensures we manually notify
• observers to bridge the time gap before the internal DB listeners catch up.
  */

NEW:
/**

• HISTORICAL NOTE ON THREAD SAFETY AND NOTIFICATIONS:
• The legacy implementation of GetDatabase() appeared to function correctly
• despite lacking proper thread-safety guarantees. This behavior depended on
• conditions that are no longer valid:
• 1. Accidental Timing:

•  Without ReentrantMonitors and with simpler execution ordering,
  

•  database initialization and listener attachment typically completed
  

•  without observable interleaving. This masked underlying races.
  

• 2. Limited Reentrancy:

•  Earlier control flow rarely re-entered GetDatabase() on the same thread
  

•  during initialization, avoiding inconsistent intermediate states.
  

• Modern changes (including reentrant monitors, stricter threading checks,
• and more asynchronous execution) invalidate these assumptions. As a result:
• • GetDatabase() may be re-entered before initialization completes.
• • Notifications or listener attachment may occur on partially initialized

• state.
  

• • Cross-thread access can expose races that were previously hidden.
• Therefore, callers must treat GetDatabase() as a potentially reentrant and
• thread-sensitive operation. Implementations must ensure:
• • Explicit serialization of initialization.
• • Safe listener registration ordering.
• • No reliance on timing-dependent behavior.
• Any refactoring in this area should assume that previous behavior was
• accidental rather than guaranteed, and must preserve correctness under
• fully concurrent and reentrant conditions.
  */

Comment 40

[ISHIKAWA, Chiaki]

My approach to solve the thread issues

Basically, my approach was to remove observed data races with proper synchronization.
You can see various data races in non-pached imap module.
See for example, the treeherder job: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=71936f395e97c499b5b9526f529b0ec76d16819b

PHASE-A (repeated many times)

step 1: identify data races.

So I tried to add monitor variables to protect data from thread races observed in TSAN run of xpcshell tests.
I needed to add a few such monitor variables to protect different variables.

step 2: THEN, I began hitting deadlocks.

Step 3: So I have to look the interaction of threads carefully to avoid deadlocks.

Actually, fixing one data race, and fixing a deadlock changes timing very much and so I got new data races.
So I had to go back to Step 1, and repeat the process by making sure that all the existing test under imap/test/unit run successfully.

Phase B
After certain repetition of phase A, I settled on four monitor variables (later increased to 5) to protect various data.
I decided on the proper acquisition order of monitors in nsImapProtocol.h and stuck to the order in my patch.

Then I began focusing on the shutdown/teardown crashes caused by data races, etc.
I realize that IMAP thread ask the MAIN thread to release the objects which MAIN thread has created in a synchronous manner.
However during the shutdown phase, the MAIN thread may have already begun shutting down and cannot honor such requests.
If so, synchronous requests would result in a deadlock. So I adopted asynchronous request. If the MAIN thread cannot respond or has died, Ilet the IMAP thread to 'forget' the pointer created in the MAIN thread and leak the data rather than trying to release the pointers created in MAIN thread. Doing so in IMAP thread would cause crash or incorrect behavior in GC that runs afterward.
Eventually, after adopting asynchronous dispatch to MAIN thread (request and proceed asynchronous manner) during shutdown,
I found it necessary to redesign of deadlock avoidance, etc. and during testing, I uncovered a few more races which had been protected and dormant in a synchronized path. Asynchronous behavior uncovered many such issues.

So I had to go back to Phase A and weeded out several data rases and removed deadlocks and/or missed requests among threads.

Phase C

After a repetition of Phase A and Phase B I came to a phase where I did not see data race at all on local PC. Great (!)
Yet, the treeherder job revealed a few more data races from time to time. The timings are different due to CPU difference, compiler difference (clang on treeherder, gcc on my local PC0,) different workload (my PC runs the test rather quickly since I am the only user). This was about a few weeks ago.

During this phase, I found a few more synchronization issues regarding read/access of a few variables. But now I did not want to go through phase A and phase B and create more monitor variables and think hard about avoiding dead locks. I felt five monitor variables are already enough, and trying to unwind the established and tested synchronization framework and create new order was too much.
So I introduced atomic variables and use them and saw they are good enough for our purposes.

Phase D.

After phase C, I could run all test files in imap/test/unit files successfully. There was a test program or two which depended on an incorrect implicit assumption about the IMAP module behavior and I had to change them. Actually, they have occasionally failed before on treeherder and on local PC and I wondered what were the causes. The modified test is in the phabricator patch with an explanation. I also introduced and am introducing a dozen or so new test programs to make sure the important patches won't be reverted in the future by someone who does not understand the delicate issue of threading.

I found a few more data races even after this. The old legacy code was written without much regard to thread concurrency. :-(
Also I mistyped the names of monitor variables here and there. They have been fixed.

To be frank, I don't expect casual developers to understand the changes in depth. I am afraid that people have to trust the code as is.
The change was only meant to solve thread issues and no logic is changed. A few timing differences are now exposed and taken care of in the test programs. So all casual developers need to accept is that the changed IMAP module run the existing tests successfully.
However, I felt that the coverage was not enough. That is why I am adding tests now so that the confidence in the modification will be high.

Comment blocks

One thing I noticed is the total lack of documentation. The legacy code was never documented well.
The fake imap server used for testing is not documented very well either.
I added comments what I found about the old code and what I am doing in the new code as much as possible because I believe
the lack of documentation hampered the upgrading of the legacy code to keep up with the times.
Comments in web-based tool such as bugzilla is not enough. I believe in comments in the source file.
If you don't agree with this, think about what was lost during the transition from CVS to HG. That was when the
old knowledge was lost forever. I don't want to see that happen again. If only the code was sprinkled with comments by the original authors.
The up-to-date comment in the source file is always the best.

Yes, the comments in the source file tend to drift from the real code unless it is kept up to date by discipline.
My native language is not English and writing such comments have been a chore. However, recent LLM AI service can create such
English comments based on the latest code rather easily. So I think it is NOT too much to expect future maintainer to update the
comments when the code is changed. I think we can insist on this when future patches are accepted.
If the future developers don't update the comments, then this code base will again rot quite rapidly.

Alternative Approach?

I don't think there is no alternative approach if we go the path of incremental update without any documentation at all.
Solving the thread race issues and not changing the exposed API as much as possible, many people will come up with a very similar patch.
The only major decision I had to make was that going asynchronous for the teardown handling (requests from IMAP thread to MAIN thread are now handled in asynchronous manner) to avoid deadlock and not crashing on invalid reference counted pointers. But this again, there was not much choice since MAIN thread may be dying or has died and would not process the request from IMAP thread at all. Thus synchronous teardown always has a chance of deadlock and/or IMAP thread crashing due to reference counted pointers handled incorrectly. In my approach, I avoid the deadlock by asynchronous request, and if the service by MAIN thread is not available, I let the pointers allocated in the main thread to leak rather than crash. This is the only sensible choice in the random teardown sequence experienced in mozilla software.
So again, the details may differ, but the overall approach would not be that different.

The above is my practical incremental paproach to eliminate the data races and deadlocks without the aid of good documentation.

Why solving the TSAN issues?

BTW, you may wonder WHY I wanted to solve threading issues in IMAP.
This actually comes from a very different angle. I have been checking the memory issues of C-C TB since I depended much on thunderbird
for my day work and thus I hate to see TB crash at random. I fixed many memory problems in C-C TB using valgrind, and when valgrind stopped working after transition from mozmill test suite to mochitest suite several years ago, I used ASAN as a limited memory tester. valgrind again started to work about a couple of years ago, though. Mind you the |mach| interface to invoke valgrind for testing does not work well if I need to pass many parameters to valgrind. Shell quoting issues are not handled correctly. :-(
I had to create a wrapper program that invokes C-C TB binary under valgrind, and put it in the C-C TB binary path after saving the C-C TB binary under a different name.

During ASAN testing (or was it valgrind?), I encountered a very strange memory issue which I could not comprehend well, and even suspected of GCC code generation error, and submitted a report to GCC team to court their opinion. The answer there was: the issue quite likely caused by a thread issue and please make sure that TSAN binary runs cleanly before discussing the issue as potential real memory issue and / or compiler code generation issue.
I concurred and thus started to work on this TSAN clean C-C TB about one and half a year ago or so.
Now that I think about it, the memory issue DID seem to be a thread issue which I think my current patch probably has solved. Ore if it was a memory leak issue experienced during shutdown, the current code handles it explicitly.

Once the patch for IMAP module lands in the current or modified form, I will go back to ASAN/valgrind testing. This is my plan.

Future work for someone

Actually I think the IMAP module will benefit much to have a proxy between the backend database. Currently IMAP protocol handler is intermixed with database calls and is a bit difficult to understand. Such a proxy will make the IMAP module easier to understand and if necessary, refactoring it further will be much easier. This is outside the scope of an incremental approach of solving thread issues (data races and deadlock avoidance) without any document.

Also, someone might want to work on enhancing the fake imap server. There was a data race issue in compression function in IMAP module, but since the fake server does not support COMPRESSed data exchange, there is no way to test the code path in current imap/test/unit test framework.
Someone might want to work in this direction. Enhancing the imap fakeserver will make writing tests easier.
If someone wants me to work on that, that someone has to pay me, though :-)

The scale of the work so far is much beyond the volunteering contribution.
For example, I had to run TSAN binary under gdb to figure out what was causing the deadlock a few times even, and to see if a quick work around works or not. It has been quite some time since I used gdb in this fashion. Quite refreshing and fun, but I don't want to do that again unless I a m forced to. :-)

In any case, looking at the bug list regarding the imap module, I think the the level of refactoring in this patch has been long overdue.
Since many mail services offer IMAP interface (gmail, for example), I may try to use IMAP in the next year or two. Before doing so, I want imap module to be in a better shape. I have stuck to POP3 until now since the early imap server implementations were buggy and there were undocumented differences. Well, I noticed that there are places where quirks of various IMAP server implementations are mentioned in the source code. It is a wonder that the imap module works as is.

I think the new imap module runs slightly faster because there is less interlocking and more overlap of main and imap threads. and main thread becomes free during testing and ften spins looking for next events to process and that is when background incremental GC kicks in. I now notice more GC-JS data races reported in my test log due to this less tied up MAIN thread. I think the GC-JS race happens only in DEBUG build, but since it occurs in a very low layer of GC, and a TSAN suppression for the GC-JS data race would basically render all other TSAN data races unreported. Tough.

If you have specific questions, please let me know.
I have already started to forget about the early patches WHY/HOW things are done now in some places :-(
The modifications were all necessitated by the found data races and deadlocks, and spin loops through testing.
So I am trying to update the comment as much as possible. Basically, already I am in the maintenance mode of the patch unless new data races or deadlocks are observed.
Creating more test programs to find any more data race issues or deadlocks and updating the comments are top priorities now.

Comment 41

[ISHIKAWA, Chiaki]

I updated the phabricator patch: From the comment there. I fixed a few typos.

---

Latest status

I need to run non-DEBUG version of C-C TB locally for TSAN test.
This is because the frequency of TSAN reports in IMAP module has
decreased to near zero now, and yet garbage collection (GC) and
JavaScript (JS) engine have a few data races in DEBUG binary.
Thus, GC-JS data races get reported more often than IMAP-related data
races (currently 0) during TSAN run of DEBUG version of C-C TB.
This GC-JS data races are so inconvenient for
test analysis. So, I switched to non-DEBUG build.
This has a practical problem. MOZ_ASSERT(), etc. won't trigger in
non-DEBUG version. Thus I am relying on DEBUG build on treeherder to
catch serious issues now.

One data race found on treeherder earlier led to another, and by
adding many tests to exercise the Imap module's new code paths, and stress
testing it, lately during browser tests of mochitests, I believe I
finally caught the LAST (famous last word) bug BEFORE planned phase 2
cleanup in the medium future.
This phase 2 (use of a single better state indicator protected by its own monitor variable) can wait for indefinite time if people find the current
code works acceptably well.

So I am posting this.

The code has gone through various fixes and after so many iterations,
I finally came to create the transition diagram and how various
routines help the imap module's thread to transition to different
states. That alone is a great documentation addition.
See the comment before the definition of ImapThreadMainLoop() for the list of states and transitions.
(NOTE: I have created an ASCII diagram, but somehow forgotten to attach it anywhere.)

I also noticed a bitrot due to the use of new type for message ID and
fixed that.

I want to get this out so that people working on imap module knows
what the clean up is like.

I need to post various test programs and minor update of
IMAPServer.sys.msj, etc. so that I can cover the newly created code
paths, etc.

I suspect the update/addition of test files need to be done
separately. So I leave them out.
(NEWLY ADDED COMMENT: Addition, that is. I do include modification of existing tests that
broke due to invalidated implicit assumptions.)

This is changeset only contains C++ files.

There are more documentation to write. But it may take time and
I may not finish it until the Japanese long holiday week is over in May 7th.

EDIT minor typos fixed. State diagram is not in the change. The list of states and transition ARE explained in a comment block.

Comment 42

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #40)

My approach to solve the thread issues

Basically, my approach was to remove observed data races with proper synchronization.
You can see various data races in non-pached imap module.
See for example, the treeherder job: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=71936f395e97c499b5b9526f529b0ec76d16819b

The artifacts expire in 11 days. Can you generate a new one?
And users will want to download build/target.tar.xz from "Linux tsan Bo"?

Comment 43

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk) from comment #42)

(In reply to ISHIKAWA, Chiaki from comment #40)

My approach to solve the thread issues

Basically, my approach was to remove observed data races with proper synchronization.
You can see various data races in non-pached imap module.
See for example, the treeherder job: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=71936f395e97c499b5b9526f529b0ec76d16819b

The artifacts expire in 11 days. Can you generate a new one?
And users will want to download build/target.tar.xz from "Linux tsan Bo"?

I am trying to create a good binary after finding a few persistent bugs.
The user would like to use debug version from a termianal and when the error is produced, might want to send me the debug log on the screen.
But please wait for a few more days, recent libdbus causes a flurry of TSAN problems (either false positive or real) and I cannot verify that the produced binary is trusable on treeherder (linked against libdubs there).
This libdbus probvlem occurs on my local PC. But I work around it by whitelisting the issue so that TSAN won't complain.Such whitelisting needs to be created by a user of TSAN version under linux.

Yes, in the final form, I would very much like the user under linux to try the TSAN-enabled version with a few environmental variable set because TSAN can only report the data race issues it experiences. Between my local PC and treeherder, I found many cases and eradicated them. But about a month ago, when I thought everything was great on my local PC, I noticed a couple of data races on treeherder, and the efforts to eradicate them led to whole heavy cycle of debugging...
Anyway, I will write a short instruction to use the TSAN binary. DEBUG/OPT binary can be used as is. Setting MOZ_LOG="IMAP:3" would produce important clue to see what goes inside IMAP module.
But don't use the binary yet for your production e-mail exchange YET.

This behemoth of 20+ year technical debt had some surprises that I have not realized. For example, it queues requests, BUT NOBODY explicitly picks the request up. TB basically worked by chance. Until I inserted a loop to pickup processing request before entering idle loop (60 seconds), I saw repeated 60 seconds sleep cycle with no progress.
I remember some people saw LOOONG period of no progress when IMAP was used.
This must be it.
It happens very rarely, but my modification currently done locally produces the situation more frequently, and I found it. I could hardly believe it when I realize there is no explicit PR_NextEvent() done within IMAP thread. After the insertion of explicit PR_NextEvent(), the strange sleep cycle is no longer observed.

Stay tuned.

Comment 44

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #43)

(In reply to Wayne Mery (:wsmwk) from comment #42)

(In reply to ISHIKAWA, Chiaki from comment #40)

My approach to solve the thread issues

Basically, my approach was to remove observed data races with proper synchronization.
You can see various data races in non-pached imap module.
See for example, the treeherder job: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=71936f395e97c499b5b9526f529b0ec76d16819b

The artifacts expire in 11 days. Can you generate a new one?
And users will want to download build/target.tar.xz from "Linux tsan Bo"?

I am trying to create a good binary after finding a few persistent bugs.
The user would like to use debug version from a termianal and when the error is produced, might want to send me the debug log on the screen.
But please wait for a few more days, recent libdbus causes a flurry of TSAN problems (either false positive or real) and I cannot verify that the produced binary is trusable on treeherder (linked against libdubs there).
This libdbus probvlem occurs on my local PC. But I work around it by whitelisting the issue so that TSAN won't complain.Such whitelisting needs to be created by a user of TSAN version under linux.

Yes, in the final form, I would very much like the user under linux to try the TSAN-enabled version with a few environmental variable set because TSAN can only report the data race issues it experiences. Between my local PC and treeherder, I found many cases and eradicated them. But about a month ago, when I thought everything was great on my local PC, I noticed a couple of data races on treeherder, and the efforts to eradicate them led to whole heavy cycle of debugging...
Anyway, I will write a short instruction to use the TSAN binary. DEBUG/OPT binary can be used as is. Setting MOZ_LOG="IMAP:3" would produce important clue to see what goes inside IMAP module.
But don't use the binary yet for your production e-mail exchange YET.

This behemoth of 20+ year technical debt had some surprises that I have not realized. For example, it queues requests, BUT NOBODY explicitly picks the request up. TB basically worked by chance. Until I inserted a loop to pickup processing request before entering idle loop (60 seconds), I saw repeated 60 seconds sleep cycle with no progress.
I remember some people saw LOOONG period of no progress when IMAP was used.
This must be it.
It happens very rarely, but my modification currently done locally produces the situation more frequently, and I found it. I could hardly believe it when I realize there is no explicit PR_NextEvent() done within IMAP thread. After the insertion of explicit PR_NextEvent(), the strange sleep cycle is no longer observed.

Stay tuned.

Comment 45

[Worcester12345]

Please add "Perf" key word for tracking purposes.

This sounds like an enormously great find, and likely at the bottom of many bugs.

Comment 46

[Wayne Mery (:wsmwk)]

Except for possible (rare?) deadlock, I don't think this bug affects performance.

Comment 47

[ISHIKAWA, Chiaki]

I am going to push a threaded series of patches.
I have never done a threaded series of patches and I am not sure
if my commit messages are in correct formats to allow that. So I may
do a few trials and errors. I apologize for the noise it may cause in advance.

Here is a reason why my patches to fix IMAP data race issues became
such a long series of threaded patches.

Stage 1. Initially, I created and consolidated the following patches into a
rather large few patches.

• Major code rewrite to avoid data race and
  problematic IMAP thread's release of use-counted pointers that are
  allocated in main thread. This is C++ code.
• New tests to exercises the changes.
• Modification existing tests to fix problems I found during the
  testing of IMAP changes.

The above so far were more or less based on xpcshell tests and problem analysis.

Stage 2. Then I began noticing issues using mochitest (browser test).
So some fixes to the code and fixes based on the mochitest analysis.
Actually, I found that there are problems in the current mochitest
harness, and in some existing test programs,too. I fixed them as well.
So there are now more patches to IMAP C++ codes, JS codes, etc. that follow the patches from stage 1.

Stage 3. Strangely I noticed a few tests that fail on treeherder, but
not on my local PC at all.

I develop patches under local linux PC.
It turns out Windows is more aggressive in switching threads.
Linux tends to let a thread to occupy the same CPU core and let it run very for a long time without switching.
Windows switches threads more aggressively than linux does, and thus
caused data races that did not occur under local linux.
So, I began local testing under linux using stress-ng to cause more CPU load and context switching pressure.
Using this approach, I found there were still issues in my patches.
AND I found there are existing tests that fail under linux with stress-ng CPU/scheduling pressure.
These were dormant issues and had not been noticed before.
I fixed them, too, except for gloda indexing issues that are too
complicated to grasp after many days' analysis, and since the problem
noticed by a few test programs regarding gloda indexing is an existing
issue BEFORE my IMAP patches, I did not bother to fix it for now.
I verified the gloda issue persists in the pristine C-C TB code WITHOUT my IMAP changes under stress-ng pressure.
Anyway, these fixes for the problems noticed using process-ng added more patches to C++ code, and JS test files.

Stage 4. And finally, there was a couple tests that failed under windows and linux on treeherder, which I confirmed that they failed locally too and the root cause was the result my IMAP change.
The invalid PKI cert was not handled properly. (M-C code's error code about invalid cert was not propagated well and was overwritten by a generic error), and a couple of test programs failed.
It is fixed after analyzing copious traces.

Stage now: Occasionally, new problems were recorded on treeherder
jobs, but they are also present in other people's job submissions,
too.
I am more or less confident that IMAP changes are very stable and do
not cause new test failures on their own now.
I felt it is urgent to let the wider community know the shape of the
changes, and addition/modification of mochitest/xpcshell programs.

In creating the today's final patch series, I tried NOT to consolidate all the C++ changes into one big chunk. Should I?
Unfortunately, most of the stage 1 changes were bundled as C++ changes, JS changes, test changes, etc.
But stage 2, 3, and 4 are not bundled in large chunks because I read
comments that smaller patches are easier to understand.
(However, I have a doubt a big change like IMAP code change may not be
amenable to small chunk approach.)

Anyway, this is why my series of fixes and new addition of test files, etc. became 104 commits (!).

I await for comments regarding

• should I split test file patches?
  and of course, the code changes as well.

Oh, I should hasten to add that

• I noticed that there are duplication of similar comments both in
  commit message and block comments in the code. These
  happened over the course of weeks. I will fix them.
  Actually, this will be much easier to notice in phabricator.

• There ARE memory leaks after some test runs. They ARE
  INTENTIONAL.

Reference-counted pointers that are created in main
thread should NEVER be released in IMAP thread. That can be the cause
of memory corruption and segmentation error which we observe.

So I modified all the known instances of this incorrect release
behavior in IMAP code.: I now dispatch the release action to main
thread. Other modules such as JS and GC follow this pattern for a long
time. I wish IMAP followed this long time ago. Unfortunately, the
main thread may no longer honor such request for release during the
teardown of modules and/or shutdown of thunderbird itself. In that
case, the current code simply lets the release of ref-counted pointers
not happen to avoid memory corruption and segmentation error.

Well, I thought back in April that the patches were in good condition.
But then, mochitest (browser) tests posed a series of issues. Some of them were red-herrings, but anyway I spend weeks to realize the architectural issues.

Then I noticed treeherder reported issues on Treeherder for windows, which I never experinced under my local linux environment.
This exposed the issue of thread scheduling differences between windows and linux, and this caused another month of chasing the issues.
Another month of tracing occasional or persistent test failures now make me more or less confident that the patches are in good order (except for cleaning up comments and removing tracing only patches.)

Since early May, TSAN run on treeherder is useless. So as far as the data race issues go, you have to take my words that I do not observe them under linux.
This is because there is a new linux library that causes data race on its own and there is no way to
whitelist the data race from the library on treeherder. Bug 2038640
AND WORSE, the mach command itself needs modification to allow me to whitelist the data race locally for mochitest. :-( Bug 2031796

Comment 48

[ISHIKAWA, Chiaki]

Actually, I am still using HG MQ to handle local patches. (Maybe the conversion to git is more amenable to the style of HG MQ patch handling.)
I converted HG MQ patches to ordinary HG patches in a new repository to push them using moz-phab using a local script, and the submission may reveal some issues with the script. We will see.

Comment 49

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL. r=#thunderbird-reviewers,mkmelin,BenC

TSAN-010-PATCH-B-idl-nsIImapIncomingServer.patch
C++ only

Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL.
C++ only

Background: In IMAP, Thunderbird maintains a pool of connections to the
mail server. Each connection runs on its own thread. The server object
(nsImapIncomingServer) needs to know whether all connections have truly
finished their work before it can declare itself idle. Previously it
tracked this using m_connectionCache, but m_connectionCache is emptied
before a thread finishes tearing down — leaving a window where the
server appeared idle while threads were still running cleanup code.
This caused test failures and incorrect idle reporting.

These two new IDL methods allow the protocol layer to maintain an
accurate count of live IMAP threads on the server object, independent
of m_connectionCache. A thread increments the counter when it starts
and decrements it only when its last observable effect has completed.

This IDL patch is a prerequisite for the CPP implementation in a later
patch. No existing callers are changed here.

Comment 50

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Fix data races, deadlocks, and lifecycle violations in the Thunderbird IMAP C++ layer. r=#thunderbird-reviewers,mkmelin,BenC

TSAN-020-1956408-CPP.patch
C++ only

Fix data races, deadlocks, and lifecycle violations in the Thunderbird IMAP C++ layer.
C++ only

═══════════════════════════════════════════════════════════════
ARCHITECTURE OVERVIEW
═══════════════════════════════════════════════════════════════

The Thunderbird IMAP implementation is a concurrent system involving
two primary threads:

Main thread: queues URLs (IMAP operations), manages the UI,
receives completion callbacks.
IMAP thread: executes one URL at a time, communicates results
back to the main thread via posted runnables.

The IMAP thread runs a hybrid event loop (ImapThreadMainLoop) with
these logical states:

WAITING — blocked on a monitor, waiting for work or a timeout
CHECK — a signal was received; examining what work is available
RUN_URL — actively executing an IMAP operation
BACKOFF — no runnable work found; short sleep before retrying
IDLE — server IDLE command in progress (server push mode)
SHUTDOWN — death signal received; cleaning up
EXIT — thread has finished

A formal TLA+-style analysis of this state machine, including liveness
proofs and livelock freedom under fairness assumptions, has been
conducted and will be submitted as a companion architecture document.
The key invariants are captured in INVARIANTS.md
(VER-008-fa871ec2-INVARIANTS-rev01.md at time of writing).

═══════════════════════════════════════════════════════════════
WHAT WAS WRONG
═══════════════════════════════════════════════════════════════

ThreadSanitizer (TSAN) analysis of the IMAP module revealed a family
of bugs:

(1) Data races on shared variables accessed from both the main thread
and the IMAP thread without locks. Some of these are fatal: they
cause crashes in production, not just in TSAN builds.

(2) Deadlocks caused by acquiring multiple locks in inconsistent order
across different code paths.

(3) Missed notifications: a URL would be queued and a Notify() signal
sent, but the IMAP thread could miss it due to a split-brain
condition where the signal flag and the URL pointer were not in
the same synchronization domain. The thread would then sleep for
60 seconds before retrying — making operations appear hung.

(4) Reference-counted object destruction on the wrong thread. In
Mozilla's reference counting model, an object created (and its
refcount first set) on the main thread must also be destroyed on
the main thread. Releasing the last reference on the IMAP thread
causes the destructor to run there, which is unsafe and can cause
memory corruption or crash. The fix introduces async teardown
patterns that post the final release back to the main thread.

(5) SetUrlState() not called on all exit paths. Each IMAP URL
operation has a state that must be set to "complete" when the
operation finishes, regardless of success or failure. Missing
this call on certain error paths left callers waiting forever
for a completion signal that never arrived.

There are NO gratuitous changes in this patch. Every change is
directly traceable to a TSAN report, a test failure, or a deadlock
observed during investigation. The fixes were developed incrementally,
with each fix sometimes revealing the next issue.

═══════════════════════════════════════════════════════════════
PER-FILE SUMMARY
═══════════════════════════════════════════════════════════════

nsImapProtocol.cpp / .h (largest change)
m_runningUrl serialization: m_runningUrl is a shared pointer read
from both the IMAP thread and the main thread. All read and write
sites are now guarded under m_imapProtocolMonitor. A
GetSafeRunningImapUrl() accessor provides a locally snapshotted
copy for callers that need to read it without holding a lock across
an operation.

SetUrlState() completeness: ProcessCurrentURL() and AbortPath() now
call SetUrlState(false, rv) in all exit paths including the
null-sink case. Previously the normal completion path skipped this
when sinkForStop was null, leaving the URL's completion Promise
permanently unresolved.

Async teardown: final release of reference-counted objects that
were created on the main thread is now posted back to the main
thread via NS_ProxyRelease, preventing destructor execution on the
IMAP thread.

MaybeDecrementActiveConnections(): an idempotent wrapper that
ensures the active connection counter is decremented exactly once
per URL lifecycle, even when multiple abort paths converge.

nsImapIncomingServer.cpp / .h
LoadNextQueuedUrl() queue-replay loop protected under m_serverLock.
DoomUrlIfChannelHasError() cancels URLs whose mock channel has
already errored before they reach the IMAP thread, preventing
queue stalls after connection failure.
GetImapConnectionAndLoadUrl() restructured to avoid lock ordering
violations with m_serverLock.

nsImapMailFolder.cpp / .h
ParseMsgHdrs benign race: NS_ERROR for "downloading hdrs for hdr
we already have" replaced with MOZ_LOG warning. This is a benign
race between an undo transaction replay on the main thread and IMAP
FETCH delivery on the IMAP thread; the continue was already
correct, only the fatal assertion was wrong.

nsImapFlagAndUidState.cpp / .h
Two-tier locking: m_flagStateLock (leaf lock) protects the primary
UID/flag arrays; mLock protects extended metadata. Eliminates races
between folder synchronization and concurrent flag updates.

nsImapServerResponseParser.cpp / .h
GetSafeRunningImapUrl() used at response parser sites that accessed
m_runningUrl without a snapshot, eliminating use-after-free races.

nsAutoSyncState.cpp / .h
SortQueueBasedOnImportance() made thread-safe.
DownloadMessagesForOffline() gains a re-entrancy guard.
Format string mismatch fixed (companion to nsAutoSyncState-format-fix).

nsImapOfflineSync.cpp
Offline sync move/copy abort: NS_BINDING_ABORTED treated
identically to NS_ERROR_NET_TIMEOUT for partially-executed
operations.

nsImapService.cpp, nsImapUrl.cpp / .h, nsImapUndoTxn.cpp
nsSyncRunnableHelpers.cpp: Various sites updated for thread-safe
URL access and async teardown patterns.

nsMsgDBFolder.cpp, nsMsgDatabase.cpp, nsMsgUtils.h
NS_IOERROR_WARN helper macro for consistent I/O error logging.
Several NS_ERROR calls converted to MOZ_LOG Warning where failure
is recoverable or expected under race conditions.

JS test files previously bundled with this patch have been split into
a companion JS-only patch that follows immediately.

Note: nsImapProtocol.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch (Phase I lifecycle hardening) and
many later patches through TSAN-V2-651. Key functional changes:
TSAN-V2-290: TryToRunUrlLocally fast-path guard
TSAN-V2-500: EstablishServerConnection cert-error preservation
TSAN-V2-570: AbortPath SetUrlState fix
TSAN-V2-571: CreateNewLineFromSocket ordering fix
TSAN-V2-583: AsyncOpen/ReadFromLocalCache part-URL bypass
TSAN-V2-585: TryToRunUrlLocally ownership guard
TSAN-V2-586: ImapThreadMainLoop fast-path ownership guard
TSAN-V2-625: ImapThreadMainLoop m_useIdle race fix
TSAN-V2-651: NotifyMessageFlags part-URL suppression

Note: nsImapIncomingServer.cpp and nsImapIncomingServer.h in this patch
are further modified by TSAN-040-PRE-PHASE-I-CPP.patch and
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase tracking).

Note: nsImapMailFolder.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch, TSAN-V2-589-no-uidplus-append-fix.patch
(CopyFileToOfflineStore key fix), and
TSAN-V2-590-fix-oncopy-guard.patch (OnCopyCompleted guard).

Note: nsImapOfflineSync.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch.

Note: nsImapProtocol.h in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch and
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase enum).

Note: nsImapServerResponseParser.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch.

Comment 51

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Update and annotate existing xpcshell tests (JS/toml only). r=#thunderbird-reviewers,mkmelin,BenC

TSAN-030-1956408-JS.patch
JS only

Update and annotate existing xpcshell tests (JS/toml only).
JS only

Companion to TSAN-020-1956408-CPP.patch. Contains only JS and toml
changes — no C++ modifications.

test_dontStatNoSelect.js:
Rewritten with cleaner structure and inline commentary. Background:
when Thunderbird opens an IMAP folder that is already selected (the
server has already told us the message count), it should not send a
redundant STAT command to ask the server for the count again. STAT
is an older IMAP command that some servers handle slowly. This test
verifies that Thunderbird avoids sending STAT in these situations.
The rewrite makes the intent and assertions clearer.

test_imapAttachmentSaves.js:
Extended with known-pitfalls documentation for future maintainers.
The duplicate const declaration that caused maildir failures is
removed.

test_imapChunks.js:
Adds AppConstants import for platform-conditional skip logic on
configurations where chunked IMAP fetch behaves differently.

test_imapFilterActions.js:
Adds inline commentary documenting the known xpcshell limitation
that SMTP fake daemon delivery is not verified by this test — it
covers only the IMAP side of filter actions.

test_imapStatusCloseDBs.js:
Documents the previous teardown hang and the C++ fix that resolved
it (NS_ProxyRelease for sinks to prevent main-thread join deadlocks).

test_starttlsFailure.js:
Adds a clean-slate guard at setup to prevent state leakage from a
previous test run.

browser.toml:
Adds requesttimeoutfactor=10 for the same reason.

Note: test_imapAttachmentSaves.js in this patch is further modified by
TSAN-110-MJS-changes-and-xpcshell-tests.patch,
TSAN-V2-560-imapAttachmentSaves-waitForCondition.patch (expunge wait), and
TSAN-V2-588-test-hasMsgOffline-wait.patch (hasMsgOffline(2) wait).

Note: test_imapFilterActions.js in this patch is further modified by
TSAN-110-MJS-changes-and-xpcshell-tests.patch and
TSAN-V2-320-fix-imapFilterActions-catchSuppression.patch
(ForwardAsAttachment uncaught rejection fix).

Comment 52

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Implement IncrementActiveConnections/DecrementActiveConnections and harden m_runningUrl access (Phase I CPP). r=#thunderbird-reviewers,mkmelin,BenC

TSAN-040-PRE-PHASE-I-CPP.patch
C++ only

Implement IncrementActiveConnections/DecrementActiveConnections and harden m_runningUrl access (Phase I CPP).
C++ only

This patch implements the IDL additions from PATCH-B-idl-nsIImapIncomingServer
and depends on the C++ restructuring introduced in TSAN-020-1956408-CPP.
It cannot apply cleanly to the original source tree without both predecessors.

═══════════════════════════════════════════════════════════════
ORIGIN AND MOTIVATION
═══════════════════════════════════════════════════════════════

The TSAN performance regression in test_index_messages_imap_online.js
under TSAN instrumentation overhead triggered this investigation.
(In a nutshell, TSAN runtime may cause very large overhead under some circumstances and runtime thread behavior.)
Attempts to reduce that overhead revealed data races in m_runningUrl
access patterns, which in turn exposed the deeper semantic gap described
below. This patch is the result of that investigation — a systematic
rewrite, not a targeted fix.

═══════════════════════════════════════════════════════════════
THE CENTRAL INSIGHT: TEARDOWN DOES NOT BEGIN AT DESTRUCTION
═══════════════════════════════════════════════════════════════

Teardown begins at the first point where the system determines:

"This URL will not complete normally."

This is a semantic transition, not a structural one. The following are
TOO LATE to be considered the start of teardown:

• Destructor execution
• Object deallocation
• Final cleanup routines
• Async cancellation completion

At those points, external observers (e.g. GetAllConnectionsIdle()) have
already observed the system in an incorrect state.

THE THREE-PHASE URL LIFECYCLE MODEL:

Phase 1 — Active execution:
URL lifecycle is active → contributes to activeConnections.
External observers correctly see activity.

Phase 2 — Teardown decision point:
The system determines the URL will not complete normally.
DecrementActiveConnections() MUST happen immediately.
BEFORE any async or cleanup work begins.
External observers must see "not active" from this point forward.

Phase 3 — Post-decision cleanup:
Resource release, socket close, callbacks, destruction.
MUST be invisible to external activity semantics.

RULE:
The moment teardown becomes inevitable, the system must behave as
if the work is already finished from an external perspective.

IMPLEMENTATION REQUIREMENT:
If m_urlLifecycleActive == true at teardown entry:
→ DecrementActiveConnections() MUST happen immediately
→ BEFORE any async or cleanup work begins

RATIONALE:
activeConnections is an externally observed signal of user-visible
work, NOT internal bookkeeping. Teardown is internal-only work and
must not be observable as "active".

FAILURE MODE (what this patch fixes):
teardown starts
→ activeConnections remains > 0
→ GetAllConnectionsIdle() observes "NOT IDLE"
→ tests fail / UI shows spurious activity

This is a semantic violation, NOT a lifecycle bug.

Note: The loose notion of "idle" used by the UI may not match exactly
what the IMAP module defines as "idle". See INVARIANTS.md §3.5 and
§4.6–§4.8 for the full accounting invariants and the teardown semantic
model (VER-025-bdf53bee-INVARIANTS.md at time of writing).

═══════════════════════════════════════════════════════════════
PER-FILE SUMMARY
═══════════════════════════════════════════════════════════════

nsImapIncomingServer.cpp / .h:
Implements IncrementActiveConnections() and DecrementActiveConnections()
using an atomic counter (m_activeConnectionCount). Adds a non-IDL
GetActiveConnectionCount() accessor for diagnostic logging.
GetAllConnectionsIdle() now consults the atomic counter rather than
m_connectionCache alone, giving a correct answer during the teardown
window between TellThreadToDie() and actual thread exit.

nsImapProtocol.cpp / .h:
MaybeDecrementActiveConnections() added as an idempotent wrapper
using an atomic guard (m_urlLifecycleActive) to ensure the decrement
fires exactly once per URL lifecycle, even when multiple abort paths
converge. The guard is the Phase 2 decision point: once it fires,
the connection is no longer externally observable as active.

Systematic m_runningUrl hardening: almost all m_runningUrl access
is now inside proper monitor lock. GetSafeRunningImapUrl() provides
a locally snapshotted copy for callers that need to read it without
holding a lock across an operation.

mPath == nullptr handling: AbortPath now correctly handles the case
where the folder path is null at teardown entry. SetUrlState() is
called in all exit paths.

#if/#endif conditional verbose logging to avoid TSAN overhead on
treeherder runs.

nsImapMailFolder.cpp:
FindUidsToAdd() call site annotated with cross-reference to the
ParseMsgHdrs benign-race analysis in the preceding CPP patch.

nsImapOfflineSync.cpp:
Offline sync abort handling extended to treat NS_BINDING_ABORTED
identically to NS_ERROR_NET_TIMEOUT for partially-executed
move/copy operations.

nsImapServerResponseParser.cpp:
GetSafeRunningImapUrl() used at response parser sites that
previously accessed m_runningUrl without a lock-protected snapshot.
Fixes the unguarded access that caused TSAN data race reports at
these sites. One redundant line removed that independently triggered
a TSAN report.

A variable was accessed using two different monitors in two different
phases. The async conversion caused those phases to overlap, so the
variable is now accessed under a single monitor consistently.

Later, fixed incorrect reversed meaning in MOZ_LOG message in nsImapProtocol.cpp (UI_COMPLETE vs UI_START).
C++ only

In nsImapProtocol.cpp, a MOZ_LOG() printed a message with reversed meaning.

Note: nsImapProtocol.cpp in this patch is further modified by
TSAN-130-treeherder-investigation-CPP.patch and many later patches.
Key functional changes:
TSAN-V2-290: TryToRunUrlLocally fast-path guard
TSAN-V2-500: EstablishServerConnection cert-error preservation
TSAN-V2-570: AbortPath SetUrlState fix
TSAN-V2-571: CreateNewLineFromSocket ordering fix
TSAN-V2-583: AsyncOpen/ReadFromLocalCache part-URL bypass
TSAN-V2-585: TryToRunUrlLocally ownership guard
TSAN-V2-586: ImapThreadMainLoop fast-path ownership guard
TSAN-V2-625: ImapThreadMainLoop m_useIdle race fix
TSAN-V2-651: NotifyMessageFlags part-URL suppression

Note: nsImapIncomingServer.cpp and nsImapIncomingServer.h in this patch
are further modified by TSAN-V2-230-cpp-imap-protocol-fixes.patch
(ConnectionPhase lifecycle tracking).

Note: nsImapMailFolder.cpp in this patch is further modified by
TSAN-V2-589-no-uidplus-append-fix.patch (CopyFileToOfflineStore key fix)
and TSAN-V2-590-fix-oncopy-guard.patch (OnCopyCompleted guard).

Note: nsImapProtocol.h in this patch is further modified by
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase enum).

Comment 53

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL. r=#thunderbird-reviewers,mkmelin,BenC

TSAN-010-PATCH-B-idl-nsIImapIncomingServer.patch
C++ only

Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL.
C++ only

Background: In IMAP, Thunderbird maintains a pool of connections to the
mail server. Each connection runs on its own thread. The server object
(nsImapIncomingServer) needs to know whether all connections have truly
finished their work before it can declare itself idle. Previously it
tracked this using m_connectionCache, but m_connectionCache is emptied
before a thread finishes tearing down — leaving a window where the
server appeared idle while threads were still running cleanup code.
This caused test failures and incorrect idle reporting.

These two new IDL methods allow the protocol layer to maintain an
accurate count of live IMAP threads on the server object, independent
of m_connectionCache. A thread increments the counter when it starts
and decrements it only when its last observable effect has completed.

This IDL patch is a prerequisite for the CPP implementation in a later
patch. No existing callers are changed here.

Comment 54

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL. r=#thunderbird-reviewers,mkmelin,BenC

TSAN-010-PATCH-B-idl-nsIImapIncomingServer.patch
C++ only

Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL.
C++ only

Background: In IMAP, Thunderbird maintains a pool of connections to the
mail server. Each connection runs on its own thread. The server object
(nsImapIncomingServer) needs to know whether all connections have truly
finished their work before it can declare itself idle. Previously it
tracked this using m_connectionCache, but m_connectionCache is emptied
before a thread finishes tearing down — leaving a window where the
server appeared idle while threads were still running cleanup code.
This caused test failures and incorrect idle reporting.

These two new IDL methods allow the protocol layer to maintain an
accurate count of live IMAP threads on the server object, independent
of m_connectionCache. A thread increments the counter when it starts
and decrements it only when its last observable effect has completed.

This IDL patch is a prerequisite for the CPP implementation in a later
patch. No existing callers are changed here.

Comment 55

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Fix data races, deadlocks, and lifecycle violations in the Thunderbird IMAP C++ layer. r=#thunderbird-reviewers,mkmelin,BenC

TSAN-020-1956408-CPP.patch
C++ only

Fix data races, deadlocks, and lifecycle violations in the Thunderbird IMAP C++ layer.
C++ only

═══════════════════════════════════════════════════════════════
ARCHITECTURE OVERVIEW
═══════════════════════════════════════════════════════════════

The Thunderbird IMAP implementation is a concurrent system involving
two primary threads:

Main thread: queues URLs (IMAP operations), manages the UI,
receives completion callbacks.
IMAP thread: executes one URL at a time, communicates results
back to the main thread via posted runnables.

The IMAP thread runs a hybrid event loop (ImapThreadMainLoop) with
these logical states:

WAITING — blocked on a monitor, waiting for work or a timeout
CHECK — a signal was received; examining what work is available
RUN_URL — actively executing an IMAP operation
BACKOFF — no runnable work found; short sleep before retrying
IDLE — server IDLE command in progress (server push mode)
SHUTDOWN — death signal received; cleaning up
EXIT — thread has finished

A formal TLA+-style analysis of this state machine, including liveness
proofs and livelock freedom under fairness assumptions, has been
conducted and will be submitted as a companion architecture document.
The key invariants are captured in INVARIANTS.md
(VER-008-fa871ec2-INVARIANTS-rev01.md at time of writing).

═══════════════════════════════════════════════════════════════
WHAT WAS WRONG
═══════════════════════════════════════════════════════════════

ThreadSanitizer (TSAN) analysis of the IMAP module revealed a family
of bugs:

(1) Data races on shared variables accessed from both the main thread
and the IMAP thread without locks. Some of these are fatal: they
cause crashes in production, not just in TSAN builds.

(2) Deadlocks caused by acquiring multiple locks in inconsistent order
across different code paths.

(3) Missed notifications: a URL would be queued and a Notify() signal
sent, but the IMAP thread could miss it due to a split-brain
condition where the signal flag and the URL pointer were not in
the same synchronization domain. The thread would then sleep for
60 seconds before retrying — making operations appear hung.

(4) Reference-counted object destruction on the wrong thread. In
Mozilla's reference counting model, an object created (and its
refcount first set) on the main thread must also be destroyed on
the main thread. Releasing the last reference on the IMAP thread
causes the destructor to run there, which is unsafe and can cause
memory corruption or crash. The fix introduces async teardown
patterns that post the final release back to the main thread.

(5) SetUrlState() not called on all exit paths. Each IMAP URL
operation has a state that must be set to "complete" when the
operation finishes, regardless of success or failure. Missing
this call on certain error paths left callers waiting forever
for a completion signal that never arrived.

There are NO gratuitous changes in this patch. Every change is
directly traceable to a TSAN report, a test failure, or a deadlock
observed during investigation. The fixes were developed incrementally,
with each fix sometimes revealing the next issue.

═══════════════════════════════════════════════════════════════
PER-FILE SUMMARY
═══════════════════════════════════════════════════════════════

nsImapProtocol.cpp / .h (largest change)
m_runningUrl serialization: m_runningUrl is a shared pointer read
from both the IMAP thread and the main thread. All read and write
sites are now guarded under m_imapProtocolMonitor. A
GetSafeRunningImapUrl() accessor provides a locally snapshotted
copy for callers that need to read it without holding a lock across
an operation.

SetUrlState() completeness: ProcessCurrentURL() and AbortPath() now
call SetUrlState(false, rv) in all exit paths including the
null-sink case. Previously the normal completion path skipped this
when sinkForStop was null, leaving the URL's completion Promise
permanently unresolved.

Async teardown: final release of reference-counted objects that
were created on the main thread is now posted back to the main
thread via NS_ProxyRelease, preventing destructor execution on the
IMAP thread.

MaybeDecrementActiveConnections(): an idempotent wrapper that
ensures the active connection counter is decremented exactly once
per URL lifecycle, even when multiple abort paths converge.

nsImapIncomingServer.cpp / .h
LoadNextQueuedUrl() queue-replay loop protected under m_serverLock.
DoomUrlIfChannelHasError() cancels URLs whose mock channel has
already errored before they reach the IMAP thread, preventing
queue stalls after connection failure.
GetImapConnectionAndLoadUrl() restructured to avoid lock ordering
violations with m_serverLock.

nsImapMailFolder.cpp / .h
ParseMsgHdrs benign race: NS_ERROR for "downloading hdrs for hdr
we already have" replaced with MOZ_LOG warning. This is a benign
race between an undo transaction replay on the main thread and IMAP
FETCH delivery on the IMAP thread; the continue was already
correct, only the fatal assertion was wrong.

nsImapFlagAndUidState.cpp / .h
Two-tier locking: m_flagStateLock (leaf lock) protects the primary
UID/flag arrays; mLock protects extended metadata. Eliminates races
between folder synchronization and concurrent flag updates.

nsImapServerResponseParser.cpp / .h
GetSafeRunningImapUrl() used at response parser sites that accessed
m_runningUrl without a snapshot, eliminating use-after-free races.

nsAutoSyncState.cpp / .h
SortQueueBasedOnImportance() made thread-safe.
DownloadMessagesForOffline() gains a re-entrancy guard.
Format string mismatch fixed (companion to nsAutoSyncState-format-fix).

nsImapOfflineSync.cpp
Offline sync move/copy abort: NS_BINDING_ABORTED treated
identically to NS_ERROR_NET_TIMEOUT for partially-executed
operations.

nsImapService.cpp, nsImapUrl.cpp / .h, nsImapUndoTxn.cpp
nsSyncRunnableHelpers.cpp: Various sites updated for thread-safe
URL access and async teardown patterns.

nsMsgDBFolder.cpp, nsMsgDatabase.cpp, nsMsgUtils.h
NS_IOERROR_WARN helper macro for consistent I/O error logging.
Several NS_ERROR calls converted to MOZ_LOG Warning where failure
is recoverable or expected under race conditions.

JS test files previously bundled with this patch have been split into
a companion JS-only patch that follows immediately.

Note: nsImapProtocol.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch (Phase I lifecycle hardening) and
many later patches through TSAN-V2-651. Key functional changes:
TSAN-V2-290: TryToRunUrlLocally fast-path guard
TSAN-V2-500: EstablishServerConnection cert-error preservation
TSAN-V2-570: AbortPath SetUrlState fix
TSAN-V2-571: CreateNewLineFromSocket ordering fix
TSAN-V2-583: AsyncOpen/ReadFromLocalCache part-URL bypass
TSAN-V2-585: TryToRunUrlLocally ownership guard
TSAN-V2-586: ImapThreadMainLoop fast-path ownership guard
TSAN-V2-625: ImapThreadMainLoop m_useIdle race fix
TSAN-V2-651: NotifyMessageFlags part-URL suppression

Note: nsImapIncomingServer.cpp and nsImapIncomingServer.h in this patch
are further modified by TSAN-040-PRE-PHASE-I-CPP.patch and
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase tracking).

Note: nsImapMailFolder.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch, TSAN-V2-589-no-uidplus-append-fix.patch
(CopyFileToOfflineStore key fix), and
TSAN-V2-590-fix-oncopy-guard.patch (OnCopyCompleted guard).

Note: nsImapOfflineSync.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch.

Note: nsImapProtocol.h in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch and
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase enum).

Note: nsImapServerResponseParser.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch.

Comment 56

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Update and annotate existing xpcshell tests (JS/toml only). r=#thunderbird-reviewers,mkmelin,BenC

TSAN-030-1956408-JS.patch
JS only

Update and annotate existing xpcshell tests (JS/toml only).
JS only

Companion to TSAN-020-1956408-CPP.patch. Contains only JS and toml
changes — no C++ modifications.

test_dontStatNoSelect.js:
Rewritten with cleaner structure and inline commentary. Background:
when Thunderbird opens an IMAP folder that is already selected (the
server has already told us the message count), it should not send a
redundant STAT command to ask the server for the count again. STAT
is an older IMAP command that some servers handle slowly. This test
verifies that Thunderbird avoids sending STAT in these situations.
The rewrite makes the intent and assertions clearer.

test_imapAttachmentSaves.js:
Extended with known-pitfalls documentation for future maintainers.
The duplicate const declaration that caused maildir failures is
removed.

test_imapChunks.js:
Adds AppConstants import for platform-conditional skip logic on
configurations where chunked IMAP fetch behaves differently.

test_imapFilterActions.js:
Adds inline commentary documenting the known xpcshell limitation
that SMTP fake daemon delivery is not verified by this test — it
covers only the IMAP side of filter actions.

test_imapStatusCloseDBs.js:
Documents the previous teardown hang and the C++ fix that resolved
it (NS_ProxyRelease for sinks to prevent main-thread join deadlocks).

test_starttlsFailure.js:
Adds a clean-slate guard at setup to prevent state leakage from a
previous test run.

browser.toml:
Adds requesttimeoutfactor=10 for the same reason.

Note: test_imapAttachmentSaves.js in this patch is further modified by
TSAN-110-MJS-changes-and-xpcshell-tests.patch,
TSAN-V2-560-imapAttachmentSaves-waitForCondition.patch (expunge wait), and
TSAN-V2-588-test-hasMsgOffline-wait.patch (hasMsgOffline(2) wait).

Note: test_imapFilterActions.js in this patch is further modified by
TSAN-110-MJS-changes-and-xpcshell-tests.patch and
TSAN-V2-320-fix-imapFilterActions-catchSuppression.patch
(ForwardAsAttachment uncaught rejection fix).

Comment 57

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Implement IncrementActiveConnections/DecrementActiveConnections and harden m_runningUrl access (Phase I CPP). r=#thunderbird-reviewers,mkmelin,BenC

TSAN-040-PRE-PHASE-I-CPP.patch
C++ only

Implement IncrementActiveConnections/DecrementActiveConnections and harden m_runningUrl access (Phase I CPP).
C++ only

This patch implements the IDL additions from PATCH-B-idl-nsIImapIncomingServer
and depends on the C++ restructuring introduced in TSAN-020-1956408-CPP.
It cannot apply cleanly to the original source tree without both predecessors.

═══════════════════════════════════════════════════════════════
ORIGIN AND MOTIVATION
═══════════════════════════════════════════════════════════════

The TSAN performance regression in test_index_messages_imap_online.js
under TSAN instrumentation overhead triggered this investigation.
(In a nutshell, TSAN runtime may cause very large overhead under some circumstances and runtime thread behavior.)
Attempts to reduce that overhead revealed data races in m_runningUrl
access patterns, which in turn exposed the deeper semantic gap described
below. This patch is the result of that investigation — a systematic
rewrite, not a targeted fix.

═══════════════════════════════════════════════════════════════
THE CENTRAL INSIGHT: TEARDOWN DOES NOT BEGIN AT DESTRUCTION
═══════════════════════════════════════════════════════════════

Teardown begins at the first point where the system determines:

"This URL will not complete normally."

This is a semantic transition, not a structural one. The following are
TOO LATE to be considered the start of teardown:

• Destructor execution
• Object deallocation
• Final cleanup routines
• Async cancellation completion

At those points, external observers (e.g. GetAllConnectionsIdle()) have
already observed the system in an incorrect state.

THE THREE-PHASE URL LIFECYCLE MODEL:

Phase 1 — Active execution:
URL lifecycle is active → contributes to activeConnections.
External observers correctly see activity.

Phase 2 — Teardown decision point:
The system determines the URL will not complete normally.
DecrementActiveConnections() MUST happen immediately.
BEFORE any async or cleanup work begins.
External observers must see "not active" from this point forward.

Phase 3 — Post-decision cleanup:
Resource release, socket close, callbacks, destruction.
MUST be invisible to external activity semantics.

RULE:
The moment teardown becomes inevitable, the system must behave as
if the work is already finished from an external perspective.

IMPLEMENTATION REQUIREMENT:
If m_urlLifecycleActive == true at teardown entry:
→ DecrementActiveConnections() MUST happen immediately
→ BEFORE any async or cleanup work begins

RATIONALE:
activeConnections is an externally observed signal of user-visible
work, NOT internal bookkeeping. Teardown is internal-only work and
must not be observable as "active".

FAILURE MODE (what this patch fixes):
teardown starts
→ activeConnections remains > 0
→ GetAllConnectionsIdle() observes "NOT IDLE"
→ tests fail / UI shows spurious activity

This is a semantic violation, NOT a lifecycle bug.

Note: The loose notion of "idle" used by the UI may not match exactly
what the IMAP module defines as "idle". See INVARIANTS.md §3.5 and
§4.6–§4.8 for the full accounting invariants and the teardown semantic
model (VER-025-bdf53bee-INVARIANTS.md at time of writing).

═══════════════════════════════════════════════════════════════
PER-FILE SUMMARY
═══════════════════════════════════════════════════════════════

nsImapIncomingServer.cpp / .h:
Implements IncrementActiveConnections() and DecrementActiveConnections()
using an atomic counter (m_activeConnectionCount). Adds a non-IDL
GetActiveConnectionCount() accessor for diagnostic logging.
GetAllConnectionsIdle() now consults the atomic counter rather than
m_connectionCache alone, giving a correct answer during the teardown
window between TellThreadToDie() and actual thread exit.

nsImapProtocol.cpp / .h:
MaybeDecrementActiveConnections() added as an idempotent wrapper
using an atomic guard (m_urlLifecycleActive) to ensure the decrement
fires exactly once per URL lifecycle, even when multiple abort paths
converge. The guard is the Phase 2 decision point: once it fires,
the connection is no longer externally observable as active.

Systematic m_runningUrl hardening: almost all m_runningUrl access
is now inside proper monitor lock. GetSafeRunningImapUrl() provides
a locally snapshotted copy for callers that need to read it without
holding a lock across an operation.

mPath == nullptr handling: AbortPath now correctly handles the case
where the folder path is null at teardown entry. SetUrlState() is
called in all exit paths.

#if/#endif conditional verbose logging to avoid TSAN overhead on
treeherder runs.

nsImapMailFolder.cpp:
FindUidsToAdd() call site annotated with cross-reference to the
ParseMsgHdrs benign-race analysis in the preceding CPP patch.

nsImapOfflineSync.cpp:
Offline sync abort handling extended to treat NS_BINDING_ABORTED
identically to NS_ERROR_NET_TIMEOUT for partially-executed
move/copy operations.

nsImapServerResponseParser.cpp:
GetSafeRunningImapUrl() used at response parser sites that
previously accessed m_runningUrl without a lock-protected snapshot.
Fixes the unguarded access that caused TSAN data race reports at
these sites. One redundant line removed that independently triggered
a TSAN report.

A variable was accessed using two different monitors in two different
phases. The async conversion caused those phases to overlap, so the
variable is now accessed under a single monitor consistently.

Later, fixed incorrect reversed meaning in MOZ_LOG message in nsImapProtocol.cpp (UI_COMPLETE vs UI_START).
C++ only

In nsImapProtocol.cpp, a MOZ_LOG() printed a message with reversed meaning.

Note: nsImapProtocol.cpp in this patch is further modified by
TSAN-130-treeherder-investigation-CPP.patch and many later patches.
Key functional changes:
TSAN-V2-290: TryToRunUrlLocally fast-path guard
TSAN-V2-500: EstablishServerConnection cert-error preservation
TSAN-V2-570: AbortPath SetUrlState fix
TSAN-V2-571: CreateNewLineFromSocket ordering fix
TSAN-V2-583: AsyncOpen/ReadFromLocalCache part-URL bypass
TSAN-V2-585: TryToRunUrlLocally ownership guard
TSAN-V2-586: ImapThreadMainLoop fast-path ownership guard
TSAN-V2-625: ImapThreadMainLoop m_useIdle race fix
TSAN-V2-651: NotifyMessageFlags part-URL suppression

Note: nsImapIncomingServer.cpp and nsImapIncomingServer.h in this patch
are further modified by TSAN-V2-230-cpp-imap-protocol-fixes.patch
(ConnectionPhase lifecycle tracking).

Note: nsImapMailFolder.cpp in this patch is further modified by
TSAN-V2-589-no-uidplus-append-fix.patch (CopyFileToOfflineStore key fix)
and TSAN-V2-590-fix-oncopy-guard.patch (OnCopyCompleted guard).

Note: nsImapProtocol.h in this patch is further modified by
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase enum).

Comment 58

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - cast nsresult into (unsigned int) for %x specifier. r=#thunderbird-reviewers,mkmelin,BenC

Preamble for IMAP data race issue removal.

Comment 59

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL. r=#thunderbird-reviewers,mkmelin,BenC

TSAN-010-PATCH-B-idl-nsIImapIncomingServer.patch
C++ only

Add IncrementActiveConnections/DecrementActiveConnections to nsIImapIncomingServer IDL.
C++ only

Background: In IMAP, Thunderbird maintains a pool of connections to the
mail server. Each connection runs on its own thread. The server object
(nsImapIncomingServer) needs to know whether all connections have truly
finished their work before it can declare itself idle. Previously it
tracked this using m_connectionCache, but m_connectionCache is emptied
before a thread finishes tearing down — leaving a window where the
server appeared idle while threads were still running cleanup code.
This caused test failures and incorrect idle reporting.

These two new IDL methods allow the protocol layer to maintain an
accurate count of live IMAP threads on the server object, independent
of m_connectionCache. A thread increments the counter when it starts
and decrements it only when its last observable effect has completed.

This IDL patch is a prerequisite for the CPP implementation in a later
patch. No existing callers are changed here.

Comment 60

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Fix data races, deadlocks, and lifecycle violations in the Thunderbird IMAP C++ layer. r=#thunderbird-reviewers,mkmelin,BenC

TSAN-020-1956408-CPP.patch
C++ only

Fix data races, deadlocks, and lifecycle violations in the Thunderbird IMAP C++ layer.
C++ only

═══════════════════════════════════════════════════════════════
ARCHITECTURE OVERVIEW
═══════════════════════════════════════════════════════════════

The Thunderbird IMAP implementation is a concurrent system involving
two primary threads:

Main thread: queues URLs (IMAP operations), manages the UI,
receives completion callbacks.
IMAP thread: executes one URL at a time, communicates results
back to the main thread via posted runnables.

The IMAP thread runs a hybrid event loop (ImapThreadMainLoop) with
these logical states:

WAITING — blocked on a monitor, waiting for work or a timeout
CHECK — a signal was received; examining what work is available
RUN_URL — actively executing an IMAP operation
BACKOFF — no runnable work found; short sleep before retrying
IDLE — server IDLE command in progress (server push mode)
SHUTDOWN — death signal received; cleaning up
EXIT — thread has finished

A formal TLA+-style analysis of this state machine, including liveness
proofs and livelock freedom under fairness assumptions, has been
conducted and will be submitted as a companion architecture document.
The key invariants are captured in INVARIANTS.md
(VER-008-fa871ec2-INVARIANTS-rev01.md at time of writing).

═══════════════════════════════════════════════════════════════
WHAT WAS WRONG
═══════════════════════════════════════════════════════════════

ThreadSanitizer (TSAN) analysis of the IMAP module revealed a family
of bugs:

(1) Data races on shared variables accessed from both the main thread
and the IMAP thread without locks. Some of these are fatal: they
cause crashes in production, not just in TSAN builds.

(2) Deadlocks caused by acquiring multiple locks in inconsistent order
across different code paths.

(3) Missed notifications: a URL would be queued and a Notify() signal
sent, but the IMAP thread could miss it due to a split-brain
condition where the signal flag and the URL pointer were not in
the same synchronization domain. The thread would then sleep for
60 seconds before retrying — making operations appear hung.

(4) Reference-counted object destruction on the wrong thread. In
Mozilla's reference counting model, an object created (and its
refcount first set) on the main thread must also be destroyed on
the main thread. Releasing the last reference on the IMAP thread
causes the destructor to run there, which is unsafe and can cause
memory corruption or crash. The fix introduces async teardown
patterns that post the final release back to the main thread.

(5) SetUrlState() not called on all exit paths. Each IMAP URL
operation has a state that must be set to "complete" when the
operation finishes, regardless of success or failure. Missing
this call on certain error paths left callers waiting forever
for a completion signal that never arrived.

There are NO gratuitous changes in this patch. Every change is
directly traceable to a TSAN report, a test failure, or a deadlock
observed during investigation. The fixes were developed incrementally,
with each fix sometimes revealing the next issue.

═══════════════════════════════════════════════════════════════
PER-FILE SUMMARY
═══════════════════════════════════════════════════════════════

nsImapProtocol.cpp / .h (largest change)
m_runningUrl serialization: m_runningUrl is a shared pointer read
from both the IMAP thread and the main thread. All read and write
sites are now guarded under m_imapProtocolMonitor. A
GetSafeRunningImapUrl() accessor provides a locally snapshotted
copy for callers that need to read it without holding a lock across
an operation.

SetUrlState() completeness: ProcessCurrentURL() and AbortPath() now
call SetUrlState(false, rv) in all exit paths including the
null-sink case. Previously the normal completion path skipped this
when sinkForStop was null, leaving the URL's completion Promise
permanently unresolved.

Async teardown: final release of reference-counted objects that
were created on the main thread is now posted back to the main
thread via NS_ProxyRelease, preventing destructor execution on the
IMAP thread.

MaybeDecrementActiveConnections(): an idempotent wrapper that
ensures the active connection counter is decremented exactly once
per URL lifecycle, even when multiple abort paths converge.

nsImapIncomingServer.cpp / .h
LoadNextQueuedUrl() queue-replay loop protected under m_serverLock.
DoomUrlIfChannelHasError() cancels URLs whose mock channel has
already errored before they reach the IMAP thread, preventing
queue stalls after connection failure.
GetImapConnectionAndLoadUrl() restructured to avoid lock ordering
violations with m_serverLock.

nsImapMailFolder.cpp / .h
ParseMsgHdrs benign race: NS_ERROR for "downloading hdrs for hdr
we already have" replaced with MOZ_LOG warning. This is a benign
race between an undo transaction replay on the main thread and IMAP
FETCH delivery on the IMAP thread; the continue was already
correct, only the fatal assertion was wrong.

nsImapFlagAndUidState.cpp / .h
Two-tier locking: m_flagStateLock (leaf lock) protects the primary
UID/flag arrays; mLock protects extended metadata. Eliminates races
between folder synchronization and concurrent flag updates.

nsImapServerResponseParser.cpp / .h
GetSafeRunningImapUrl() used at response parser sites that accessed
m_runningUrl without a snapshot, eliminating use-after-free races.

nsAutoSyncState.cpp / .h
SortQueueBasedOnImportance() made thread-safe.
DownloadMessagesForOffline() gains a re-entrancy guard.
Format string mismatch fixed (companion to nsAutoSyncState-format-fix).

nsImapOfflineSync.cpp
Offline sync move/copy abort: NS_BINDING_ABORTED treated
identically to NS_ERROR_NET_TIMEOUT for partially-executed
operations.

nsImapService.cpp, nsImapUrl.cpp / .h, nsImapUndoTxn.cpp
nsSyncRunnableHelpers.cpp: Various sites updated for thread-safe
URL access and async teardown patterns.

nsMsgDBFolder.cpp, nsMsgDatabase.cpp, nsMsgUtils.h
NS_IOERROR_WARN helper macro for consistent I/O error logging.
Several NS_ERROR calls converted to MOZ_LOG Warning where failure
is recoverable or expected under race conditions.

JS test files previously bundled with this patch have been split into
a companion JS-only patch that follows immediately.

Note: nsImapProtocol.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch (Phase I lifecycle hardening) and
many later patches through TSAN-V2-651. Key functional changes:
TSAN-V2-290: TryToRunUrlLocally fast-path guard
TSAN-V2-500: EstablishServerConnection cert-error preservation
TSAN-V2-570: AbortPath SetUrlState fix
TSAN-V2-571: CreateNewLineFromSocket ordering fix
TSAN-V2-583: AsyncOpen/ReadFromLocalCache part-URL bypass
TSAN-V2-585: TryToRunUrlLocally ownership guard
TSAN-V2-586: ImapThreadMainLoop fast-path ownership guard
TSAN-V2-625: ImapThreadMainLoop m_useIdle race fix
TSAN-V2-651: NotifyMessageFlags part-URL suppression

Note: nsImapIncomingServer.cpp and nsImapIncomingServer.h in this patch
are further modified by TSAN-040-PRE-PHASE-I-CPP.patch and
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase tracking).

Note: nsImapMailFolder.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch, TSAN-V2-589-no-uidplus-append-fix.patch
(CopyFileToOfflineStore key fix), and
TSAN-V2-590-fix-oncopy-guard.patch (OnCopyCompleted guard).

Note: nsImapOfflineSync.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch.

Note: nsImapProtocol.h in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch and
TSAN-V2-230-cpp-imap-protocol-fixes.patch (ConnectionPhase enum).

Note: nsImapServerResponseParser.cpp in this patch is further modified by
TSAN-040-PRE-PHASE-I-CPP.patch.

Comment 61

[ISHIKAWA, Chiaki]

Attachment: Bug 1956408 - Update and annotate existing xpcshell tests (JS/toml only). r=#thunderbird-reviewers,mkmelin,BenC

TSAN-030-1956408-JS.patch
JS only

Update and annotate existing xpcshell tests (JS/toml only).
JS only

Companion to TSAN-020-1956408-CPP.patch. Contains only JS and toml
changes — no C++ modifications.

test_dontStatNoSelect.js:
Rewritten with cleaner structure and inline commentary. Background:
when Thunderbird opens an IMAP folder that is already selected (the
server has already told us the message count), it should not send a
redundant STAT command to ask the server for the count again. STAT
is an older IMAP command that some servers handle slowly. This test
verifies that Thunderbird avoids sending STAT in these situations.
The rewrite makes the intent and assertions clearer.

test_imapAttachmentSaves.js:
Extended with known-pitfalls documentation for future maintainers.
The duplicate const declaration that caused maildir failures is
removed.

test_imapChunks.js:
Adds AppConstants import for platform-conditional skip logic on
configurations where chunked IMAP fetch behaves differently.

test_imapFilterActions.js:
Adds inline commentary documenting the known xpcshell limitation
that SMTP fake daemon delivery is not verified by this test — it
covers only the IMAP side of filter actions.

test_imapStatusCloseDBs.js:
Documents the previous teardown hang and the C++ fix that resolved
it (NS_ProxyRelease for sinks to prevent main-thread join deadlocks).

test_starttlsFailure.js:
Adds a clean-slate guard at setup to prevent state leakage from a
previous test run.

browser.toml:
Adds requesttimeoutfactor=10 for the same reason.

Note: test_imapAttachmentSaves.js in this patch is further modified by
TSAN-110-MJS-changes-and-xpcshell-tests.patch,
TSAN-V2-560-imapAttachmentSaves-waitForCondition.patch (expunge wait), and
TSAN-V2-588-test-hasMsgOffline-wait.patch (hasMsgOffline(2) wait).

Note: test_imapFilterActions.js in this patch is further modified by
TSAN-110-MJS-changes-and-xpcshell-tests.patch and
TSAN-V2-320-fix-imapFilterActions-catchSuppression.patch
(ForwardAsAttachment uncaught rejection fix).

Comment 62

[ISHIKAWA, Chiaki]

Agha.

moz-phab submit failed after a few patches were uploaded.
Error message:

You didn't specify a valid command, so we ran submit for you, and it failed.
ValueError: server not connected
Run moz-phab again with '--trace' to show debugging output
Sentry is attempting to send 2 pending events
Waiting up to 2 seconds
Press Ctrl-C to quit

I re-tried still failure, and I realize there are intermediate upload of duplicated patches.
I tried to "abandon" them here, but in my haste, I am afraid that I deleted the very first reviewed patch discussion :-(
I HAVE ADDRESSED all the comments there, though.

I have submitted my HG MQ changes to treeherder successfully, so I must have done something incorrectly when I created new HG repository that has my changes as proper HG patchsets. (Maybe deleting a few local HG MQ patches to avoid strict GCC compile time check may have played funnily? But that is hard to be the root cause of moz-phab failure since the patchsets from HG MQ patches were successfully created with no conflict or merge error as far as I could see.

I will see if breaking down the upload into smaller sets will help, but the failure occurred only after five or so patches. Hmm...
Any ideas?

EDIT: a typo fixed.
I am now pondering to create an HG repository from an ephemeral HG repository created for treeherder submission.
Also, "Server not connected" suggests that server is not responding. Maybe a patch was large and the server was not responding in a short time.
The big patch I created for stage 1 may not play nicely. Considering splitting them into smaller chunks.

Comment 63

[Wayne Mery (:wsmwk)]

I will see if breaking down the upload into smaller sets will help, but the failure occurred only after five or so patches. Hmm...
Any ideas?

I seem to recall that phab is occasionally slow. Is that true?

Comment 64

[Wayne Mery (:wsmwk)]

I will see if breaking down the upload into smaller sets will help, but the failure occurred only after five or so patches. Hmm...

Chiaki, did that help?

Comment 65

[Toby Pilling [:tobyp]]

It's true that Phab sometimes struggles. If you have thing locally, we can export the diffs and bring in to the git repository or use tools to move them over. Let me know if you need any help with that - I think Ben moved a number of his over without any issues.