On Win64, ASan commits shadow memory on-demand with a VectoredExceptionHandler that catches page faults. DetectInSendMessageExCompat is also a VectoredExceptionHandler, and it is registered with FirstHandler = TRUE. So if DetectInSendMessageExCompat faults while accessing shadow memory, the VEH logic will call DetectInSendMessageExCompat again, and we recurse forever until the stack is exhausted. dbolter (since aklotz is out): do you recall from bug 1310056 if this handler has any specific need to be first? The simplest fix here would be to set FirstHandler = FALSE when registering the function. Alternatively we could set FirstHandler = FALSE only for ASan builds, like we do in JS: https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/js/src/wasm/WasmSignalHandlers.cpp#1397 Or, really, we could probably just disable this code if MOZ_ASAN, right? I'm guessing our automation machines aren't too concerned about this compat workaround.

Obviously, I'm not DBolter, but I am somewhat familiar with what this code is supposed to do. (In reply to David Major [:dmajor] from comment #0) > dbolter (since aklotz is out): do you recall from bug 1310056 if this > handler has any specific need to be first? The simplest fix here would be to > set FirstHandler = FALSE when registering the function. I guess the intent was to make sure that we absolutely catch this RPC_E_CANTCALLOUT_ININPUTSYNCCALL exception before anything else does, since we really need to catch it in order to enable the hack. However, I can't see any reason for another vectored handler to swallow this, so perhaps being first is not important. I can try to test this if that is helpful. > Or, really, we could probably just disable this code if MOZ_ASAN, right? I'm > guessing our automation machines aren't too concerned about this compat > workaround. Correct, unless we start doing automated integration testing with a commercial screen reader, but I don't imagine that will ever happen. :)

For what it's worth, we can't be 100% certain of going first anyway, since someone else could come along later and set their own FirstHandler == TRUE. As long as FirstHandler == FALSE still fixes the bug 1310056 case in ordinary usage, I think that would be my preferred route.

Sounds like a plan - who's taking the bug?

Jamie, if you could test this out as well, I'd appreciate it!

Comment on attachment 8963281 [details] [diff] [review] Bug 1449337: Don't take the first VectoredExceptionHandler slot away from ASan. TL;DR: I believe this works as expected and doesn't break anything. Some notes on testing for reference: Testing this properly took a bit of work, as I don't know how to reproduce the RPC_E_CANTCALLOUT_ININPUTSYNCCALL problem in the real world; NVDA worked around this some time ago and I can't seem to reproduce it with JAWS with some very brief testing. I ended up patching NVDA with a hack which catches a sent message, makes an outgoing COM call and returns the result, which was a lot easier than writing an in-proc test case from scratch. Steps: 1. Apply the attached patch to NVDA. 2. Patch Gecko to engage the hack even for NVDA: diff --git a/accessible/windows/msaa/Compatibility.cpp b/accessible/windows/msaa/Compatibility.cpp index 00556ed5f069..b2388e6d499f 100644 --- a/accessible/windows/msaa/Compatibility.cpp +++ b/accessible/windows/msaa/Compatibility.cpp @@ -264,7 +264,7 @@ Compatibility::Init() // If we have a consumer who is not NVDA, we enable detection for the // InSendMessageEx compatibility hack. NVDA does not require this. // We also skip UIA, as we see crashes there. - if ((sConsumers & (~(UIAUTOMATION | NVDA))) && + if (/*(sConsumers & (~(UIAUTOMATION | NVDA))) &&*/ BrowserTabsRemoteAutostart()) { sUser32Interceptor.Init("user32.dll"); if (!sInSendMessageExStub) { 3. Run the patched builds of both apps. (This should include, e.g., the patch from this bug as well.) 4. In Firefox, focus a document with at least one child. 5. press NVDA+control+z to open the NVDA Python console. 6. Enter the following command: import winUser; winUser.sendMessage(focus.windowHandle, 1100, focus.firstChild.IA2UniqueID, 0) 7. Run the same command again. (The hack isn't engaged until it sees the first failure.) In step 7, if all is working as expected, you should get a return code of 0. If the hack isn't being engaged, you'll instead get a return code of -2147024809. Note that this is not RPC_E_CANTCALLOUT_ININPUTSYNCCALL, but that error gets masked by our accChild code in the parent process and we instead get E_INVALIDARG. Regardless, whether or not the hack is engaged correctly is evident in the failure.

Wow, thanks for doing all that work to check this!

Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad923f6b31c Don't take the first VectoredExceptionHandler slot away from ASan. r=Jamie