1367715 - Crash in NS_CycleCollectorSuspect3

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-31acd98d-3c1a-4d96-a943-a25ae0170525.
=============================================================

There is 1 crash in nightly 55 with buildid 20170524030204. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1361879.

[1] https://hg.mozilla.org/mozilla-central/rev?node=d7c33d5c6270bbe2ad93f21fd0e7347debb788de

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

This is due to an inadvertent AddRef() taking place off the main thread.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Convert IsProxy check to assertion

Normally we don't expect to be wrapping proxies. It's not a good thing for us to do, but since it shouldn't normally be happening, I think we should just convert this to an assertion.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67e4f5e8a105f135f004fd40cb2e4ab28c530e8c
Bug 1367715: Convert IsProxy check to assertion to avoid unnecessary and potentially incorrect off-main-thread QIs on release builds; r=jimm

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67e4f5e8a105

Comment 5

[Makoto Kato [:m_kato]]

After landing this, web-platform-tests always hits assertion on my environment.  Of course, it work with --disable-e10s.

0:028> r
rax=0000000000000000 rbx=00000000000001fc rcx=00000000ffffffff
rdx=000000fbcb1fdda8 rsi=0000024ea6cf3620 rdi=0000024ea4a47e20
rip=00007ffe02192a91 rsp=000000fbcb1fdd90 rbp=000000fbcb1fde10
 r8=000000fbcb1fdda0  r9=000000fbcb1fdd98 r10=0000000000000000
r11=000000fbcb1f92f0 r12=000000fbcb1fe210 r13=0000000000000000
r14=0000000000000000 r15=000000fbcb1fe200
iopl=0         nv up ei pl nz na po nc
cs=0033  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000206
xul!mozilla::mscom::MainThreadHandoff::OnWalkInterface+0x285:
00007ffe`02192a91 cc              int     3

0:028> k
Child-SP          RetAddr           Call Site
000000fb`cb1fdd90 00007ffe`562c00e9 xul!mozilla::mscom::MainThreadHandoff::OnWalkInterface+0x285
000000fb`cb1fde30 00007ffe`562ebf77 ole32!CallFrame::WalkWorker+0xad
000000fb`cb1fdf40 00007ffe`562c0858 ole32!CallFrame::WalkWorker+0x2bf3b
000000fb`cb1fe050 00007ffe`021924c2 ole32!CallFrame::WalkFrame+0xd8
000000fb`cb1fe0c0 00007ffe`562c14e7 xul!mozilla::mscom::MainThreadHandoff::OnCall+0x1f6
000000fb`cb1fe140 00007ffe`562e39ef ole32!Interceptor::CallIndirect+0x117
000000fb`cb1fe1a0 00007ffe`566b88d3 ole32!COMInterceptor+0x8f
000000fb`cb1fe210 00007ffe`566595cf RPCRT4!Invoke+0x73
000000fb`cb1fe260 00007ffe`566591d3 RPCRT4!NdrStubCall2+0x38f
000000fb`cb1fe900 00007ffe`547c4dbf RPCRT4!NdrStubCall3+0xe3
000000fb`cb1fe960 00007ffe`566a3a2b combase!CStdStubBuffer_Invoke+0x7f
000000fb`cb1fe9a0 00007ffe`54867963 RPCRT4!CStdStubBuffer_Invoke+0x3b
(Inline Function) --------`-------- combase!InvokeStubWithExceptionPolicyAndTracing::__l6::<lambda_76d9e92c799d246a4afbe64a2bf5673d>::operator()+0x2b
000000fb`cb1fe9d0 00007ffe`54866286 combase!ObjectMethodExceptionHandlingAction<<lambda_76d9e92c799d246a4afbe64a2bf5673d> >+0x53
(Inline Function) --------`-------- combase!InvokeStubWithExceptionPolicyAndTracing+0x89
000000fb`cb1fea30 00007ffe`5486c75e combase!DefaultStubInvoke+0x216
(Inline Function) --------`-------- combase!SyncStubCall::Invoke+0x2c
(Inline Function) --------`-------- combase!SyncServerCall::StubInvoke+0x2c
(Inline Function) --------`-------- combase!StubInvoke+0x290
000000fb`cb1fec40 00007ffe`54867cd8 combase!ServerCall::ContextInvoke+0x45e
(Inline Function) --------`-------- combase!CServerChannel::ContextInvoke+0x86
(Inline Function) --------`-------- combase!DefaultInvokeInApartment+0x9f
000000fb`cb1fef20 00007ffe`548648aa combase!AppInvoke+0x338
000000fb`cb1ff090 00007ffe`548458c7 combase!ComInvokeWithLockAndIPID+0x57a
000000fb`cb1ff310 00007ffe`56693bb4 combase!ThreadInvoke+0xe17
000000fb`cb1ff540 00007ffe`56692cbd RPCRT4!DispatchToStubInCNoAvrf+0x24
000000fb`cb1ff590 00007ffe`566937d4 RPCRT4!RPC_INTERFACE::DispatchToStubWorker+0x1bd
000000fb`cb1ff660 00007ffe`566810be RPCRT4!RPC_INTERFACE::DispatchToStubWithObject+0x154
000000fb`cb1ff700 00007ffe`56681d35 RPCRT4!LRPC_SCALL::DispatchRequest+0x17e
000000fb`cb1ff7e0 00007ffe`5668521e RPCRT4!LRPC_SCALL::HandleRequest+0x8d5
000000fb`cb1ff8f0 00007ffe`56686ad8 RPCRT4!LRPC_ADDRESS::HandleRequest+0x2ee
000000fb`cb1ff990 00007ffe`566881e8 RPCRT4!LRPC_ADDRESS::ProcessIO+0x8d8
000000fb`cb1ffad0 00007ffe`567b2cd3 RPCRT4!LrpcIoComplete+0xd8
000000fb`cb1ffb70 00007ffe`567b16c6 ntdll!TppAlpcpExecuteCallback+0x273
000000fb`cb1ffc20 00007ffe`54ad2774 ntdll!TppWorkerThread+0x3f6
000000fb`cb1fff30 00007ffe`567e0d61 KERNEL32!BaseThreadInitThunk+0x14
000000fb`cb1fff60 00000000`00000000 ntdll!RtlUserThreadStart+0x21

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Ah, yes. It actually is fairly common to see proxies after all; the problem is that I need to detect this in a way that is both safe and performant.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add explicit checks for IClientSecurity to AccessibleWrap and sdnAccessible QueryInterface

