Closed Bug 514073 Opened 15 years ago Closed 15 years ago

[@ XPCIDispatchExtension::Enumerate]

Categories

(Core :: XPConnect, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: timeless, Assigned: timeless)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Severity: normal → critical
Attached patch patch (obsolete) — Splinter Review
this seems to be the logical patch
Attachment #398023 - Flags: review?(mrbkap)
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.
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?
Blocks: 506961
Attached patch revised patch Splinter Review
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?
Attachment #398023 - Attachment is obsolete: true
Attachment #398023 - Flags: review?(mrbkap)
Attachment #399841 - Attachment is patch: true
Attachment #399841 - Attachment mime type: application/octet-stream → text/plain
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).
Attachment #399841 - Flags: review?(mrbkap)
Attachment #399841 - Flags: review?(mrbkap) → review+
Well I will now have to create a real test case. I tripped an access crash again.
Whiteboard: [firebug-p1]
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?
(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.
this has been reviewed. Is it ready to land on trunk/branch?
Flags: wanted1.9.2?
Whiteboard: [firebug-p1] → [firebug-p1][needs-checkin]
I think someone should determine if this patch fixes something. I'm not it, since I don't understand what is going on here.
Whiteboard: [firebug-p1][needs-checkin] → [needs-checkin]
hopefully it'll fix a crash, if we weren't doing those checks before.
Keywords: checkin-needed
Whiteboard: [needs-checkin]
http://hg.mozilla.org/mozilla-central/rev/c0a5b63c3f3d
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Crash Signature: [@ XPCIDispatchExtension::Enumerate]
clearing old flags.
Flags: wanted1.9.2?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: