209965 - XPConnect IDispatch support causes sporadic crashes during garbage collection

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Stef Walter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030618

SHORT VERSION

Currently the IDispatch support has a small bug which causes an application
crash. I won't pretend to understand it fully. It occurs during garbage
collection and is difficult to reproduce. I've included a patch which fixes it
however.

Basically, the XPCWrappedNativeTearOff::SetIDispatch can fail, and should have a
return type to reflect that. Additionally the return value should be checked at
every instance that XPCWrappedNativeTearOff::SetIDispatch is called.

LONG VERSION

I'm writing a large program based on Mozilla and XPCOM. After updating the
mozilla base to 1.4 the app started crashing mysteriously at certain spots. The
bug is in the IDispatch support of the XPConnect code.

The application would crash during javascript garbage collection when trying to
free a XPCWrappedNativeTearOff that had already been freed. Here's why:

- Prior to the crash XP Connect tries to instantiate a XPCWrappedNativeTearOff 
  object for IDispatch interface . I'm not sure why it does this. Possibly 
  looking for a property or method that isn't present on the native object's
  interfaces.
- When creating the object, it calls XPCWrappedNativeTearOff::SetIDispatch which 
  calls XPCDispInterface::NewInstance which fails for some reason. Leaving the 
  XPCWrappedNativeTearOff marked as an IDispatch type object (see IsIDispatch),
but with an null XPCDispInterface pointer.
- Later on when XPCWrappedNativeTearOff::SetJSObject is called since the object is 
  marked as a IDispatch type, but without a valid interface pointer the JSObject 
  isn't set, and the reference counting screws up. 
- And when garbage collection comes around, boom insta crash.

It would difficult for me to make a testcase for this issue, as it has to do
with garbage collection, and my app is very large. But I think based on this
logic, I can demonstrate that the patches have merit:

- XPCDispInterface::NewInstance can fail, so anything that calls it should have 
  some way of dealing with the failure.
- XPCWrappedNativeTearOff::SetIDispatch currently does not deal with that failure
  and this needs to be remedied by changing the return type etc...
- Anything that calls XPCWrappedNativeTearOff::SetIDispatch needs to take this 
  change into account.

The patches included do exactly that.

BTW, the app is an 'Access type' IDE for LDAP. It'll be open source once a beta
is reached and license issues are resolved. Currently this is a show stopper for
this project working with the Gecko SDK or any unpatched version of Mozilla.



Reproducible: Sometimes

Steps to Reproduce:
As noted above the issue is a complicated one, difficult to reproduce in a test
case. However it's pretty simple to fix, and a patch has been included.
Actual Results:  
Mozilla crashes randomly.

Expected Results:  
Not have crashed.

Comment 1

[Stef Walter]

Attachment: Patch to fix the issue

Comment 2

[David Bradley]

Thanks for the patch.

>Prior to the crash XP Connect tries to instantiate a XPCWrappedNativeTearOff
>object for IDispatch interface . I'm not sure why it does this.

Are you working with any COM objects at all? If you are not, then this should
never be getting called. From the behavior it sounds like something is reusing
the IDispatch IID and providing something that isn't an IDispatch interface and
that's causing the code down line to get an error.

Comment 3

[Stef Walter]

No, not using COM at all. I have no idea why it's calling an IDispatch. As far
as I recall it's being called from JS property code (ummmm.... JS_DefineProperty
or something like that). I'll get you a call stack in a bit. 

Comment 4

[Stef Walter]

Attachment: Call stack to XPCWrappedNativeTearOff::SetIDispatch

Comment 5

[David Bradley]

The fix for bug 205768 may have fixed or changed the behavior. This fix went in
June 5th on the trunk and from your stack, you build appears to be before that.

The fix in Bug 205768 changed the resolving from DefinePropertyIfFound to using
a scriptable helper and was done to fix the broken enumeration of wrapped
natives wrapping IDispatch interface. That fix may have inadvertantly fixed this
problem. It definitely put the implementation more in line with the architecture
of XPConnect's class info and scriptable helpers.

Comment 6

[Stef Walter]

Yes, it seems to have fixed the problem. I'll test this issue thoroughly it over
the next few days. 

In addition my patch isn't perfect. The invalid IDispatch Wrapper is still in
some sort of cache even though a failure is returned from
XPCWrappedNative::InitTearOff.

Comment 7

[David Bradley]

Thanks for taking the time to check it out. I'm going to resolve this as fixed,
since bug 205768 appears to have fixed this problem. Please reopen if you
encounter it in builds after June 5th.

Comment 8

[Phil Schwartau]

Rubber-stamp vrfy; Nate, please reopen if not fixed -

Comment 9

[Stef Walter]

I'd like to confirm. Haven't had any further problems with this particular issue.