1474007 - Crash in RtlReleaseSRWLockExclusive | mozilla::mscom::Interceptor::GetInitialInterceptorForIID

Status: VERIFIED FIXED

(Core :: IPC: MSCOM, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-fe95dc9f-e2e7-4617-84ea-3df960180706.
=============================================================

Top 10 frames of crashing thread:

0 ntdll.dll RtlReleaseSRWLockExclusive 
1 xul.dll mozilla::mscom::Interceptor::GetInitialInterceptorForIID ipc/mscom/Interceptor.cpp:603
2 xul.dll mozilla::mscom::Interceptor::Create ipc/mscom/Interceptor.cpp:265
3 xul.dll mozilla::mscom::CreateInterceptor<IAccessible> ipc/mscom/Interceptor.h:186
4 xul.dll mozilla::mscom::MainThreadHandoff::WrapInterface<IAccessible> ipc/mscom/MainThreadHandoff.h:50
5 xul.dll mozilla::a11y::CreateHolderFromAccessible accessible/ipc/win/COMPtrTypes.cpp:52
6 xul.dll mozilla::a11y::DocAccessible::DoInitialUpdate accessible/generic/DocAccessible.cpp:1476
7 xul.dll mozilla::a11y::DocAccessibleWrap::DoInitialUpdate accessible/windows/msaa/DocAccessibleWrap.cpp:146
8 xul.dll mozilla::a11y::NotificationController::WillRefresh accessible/base/NotificationController.cpp:666
9 xul.dll nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:1896

=============================================================

this shutdownhang signature is newly (and so far only) appearing in 61.0.1, so i guess it's related to the work to address bug 1472137 that went into the dot-release. so far it's hitting win64 builds on all win versions.

Comment 1

[[:philipp]]

sorry, a correction: it's a crash in the content process, not a shutdownhang

Comment 2

[Eric Rahm [:erahm]]

This looks like maybe we're double releasing? That stack implies we're hitting the error cleanup path, but also implies we're hitting the non-error exit path. It's possible inlining is messing things up.

Comment 3

[James Teh [:Jamie]]

Ug. This is the trouble with "fixing" stuff you can't reproduce.

More useful stack (including inline calls):

0:000> kp
 # Child-SP          RetAddr           Call Site
00 00000000`003cda70 000007fe`df4fb6f6 ntdll!RtlReleaseSRWLockExclusive+0x8
01 (Inline Function) --------`-------- xul!mozilla::OffTheBooksMutex::Unlock+0x9 [z:\build\build\src\obj-firefox\dist\include\mozilla\mutex.h @ 70] 
02 (Inline Function) --------`-------- xul!mozilla::mscom::detail::LiveSet::Unlock+0x9 [z:\build\build\src\ipc\mscom\interceptor.cpp @ 56] 
03 (Inline Function) --------`-------- xul!mozilla::mscom::detail::LiveSetAutoLock::Unlock+0x9 [z:\build\build\src\ipc\mscom\interceptor.cpp @ 107] 
04 (Inline Function) --------`-------- xul!mozilla::mscom::Interceptor::GetInitialInterceptorForIID::__l2::<lambda_755964676d3d653c8bdddcf1e8986166>::operator()+0x9 [z:\build\build\src\ipc\mscom\interceptor.cpp @ 552] 
05 (Inline Function) --------`-------- xul!mozilla::mscom::ExecuteWhen<<lambda_8ba5c7f77a71162aef42492895e8ffc8>,<lambda_755964676d3d653c8bdddcf1e8986166> >::{dtor}+0x13 [z:\build\build\src\obj-firefox\dist\include\mozilla\mscom\utils.h @ 87] 
06 00000000`003cdaa0 000007fe`df4fb184 xul!mozilla::mscom::Interceptor::GetInitialInterceptorForIID(class mozilla::mscom::detail::LiveSetAutoLock * aLiveSetLock = 0x00000000`003cdba8, struct _GUID * aTargetIid = <Value unavailable error>, class mozilla::UniquePtr<IUnknown,mozilla::mscom::detail::MainThreadRelease<IUnknown> > * aTarget = 0x00000000`003cdb60, void ** aOutInterceptor = 0x00000000`003cdc58)+0x2b2 [z:\build\build\src\ipc\mscom\interceptor.cpp @ 603] 

