1760353 - bustage: /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapProtocol.cpp:4495:7: error: mutex 'm_waitForBodyIdsMonitor' is not held on every path through here [-Werror,-Wthread-safety-analysis]

Status: RESOLVED FIXED

(Thunderbird :: Upstream Synchronization, defect, P5)

Description

[Treeherder Bug Filer]

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

---

Comment 1

[Magnus Melin [:mkmelin]]

See https://firefox-source-docs.mozilla.org/xpcom/thread-safety.html (== https://searchfox.org/mozilla-central/source/xpcom/docs/thread-safety.rst)

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dbccf34ef270
Fix m_waitForBodyIdsMonitor issue after bug 1759501. r=rjl

Comment 3

[Intermittent Failures Robot]

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

Repository breakdown:

• comm-central: 1

Platform and build breakdown:

• linux32: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1760353&startday=2022-03-14&endday=2022-03-20&tree=all

Comment 4

[Magnus Melin [:mkmelin]]

Ben, can you check how we should fix this properly?

Comment 5

[Geoff Lankow (:darktrojan)]

This is probably relevant: https://groups.google.com/a/mozilla.org/g/dev-platform/c/FALiZRSJUYg

Comment 6

[Ben Campbell]

Hmm... I think the original code was OK.
Maybe the static analysis was getting confused by the entered_waitForBodyIdsMonitor flag? If so, I'm a little surprised it'd trip up on something so straightforward... maybe there was some subtle complication- the flag being set after the Enter() call, perhaps?

In any case, it was a little shonky, so I've rearranged things to ditch the flag altogether.

try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0ffb150e743649110843d3d5f0d22cf042106e32

I assume that does the static analysis? If so, and it works, I'll submit it via phab.
What is the magic incantation to run a static analysis locally? I'm sure I've done that in the past, but it eludes me now...

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Ben Campbell from comment #6)

What is the magic incantation to run a static analysis locally? I'm sure I've done that in the past, but it eludes me now...

AFAIK, if you have ac_add_options --enable-warnings-as-errors in your .mozconfig many (all?) of the static analysis error will show up locally when compiling

Other than that, see docs at https://firefox-source-docs.mozilla.org/code-quality/static-analysis/existing.html

Comment 8

[Ben Campbell]

Attachment: Bug 1760353 - Fix possible bogus release of un-held waitForBodyIdsMonitor object in IMAP. r=mkmelin

Comment 9

[Ben Campbell]

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

AFAIK, if you have ac_add_options --enable-warnings-as-errors in your .mozconfig many (all?) of the static analysis error will show up locally when compiling

Thanks - that did the trick nicely! (Although it didn't do much for my build times: now >90mins!)

Confirmed that I got the compile error when I backed out your NO_THREAD_SAFETY_ANALYSIS fix.
And confirmed that the build works with my new patch, so I'm submitted it to phab.
Still not 100% certain why the static analyzer picked it up in the first place, but even if it was erroneous, I think my patch cleans up some cruft as well as satisfying the analyzer.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/55e518879968
Fix possible bogus release of un-held waitForBodyIdsMonitor object in IMAP. r=mkmelin