1954896 - MOZ_CRASH(mozilla::a11y::AccAttributes not thread-safe)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[:gerard-majax]

03-18 20:22:27.138  5531  5531 F MOZ_CRASH: [5531] Hit MOZ_CRASH(mozilla::a11y::AccAttributes not thread-safe) at /home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/xpcom/base/nsISupportsImpl.cpp:43
03-18 20:22:27.143  5531  5531 F MOZ_Crash: #01: ???[/data/app/~~2ho3RFyzmcuhABVZCre1Cw==/org.mozilla.fenix.debug-jqZ1l5dnf63X1GONM3pRDg==/lib/x86_64/libxul.so +0x5d5ef40]
03-18 20:22:27.147  5531  5531 F MOZ_Crash: #02: ???[/data/app/~~2ho3RFyzmcuhABVZCre1Cw==/org.mozilla.fenix.debug-jqZ1l5dnf63X1GONM3pRDg==/lib/x86_64/libxul.so +0xc2ccb54]
03-18 20:22:27.147  5531  5531 F MOZ_Crash: #03: ???[/data/app/~~2ho3RFyzmcuhABVZCre1Cw==/org.mozilla.fenix.debug-jqZ1l5dnf63X1GONM3pRDg==/lib/x86_64/libxul.so +0xc2baa60]
03-18 20:22:27.147  5531  5531 F MOZ_Crash: #04: ???[/data/app/~~2ho3RFyzmcuhABVZCre1Cw==/org.mozilla.fenix.debug-jqZ1l5dnf63X1GONM3pRDg==/lib/x86_64/libxul.so +0xc2bb31f]
03-18 20:22:27.149  5531  5531 F MOZ_Crash: #05: ???[/data/app/~~2ho3RFyzmcuhABVZCre1Cw==/org.mozilla.fenix.debug-jqZ1l5dnf63X1GONM3pRDg==/lib/x86_64/libxul.so +0xc207370]
03-18 20:22:27.149  5531  5531 F MOZ_Crash: #06: ???[/data/app/~~2ho3RFyzmcuhABVZCre1Cw==/org.mozilla.fenix.debug-jqZ1l5dnf63X1GONM3pRDg==/lib/x86_64/libxul.so +0xc206a21]
03-18 20:22:27.149  5531  5531 F MOZ_Crash: #07: ???[/data/app/~~2ho3RFyzmcuhABVZCre1Cw==/org.mozilla.fenix.debug-jqZ1l5dnf63X1GONM3pRDg==/lib/x86_64/libxul.so +0xc211062]

Seems to translate to:

/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/obj-browser-android-dbg/dist/include/mozilla/Assertions.h:55
/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/obj-browser-android-dbg/dist/include/nsISupportsImpl.h:354
/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/accessible/ipc/RemoteAccessible.cpp:1552
/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/obj-browser-android-dbg/dist/include/mozilla/RefPtr.h:338
/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/obj-browser-android-dbg/dist/include/mozilla/AlreadyAddRefed.h:153
/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/accessible/android/SessionAccessibility.cpp:?
/home/alexandre/Documents/codaz/Mozilla/MiscWork/mozilla-source/mozilla-unified/obj-browser-android-dbg/dist/include/mozilla/jni/Natives.h:609

VERIFY_CACHE at https://searchfox.org/mozilla-central/rev/0f5273b9b232a3daa0a710e851cbb24393d62845/accessible/ipc/RemoteAccessible.cpp#1552

Repro with (full stack that includes) https://phabricator.services.mozilla.com/D229885 and ./mach gradle -p mobile/android/fenix/ connectedFenixDebugAndroidTest -Pandroid.testInstrumentationRunnerArguments.class=org.mozilla.fenix.ui.UnsubmittedCrashDialogTest#UnsubmittedCrashDialog_PullingOneCrash_ClickOnLearnMoreTriggersIntent_andDontDismissesDialog, debug build, running against an android 15 emulator

Comment 1

[James Teh [:Jamie]]

I think VERIFY_CACHE is a red herring, since we only execute that if eCache logging is enabled, which it isn't by default.

I think the problem is that we're getting an AccAttributes RefPtr out of mCachedFields, but that means we have to AddRef it, which we can't do off the main thread. We might need some way to get a raw AccAttributes pointer instead. That's theoretically less safe, but callers shouldn't hold onto it beyond the stack frame anyway, and its lifetime is guaranteed as long as the RemoteAccessible isn't mutated. Alternatively, we could make AccAttributes use NS_INLINE_DECL_THREADSAFE_REFCOUNTING, but I'm not sure what overhead that incurs and it seems wasteful given that we explicitly acquire a lock to ensure this isn't accessed concurrently.

Comment 2

[James Teh [:Jamie]]

Eitan, any thoughts, preferences or better ideas regarding the proposed solutions in comment 1?

Comment 3

[:gerard-majax]

Attachment: WIP: Bug 1954896 - Use refcounting for AccAttributes

Comment 4

[:gerard-majax]

I can confirm the suggested workaround at least unblocks me on the test I was working on.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #1)

I think VERIFY_CACHE is a red herring, since we only execute that if eCache logging is enabled, which it isn't by default.

I think the problem is that we're getting an AccAttributes RefPtr out of mCachedFields, but that means we have to AddRef it, which we can't do off the main thread.

What I don't understand is why is this assertion not tripping all the time? We get a strong ref to the attributes every time we populate a node:
https://searchfox.org/mozilla-central/rev/4ce36232b265b53de4fb7eb754430f94e262bbbe/accessible/android/SessionAccessibility.cpp#696

We might need some way to get a raw AccAttributes pointer instead. That's theoretically less safe, but callers shouldn't hold onto it beyond the stack frame anyway, and its lifetime is guaranteed as long as the RemoteAccessible isn't mutated. Alternatively, we could make AccAttributes use NS_INLINE_DECL_THREADSAFE_REFCOUNTING, but I'm not sure what overhead that incurs and it seems wasteful given that we explicitly acquire a lock to ensure this isn't accessed concurrently.

It could really only be safe when our Android ReleasableMonitorAutoLock outlives the attributes weak link.

Comment 6

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #5)

What I don't understand is why is this assertion not tripping all the time? We get a strong ref to the attributes every time we populate a node

I think it's just a missing test case. This will only trip if the ARIA attributes set actually contains something, which it won't if there are no aria- attributes that need to be exposed using object attributes. That won't happen with aria-checked, aria-selected, etc. To trip it, we'd need to use something like aria-sort. We do have an aria-live test which would theoretically trip it, but only if we tried to fetch the info for the node, not just listen to announcement events.

It could really only be safe when our Android ReleasableMonitorAutoLock outlives the attributes weak link.

Sure, but that should always be the case, since in every case, we fetch the attributes on the stack, use them and then return. It'd be good if we could enforce that, but that's tricky, unless we create some MOZ_STACK_CLASS wrapper, which is a bit fiddly.

Comment 7

[Eitan Isaacson [:eeejay]]

OK, I'm happy with that approach. I could work on a patch tomorrow unless you already have one ready.

Comment 8

[James Teh [:Jamie]]

I don't have one. Please feel free to take it if you have the cycles.

Comment 9

[:gerard-majax]

I cannot reproduce anymore, so removing the blocking

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1954896 - Use weak reference in RemoteAccessible::GetCachedARIAAttributes. r?Jamie

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30ae238e0cea
Use weak reference in RemoteAccessible::GetCachedARIAAttributes. r=Jamie,geckoview-reviewers,m_kato

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/30ae238e0cea