Closed
Bug 1474007
Opened 6 years ago
Closed 6 years ago
Crash in RtlReleaseSRWLockExclusive | mozilla::mscom::Interceptor::GetInitialInterceptorForIID
Categories
(Core :: IPC: MSCOM, defect)
Tracking
()
VERIFIED
FIXED
mozilla63
People
(Reporter: philipp, Assigned: Jamie)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
bugzilla
:
review+
lizzard
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-release+
RyanVM
:
approval-mozilla-esr60+
|
Details |
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•6 years ago
|
||
sorry, a correction: it's a crash in the content process, not a shutdownhang
Comment 2•6 years 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.
Updated•6 years ago
|
Flags: needinfo?(jteh)
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 3•6 years 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•6 years ago
|
Assignee: nobody → jteh
Component: Disability Access APIs → IPC: MSCOM
Comment hidden (mozreview-request) |
Updated•6 years ago
|
tracking-firefox61:
--- → +
tracking-firefox62:
--- → +
tracking-firefox63:
--- → +
tracking-firefox-esr60:
--- → 62+
Comment 5•6 years 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+
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/391dc33b9d80
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Reporter | ||
Comment 8•6 years 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•6 years 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 10•6 years ago
|
||
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•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a498f0d3e43c
Reporter | ||
Updated•6 years ago
|
Crash Signature: [@ RtlReleaseSRWLockExclusive | mozilla::mscom::Interceptor::GetInitialInterceptorForIID] → [@ RtlReleaseSRWLockExclusive | mozilla::mscom::Interceptor::GetInitialInterceptorForIID]
[@ RtlReleaseSRWLockExclusive | mozilla::detail::MutexImpl::unlock | mozilla::mscom::Interceptor::GetInitialInterceptorForIID]
Comment 12•6 years ago
|
||
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•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/97ac105db46d
Updated•6 years ago
|
Comment 14•6 years ago
|
||
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•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/ad93c4fae965
You need to log in
before you can comment on or make changes to this bug.
Description
•