This is a bit of a hack, but it is by far the best option at the moment.

The problem is that we need to query an interface for IClientSecurity to determine whether or not it is a proxy. Unfortunately we are doing this from a background thread, but the cost of properly switching to the main thread to do this query is way too expensive.

The quick 'n' dirty solution is to immediately check for IClientSecurity and return E_NOINTERFACE, thus preventing any thread-unsafe code from inadvertently being tripped over.

I'd like to have a nicer solution to this in the long term, but I think this will have to suffice for now.

Comment 8

[Eitan Isaacson [:eeejay]]

Comment on attachment 8874515 [details] [diff] [review]
Add explicit checks for IClientSecurity to AccessibleWrap and sdnAccessible QueryInterface

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

This looks like a simple enough change, but I think Alex to take this since I have never even looked at ISimpleDOMNode before.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8874515 [details] [diff] [review]
Add explicit checks for IClientSecurity to AccessibleWrap and sdnAccessible QueryInterface

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

r=me with comments addressed

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +119,5 @@
>    *ppv = nullptr;
>  
> +  if (IID_IClientSecurity == iid) {
> +    // Used to detect proxies; we want to check this immediately
> +    return E_NOINTERFACE;

what exactly expensive in this QI?

@@ +120,5 @@
>  
> +  if (IID_IClientSecurity == iid) {
> +    // Used to detect proxies; we want to check this immediately
> +    return E_NOINTERFACE;
> +  } else if (IID_IUnknown == iid)

you don't need to have else if after return

::: accessible/windows/sdn/sdnAccessible.cpp
@@ +38,5 @@
>    if (!aInstancePtr)
>      return E_FAIL;
>    *aInstancePtr = nullptr;
>  
> +  // Used to detect proxies; we want to check this immediately

it'd be nice to have more detailed comment here

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to alexander :surkov from comment #9)
> Comment on attachment 8874515 [details] [diff] [review]
> Add explicit checks for IClientSecurity to AccessibleWrap and sdnAccessible
> QueryInterface
> 
> Review of attachment 8874515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed
> 
> ::: accessible/windows/msaa/AccessibleWrap.cpp
> @@ +119,5 @@
> >    *ppv = nullptr;
> >  
> > +  if (IID_IClientSecurity == iid) {
> > +    // Used to detect proxies; we want to check this immediately
> > +    return E_NOINTERFACE;
> 
> what exactly expensive in this QI?

It's not really about expense but it's about QueryInterface(IID_IClientSecurity) being called off main thread. We want to handle that immediately (since that part is thread safe) before doing anything else inside that code that is likely not thread-safe.

> 
> @@ +120,5 @@
> >  
> > +  if (IID_IClientSecurity == iid) {
> > +    // Used to detect proxies; we want to check this immediately
> > +    return E_NOINTERFACE;
> > +  } else if (IID_IUnknown == iid)
> 
> you don't need to have else if after return

Fixed.

> 
> ::: accessible/windows/sdn/sdnAccessible.cpp
> @@ +38,5 @@
> >    if (!aInstancePtr)
> >      return E_FAIL;
> >    *aInstancePtr = nullptr;
> >  
> > +  // Used to detect proxies; we want to check this immediately
> 
> it'd be nice to have more detailed comment here

Sure.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add explicit checks for IClientSecurity to AccessibleWrap and sdnAccessible QueryInterface (r2)

Updated to address review comments, carrying forward r+.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e543c8a893137a588b4c6696d4c8b0920e260ab
Bug 1367715: Back out 67e4f5e8a105 as it was not the correct fix; r=backout

https://hg.mozilla.org/integration/mozilla-inbound/rev/43dd97a57eddaaacf8f373a3e7272a1920c4c9c6
Bug 1367715: Check for IClientSecurity in a11y QueryInterface implementations; r=surkov

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e543c8a8931
https://hg.mozilla.org/mozilla-central/rev/43dd97a57edd