Closed Bug 1374792 Opened 2 years ago Closed 2 years ago

Crash in @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface

Categories

(Core :: Disability Access APIs, defect, critical)

56 Branch
Unspecified
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 + fixed

People

(Reporter: marcia, Assigned: aklotz)

References

Details

(Keywords: crash, regression, Whiteboard: aes+)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-f19a3b39-8d9f-40de-a5f6-11fd90170620.
=============================================================

Seen while looking at crash stats - new crash which started in 20170620030208 - 3 installs/18 crashes: http://bit.ly/2snbLpy. Also another signature - [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] which seems to be one install crashing.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=95543bdc59bd038a3d5d084b85a4fec493c349ee&tochange=7a6baa6cca3292e8099e652b64d27e74df560874

Not sure if I put it in the correct component, feel free to bucket in the right place.
Thanks, Marcia, yes this is ours. :-) Aaron, any ideas?
Flags: needinfo?(aklotz)
We're looking into it...
Flags: needinfo?(aklotz)
Whiteboard: aes+
Crash Signature: [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] → [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID]
Crash Signature: [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] → [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ mozilla::mscom::Interceptor::Lookup ]
This smells to me like more defunct IID pointers...
Hmmm... examining a dump tells a different story. Another null vtable! I think we can dupe some bugs now...
Duplicate of this bug: 1374984
Crash Signature: [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ mozilla::mscom::Interceptor::Lookup ] → [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ mozilla::mscom::Interceptor::Lookup ] [@ <T>::o…
Crash Signature: [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ mozilla::mscom::Interceptor::Lookup ] [@ <T>::o… → [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ nsCOMPtr<T>::~nsCOMPtr<T> | mozilla::mscom::Inte…
Crash Signature: [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ nsCOMPtr<T>::~nsCOMPtr<T> | mozilla::mscom::Inte… → [@ @0x0 | mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::ThreadSafeQueryInterface] [@ mozilla::mscom::Interceptor::~Interceptor] [@ RefPtr<T>::~RefPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ n…
Crash Signature: nsCOMPtr<T>::~nsCOMPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ mozilla::mscom::Interceptor::Lookup ] [@ <T>::operator() | mozilla::detail::RunnableFunction<T>::Run ] [@ RefPtr<T>::assign_assuming_AddRef | <T>::operator() | mozilla::… → nsCOMPtr<T>::~nsCOMPtr<T> | mozilla::mscom::Interceptor::GetInterceptorForIID] [@ mozilla::mscom::Interceptor::Lookup ] [@ <T>::operator() | mozilla::detail::RunnableFunction<T>::Run ] [@0x0 | <T>::operator() | mozilla::detail::RunnableFunction<T>::Ru…
Duplicate of this bug: 1377028
Tracking this since it's a fairly high volume crash in 56 (even just looking at a couple of the signatures listed)
I have a patch that will probably eliminate the symptoms.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
This doesn't fix the root cause but it should prevent the conditions that trigger the root cause, if that makes sense.

Currently the handler calls IGeckoBackChannel::put_HandlerControl once per accessible, when really we only need to call it once for the lifetime of the handler itself. I added a boolean flag to track this.
Attachment #8882640 - Flags: review?(eitan)
Keywords: leave-open
OS: Windows 7 → Windows
Comment on attachment 8882640 [details] [diff] [review]
Only call IGeckoBackChannel::put_HandlerControl once

Review of attachment 8882640 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the late review!

::: accessible/ipc/win/handler/AccessibleHandlerControl.cpp
@@ +198,5 @@
> +  }
> +
> +  long pid = static_cast<long>(::GetCurrentProcessId());
> +  HRESULT hr = aGecko->put_HandlerControl(pid, this);
> +  mIsRegistered = SUCCEEDED(hr);

This assumes that put_HandlerControl will eventually succeed. If not, then we try to register forever. I don't see a real case where that happens though.
Attachment #8882640 - Flags: review?(eitan) → review+
Crash Signature: mozilla::detail::RunnableFunction<T>::Run [@ RefPtr<T>::assign_assuming_AddRef | <T>::operator() | mozilla::detail::RunnableFunction<T>::Run] [@ CoMarshalInterface ] [@ CDestObjectWrapper::MarshalInterface ] [@ @0x0 | CoMarshalInterface ] [@ gfxDWri… → mozilla::detail::RunnableFunction<T>::Run [@ RefPtr<T>::assign_assuming_AddRef | <T>::operator() | mozilla::detail::RunnableFunction<T>::Run] [@ CoMarshalInterface ] [@ CDestObjectWrapper::MarshalInterface ] [@ @0x0 | CoMarshalInterface ] [@ @0x0 | …
Duplicate of this bug: 1378382
https://hg.mozilla.org/integration/mozilla-inbound/rev/210dc607699827d3c57cc3b250e8e99a55ef6e30
Bug 1374792: Modify a11y handler to only invoke IGeckoBackChannel::put_HandlerControl once; r=eeejay
I think I've sorted this out: as a perf optimization, I made the event object in EnsureMTA::EnsureMTA static so that we didn't need to create a new event on each call. Unfortunately we later do a WaitForSingleObjectEx on that event, which might dispatch APCs, which might reenter EnsureMTA::EnsureMTA, and would then attempt to reuse the existing event.

I could get more exotic with maintaining a stack of event objects for reuse, but for now I think I'll just add a comment and remove the static keyword. If profiling shows it to be an issue then I'll reevaluate.
Attachment #8884099 - Flags: review?(davidp99)
Duplicate of this bug: 1376523
Comment on attachment 8884099 [details] [diff] [review]
Remove static from declaration of event in mscom::EnsureMTA::EnsureMTA

LGTM
Attachment #8884099 - Flags: review?(davidp99) → review+
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/843a2c9538f9fc44dad973f51f5e31ce72a05776
Bug 1374792: Remove static keyword from declaration of event in mscom::EnsureMTA::EnsureMTA to prevent reuse when thread reenters; r=handymand
https://hg.mozilla.org/mozilla-central/rev/843a2c9538f9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
We'll file a new bug if necessary; any remaining crashes would have a different cause.
Flags: needinfo?(aklotz)
I just got this one-time crash in the July 21 nightly build after Gmail had been refreshed.
bp-a878862d-190a-4ded-8c6b-61cd10170721

Aaron, is this the same/related bug or something new?
Flags: needinfo?(aklotz)
Something new.
Flags: needinfo?(aklotz)
You need to log in before you can comment on or make changes to this bug.