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

VERIFIED FIXED in Firefox -esr60

Status

()

--
critical
VERIFIED FIXED
9 months ago
8 months ago

People

(Reporter: philipp, Assigned: Jamie)

Tracking

({crash, regression})

61 Branch
mozilla63
x86_64
Windows
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr6062+ fixed, firefox61+ fixed, firefox62+ verified, firefox63+ verified)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
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.
(Reporter)

Comment 1

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

Comment 2

9 months ago
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.
Flags: needinfo?(jteh)
(Reporter)

Updated

9 months ago
status-firefox62: ? → affected
(Assignee)

Comment 3

8 months ago
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. :(
Flags: needinfo?(jteh)
(Assignee)

Updated

8 months ago
Assignee: nobody → jteh
Component: Disability Access APIs → IPC: MSCOM
Comment hidden (mozreview-request)
status-firefox63: ? → affected
status-firefox-esr60: unaffected → affected
tracking-firefox61: --- → +
tracking-firefox62: --- → +
tracking-firefox63: --- → +
tracking-firefox-esr60: --- → 62+

Comment 5

8 months ago
mozreview-review
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
Attachment #8990634 - Flags: review?(aklotz) → review+

Comment 6

8 months ago
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/391dc33b9d80
Null check to prevent crash when ipc::mscom::GetInitialInterceptorForIID fails after PublishTarget. r=aklotz

Comment 7

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/391dc33b9d80
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Reporter)

Comment 8

8 months ago
hi, can you also request uplift to 62/esr in case you deem fit to do so? thank you
Flags: needinfo?(jteh)
(Assignee)

Comment 9

8 months ago
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.
Flags: needinfo?(jteh)
Attachment #8990634 - Flags: approval-mozilla-release?
Attachment #8990634 - Flags: approval-mozilla-esr60?
Attachment #8990634 - Flags: approval-mozilla-beta?
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.
Attachment #8990634 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 11

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/a498f0d3e43c
status-firefox62: affected → fixed
(Reporter)

Updated

8 months ago
Crash Signature: [@ RtlReleaseSRWLockExclusive | mozilla::mscom::Interceptor::GetInitialInterceptorForIID] → [@ RtlReleaseSRWLockExclusive | mozilla::mscom::Interceptor::GetInitialInterceptorForIID] [@ RtlReleaseSRWLockExclusive | mozilla::detail::MutexImpl::unlock | mozilla::mscom::Interceptor::GetInitialInterceptorForIID]
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.
Attachment #8990634 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Comment 13

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr60/rev/97ac105db46d
status-firefox-esr60: affected → fixed
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox63: fixed → verified
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.
Attachment #8990634 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment 15

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/ad93c4fae965
status-firefox61: affected → fixed
You need to log in before you can comment on or make changes to this bug.