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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marcia Knous [:marcia]]

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.

Comment 1

[Marco Zehe (:MarcoZ)]

Thanks, Marcia, yes this is ours. :-) Aaron, any ideas?

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

We're looking into it...

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

This smells to me like more defunct IID pointers...

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Hmmm... examining a dump tells a different story. Another null vtable! I think we can dupe some bugs now...

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this since it's a fairly high volume crash in 56 (even just looking at a couple of the signatures listed)

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

I have a patch that will probably eliminate the symptoms.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Only call IGeckoBackChannel::put_HandlerControl once

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.

Comment 10

[Eitan Isaacson [:eeejay]]

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.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/210dc607699827d3c57cc3b250e8e99a55ef6e30
Bug 1374792: Modify a11y handler to only invoke IGeckoBackChannel::put_HandlerControl once; r=eeejay

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/210dc6076998

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Remove static from declaration of event in mscom::EnsureMTA::EnsureMTA

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.

Comment 16

[David Parks [:handyman]]

Comment on attachment 8884099 [details] [diff] [review]
Remove static from declaration of event in mscom::EnsureMTA::EnsureMTA

LGTM

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/843a2c9538f9

Comment 19

[Ting-Yu Chou [:ting] (away)]

The signature is still the top #5 of Nightly 20170709030204 on Windows:

  https://crash-stats.mozilla.com/signature/?build_id=20170709030204&product=Firefox&release_channel=nightly&_sort=-date&platform=Windows&signature=mozilla%3A%3Amscom%3A%3AInterceptor%3A%3AThreadSafeQueryInterface&date=%3E%3D2017-07-09T00%3A00%3A00.000Z&date=%3C2017-07-12T02%3A01%3A00.000Z#aggregations

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

We'll file a new bug if necessary; any remaining crashes would have a different cause.

Comment 21

[Marco Zehe (:MarcoZ)]

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?

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

Something new.