1931863 - [tsan] perma comm/mailnews/base/test/unit/test_folderCompact2.js | ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:397:37 in Length

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Treeherder Bug Filer]

Filed by: mkmelin [at] iki.fi
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=483104043&repo=comm-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/KcpdKawwS0uy4E_SyKq4iA/runs/0/artifacts/public/logs/live_backing.log

---

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: Bug 1931863 - Fix [tsan] perma comm/mailnews/base/test/unit/test_folderCompact2.js | ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:397:37 in Length. r=BenC,#thunderbird-reviewers

Comment 2

[Intermittent Failures Robot]

1 failures in 3002 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Platform and build breakdown:

• linux1804-64-tsan-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1931863&startday=2024-11-18&endday=2024-11-24&tree=all

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: Bug 1931863 - V2: Fix [tsan] perma comm/mailnews/base/test/unit/test_folderCompact2.js | ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:397:37 in Length. r=BenC

Comment 4

[Kai Engert [:KaiE:]]

Wow. This is an incedibly useful report.

I think we should investigate this concurrency more deeply.

Comment 5

[Kai Engert [:KaiE:]]

I'd prefer to implement more complete protection of member variables before committing.

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Bug 1931863 - Additional locking for MboxMsgInputStream. r=mkmelin

Comment 7

[Ben Campbell]

OK, I've managed to catch a concrete case of the asyncpump causing reentrancy to MboxParser->Feed(), so this isn't a speculative issue any more.
(I just added a bool member var to MboxParser which I set while inside Feed(), and asserted if it was already set at the start).

I think Kai's patch is the way to go (and should probably be uplifted, once landed).

I think Bug 1917447 might be down to this, but I'm still following up.
That bug deals with NNTP and messages with large, mime-encoded attachments. My theory is that line lengths on such NNTP messages tends to be longer than for email messages, and that difference causes enough change in timing for the STS and main thread to step on each other's toes.
Bug 1917447 tends to cause a hard crash, which is kind of comforting - less chance for unnoticed weirdness.
Anyway, I'll test this patch against that bug and report back.

Comment 8

[Ben Campbell]

Reporting back: Kai's patch seemed to fix the crashing in Bug 1917447 - hooray!
(I'll add my STR over there, but I suspect you could replicate it on a local folder (rather than NNTP) with some fake emails with big attachments and long line lengths)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/564a0a9d4682
V2: Fix [tsan] perma comm/mailnews/base/test/unit/test_folderCompact2.js | ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:397:37 in Length. r=BenC
https://hg.mozilla.org/comm-central/rev/e64e5d561ed8
Additional locking for MboxMsgInputStream. r=mkmelin

Comment 11

[Ben Campbell]

Attachment: Bug 1931863 - Remove mozilla namespace pollution from MboxMsgInputStream header. r=#thunderbird-back-end-reviewers

Prevent namespace pollution as per
https://firefox-source-docs.mozilla.org/code-quality/coding-style/coding_style_cpp.html#c-namespaces

Also minor tweak to remove locking in the constructor (no other thread can access the object during construction).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8513f49ad8b0
Remove mozilla namespace pollution from MboxMsgInputStream header. r=kaie,mkmelin

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9444263 [details]
Bug 1931863 - V2: Fix [tsan] perma comm/mailnews/base/test/unit/test_folderCompact2.js | ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:397:37 in Length. r=BenC

[Approval Request Comment] (all the 3 patches)
User impact if declined: unexpected behavior, and potential crash
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): not risk free, so good to bake on beta before potential uplift

Comment 14

[Corey Bryant]

Comment on attachment 9444263 [details]
Bug 1931863 - V2: Fix [tsan] perma comm/mailnews/base/test/unit/test_folderCompact2.js | ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsTArray.h:397:37 in Length. r=BenC

[Triage Comment]
Approved for beta
Let's keep an eye on these in beta as release of 135.0 in 3 weeks is slated to be the default download on tb.net.

Comment 15

[Corey Bryant]

Comment on attachment 9446026 [details]
Bug 1931863 - Additional locking for MboxMsgInputStream. r=mkmelin

[Triage Comment]
Approved for beta

Comment 16

[Corey Bryant]

Comment on attachment 9446403 [details]
Bug 1931863 - Remove mozilla namespace pollution from MboxMsgInputStream header. r=#thunderbird-back-end-reviewers

[Triage Comment]
Approved for beta

Comment 17

[Daniel Darnell [:dandarnell]]

Thunderbird 135.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/e8beff5ec365
https://hg.mozilla.org/releases/comm-beta/rev/6951606f38e4
https://hg.mozilla.org/releases/comm-beta/rev/7b98ec62cb39