1761040 - Consider renaming the thread safety macros

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[[:fabrice] Fabrice Desré]

On Gonk, we had to workaround re-definition of the CAPABILITY macro because of some unfortunate include sequence in unified builds. That looks like:

1. #include "mfbt/ThreadSafety.h" -> CAPABILITY is defined, as well as the mozilla_ThreadSafety_h header guard.

2. One of b2g-sysroot/include/android-base/thread_annotations.h or b2g-sysroot/include/utils/Mutex.h are included, modifiying CAPABILITY (these headers are from the android sysroot).

3. #include "mfbt/ThreadSafety.h" appears again, but because mozilla_ThreadSafety_h is defined, CAPABILITY keeps the android sysroot value.

4. Kaboom.

We worked around on Gonk with a crappy "let's always re-include ThreadSafety.h" (https://github.com/kaiostech/gecko-b2g/commit/94b6ac940c927061200244768f32a5e9db30f96d) but this is not great obviously, and that could happen to other platforms.

I think using a unique name for these macros would prevent such issues down the line.

Comment 1

[[:fabrice] Fabrice Desré]

ping Jesup. Can we figure out a good way out of this?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Either remove the thread safety macros to use MOZ_ prefixes, or use #pragma push_macro("CAPABILITY") like we do here or so?

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: Bug 1761040 - Prefix thread safety macros with MOZ_ r=emilio

Comment 4

[[:fabrice] Fabrice Desré]

The try run is at https://treeherder.mozilla.org/jobs?repo=try&revision=d2d829d6bf390d3c46b2b3f340e2e9bca31816a3

Most of the changes have been generated by applying this script with the needed macro name as input:

#!/bin/bash

export SEARCH=$1

SED_ARG="s/${SEARCH}/MOZ_${SEARCH}/g"

rg -l " $SEARCH\(" | grep -v -E "other_licenses|third_party|security/sandbox/chromium|toolkit/components/protobuf" | xargs sed -i $SED_ARG

And a couple of manual fixup in tests afterwards.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: Bug 1761040 - fix linting errors r=emilio

Depends on D152575

Comment 6

[[:fabrice] Fabrice Desré]

I'm not sure why TC complains about:

Message: 'rlbox.wasm.h' file not found
Location: gfx/thebes/ThebesRLBox.h:17

since this patch doesn't touch that file and running ./mach static-analysis check --outgoing locally does not produce errors.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a907159a482f
Prefix thread safety macros with MOZ_ r=geckoview-reviewers,media-playback-reviewers,alwu,jesup,m_kato

Comment 8

[Andreea Pavel [:apavel]]

Backed out for causing build bustages
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=a907159a482f622fcf42b0651a299aa46a5f3c12&selectedTaskRun=TG7uaCi3S2KJcXEtmiqqOw.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=386072929&repo=autoland&lineNumber=6098
Backout: https://hg.mozilla.org/integration/autoland/rev/e0b787859230cd1d17486bfbf8c7f08775a22db6

Comment 9

[[:fabrice] Fabrice Desré]

Build failure fixed and pushed to try: https://treeherder.mozilla.org/jobs?repo=try&revision=106a869dd0cd7d4a2ce5c419e939d1273138124b&selectedTaskRun=Mt6-Tv3ERXyNeCovLWk4Iw.0

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9d2965591b9
Prefix thread safety macros with MOZ_ r=geckoview-reviewers,media-playback-reviewers,alwu,jesup,m_kato

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05ab424ab12d
Backed out changeset b9d2965591b9 for landing with wrong author CLOSED TREE DONTBUILD

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10d13b6a9379
Prefix thread safety macros with MOZ_ r=geckoview-reviewers,media-playback-reviewers,alwu,jesup,m_kato

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/10d13b6a9379