interceptor.cpp @ 603 is the return for the success case, but I think that line number is wrong:

0:000> .frame 6
06 00000000`003cdaa0 000007fe`df4fb184 xul!mozilla::mscom::Interceptor::GetInitialInterceptorForIID+0x2b2 [z:\build\build\src\ipc\mscom\interceptor.cpp @ 603] 
0:000> ?? hr
HRESULT 0x80070002

So GetInterceptorForIID (line 601) actually failed, thus triggering the unlock.

Unfortunately, while I foresaw this possibility in bug 1472137 comment 9, my analysis about what would happen if it occurred was incorrect:

> One thing worth noting with this patch is that there are theoretically cases where PublishTarget succeeds (and releases the lock), but a subsequent call (e.g. QueryInterface or GetInterceptorForIID) fails. In that case, the ExecuteWhen cleanup code will call Unlock again, thus triggering MOZ_ASSERT(mLiveSet) in LiveSetAutoLock::Unlock. I figure ENSURE_HR_SUCCEEDED would already have asserted in that case, so we're already "broken".

We don't assert, of course, but mLiveSet is indeed null:

0:000> ?? aLiveSetLock
class mozilla::mscom::detail::LiveSetAutoLock * 0x00000000`003cdba8
   +0x000 mLiveSet         : (null) 

The problem is that while we do safety check mLiveSet is not null in ~LiveSetAutoLock, we *don't* check it in Unlock. Very silly of me not to realise that. :(

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1474007: Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget.

PublishTarget calls Unlock on our LiveSetAutolock.
It's possible for GetInitialInterceptorForIID to fail after this point.
This will cause the failure cleanup code to run, which tries to call Unlock again.
However, the previous call to Unlock set mLiveSet to null, and Unlock previously didn't handle this case.
Now, unlock is a no-op (in release builds) if it's already been called.

Review commit: https://reviewboard.mozilla.org/r/255710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255710/

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8990634 [details]
Bug 1474007: Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget.

https://reviewboard.mozilla.org/r/255710/#review262648

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/391dc33b9d80
Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget. r=aklotz

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/391dc33b9d80

Comment 8

[[:philipp]]

hi, can you also request uplift to 62/esr in case you deem fit to do so? thank you

Comment 9

[James Teh [:Jamie]]

Comment on attachment 8990634 [details]
Bug 1474007: Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1364624/bug 1472137.
[User impact if declined]: Some users who use third party software which cause Firefox accessibility to be enabled (not necessarily traditional accessibility tools) may experience crashes.
[Is this code covered by automated tests?]: No. Among other things, we don't actually know how to reproduce it.
[Has the fix been verified in Nightly?]: No, because we don't know how to reproduce it.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1472137 (which has been uplifted to beta and release but not 60 ESR).
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Simple null check.
[String changes made/needed]: None.

[ESR specific ]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes crashes seen in the wild.
Fix Landed on Version: 63, but also requesting uplift to 62 and 61 here.
String or UUID changes made by this patch: None.

Comment 10

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8990634 [details]
Bug 1474007: Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget.

Simple fix for a moderate (on release) volume crash, let's uplift for beta 8.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a498f0d3e43c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8990634 [details]
Bug 1474007: Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget.

Follow-up fix for bug 1472137. Approved for ESR 60.2.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/97ac105db46d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8990634 [details]
Bug 1474007: Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget.

Follow-up to a crash fix taken in 61.0.1. Well baked on Nightly and Beta. Approved for 61.0.2.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/ad93c4fae965