514073 - [@ XPCIDispatchExtension::Enumerate]

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[timeless]

Signature	XPCIDispatchExtension::Enumerate(XPCCallContext&, JSObject*, XPCWrappedNative*)
UUID	5d4e899f-0eb6-47f4-8ec4-560c82090831
Time 	2009-08-31 17:34:45.368239
Uptime	85
Last Crash	6918727 seconds before submission
Product	Firefox
Version	3.5.2
Build ID	20090729225027
Branch	1.9.1
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 15 model 2 stepping 7
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x4
User Comments	jsdIValueFromXMLHttpRequest
Processor Notes 	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	XPCIDispatchExtension::Enumerate 	js/src/xpconnect/src/XPCIDispatchExtension.cpp:332
1 	xul.dll 	XPC_WN_Shared_Enumerate 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:614
2 	js3250.dll 	js_Enumerate 	js/src/jsobj.cpp:4859
3 	js3250.dll 	js_DefineNativeProperty 	js/src/jsobj.cpp:3737

This is slightly odd, I remember analyzing this earlier last week, but i can only find one instance of this crash.

Comment 1

[timeless]

Attachment: patch

this seems to be the logical patch

Comment 2

[John J. Barton]

Hmm.. that patch does not compile for me on trunk:
 'IsIDispatch' : is not a member of 'XPCDispInterface'
and I could not find IdIDispatch() in mxr.

Comment 3

[John J. Barton]

Any suggestions? Do I need to build a different way? I see calls to IsIDispatch are guarded by XPC_IDISPATCH_SUPPORT:

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappednative.cpp#1419
1418 #ifdef XPC_IDISPATCH_SUPPORT
1419                 if(to->IsIDispatch())
1420                     delete to->GetIDispatchInfo();
1421 #endif

Is that macro normally true?

Comment 4

[John J. Barton]

Attachment: revised patch

Based on a suggestion by Mook, I modified the patch slightly. The code compiles and does not crash. Thanks!

How can we move this forward?

Comment 5

[James Ross]

Comment on attachment 399841 [details] [diff] [review]
revised patch 

Asking for review seems good (if the comment in the attachment means you've already had it reviewed, + the review and move on to asking for SR).

Comment 6

[John J. Barton]

Well I will now have to create a real test case. I tripped an access crash again.

Comment 7

[John J. Barton]

I think the problem here is not solved by the patch.

Here is the stack at the crash, we are in javascript calling
  for( var p in x)

 	xpc3250.dll!XPCIDispatchExtension::Enumerate(XPCCallContext &, JSObject *, XPCWrappedNative *)  Line 334	C++
>	xpc3250.dll!XPC_WN_Shared_Enumerate(JSContext *, JSObject *)  Line 658	C++
 	js3250.dll!js_Enumerate(JSContext *, JSObject *, JSIterateOp, int *, int *)  Line 4881	C++
 	xpc3250.dll!XPC_WN_JSOp_Enumerate(JSContext *, JSObject *, JSIterateOp, int *, int *)  Line 1283	C++
 	js3250.dll!JSObject::enumerate(JSContext *, JSIterateOp, int *, int *)  Line 211	C++
 	js3250.dll!JS_Enumerate(JSContext *, JSObject *)  Line 3949	C++
 	xpc3250.dll!XPCWrapper::Enumerate(JSContext *, JSObject *, JSObject *)  Line 294	C++
 	xpc3250.dll!XPCWrapper::CreateIteratorObj(JSContext *, JSObject *, JSObject *, JSObject *, int)  Line 216	C++
 	xpc3250.dll!XPC_SJOW_Iterator(JSContext *, JSObject *, int)  Line 1004	C++
 	js3250.dll!js_ValueToIterator(JSContext *, unsigned int, int *)  Line 371	C++
 	js3250.dll!js_Interpret(JSContext *)  Line 461	C++

Our caller (second frame) has this code:

#ifdef XPC_IDISPATCH_SUPPORT
        if(iface->GetIID()->Equals(NSID_IDISPATCH))
        {
            XPCIDispatchExtension::Enumerate(ccx, obj, wrapper);
            continue;
        }
#endif

which I think says that the test we added in the patch is pointless: we've already made the test before the call. So inside the crashing frame we test again, get the same wrong answer and crash.

So the bug here is not in the enumerate code but rather the object we are enumerating has lied to us.  

That actually makes a lot of sense, because we got this object from a jsd function call that may not have been used since the wrapper stuff came about.

Does that make sense?

Comment 8

[John J. Barton]

(In reply to comment #4)
> Created an attachment (id=399841) [details]
> revised patch 
> 
> Based on a suggestion by Mook, I modified the patch slightly. The code compiles
> and does not crash. Thanks!

I just want to clarify that I ran the wrong test, so the "does not crash" was not for the case that crashed before. This crash is deterministic.

Comment 9

[Rob Campbell [:rc] (:robcee)]

this has been reviewed. Is it ready to land on trunk/branch?

Comment 10

[John J. Barton]

I think someone should determine if this patch fixes something. I'm not it, since I don't understand what is going on here.

Comment 11

[Rob Campbell [:rc] (:robcee)]

hopefully it'll fix a crash, if we weren't doing those checks before.

Comment 12

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/c0a5b63c3f3d

Comment 13

[Rob Campbell [:rc] (:robcee)]

clearing old flags.