Closed
Bug 522590
Opened 15 years ago
Closed 15 years ago
FF 3.7 crash on startup with chromebug
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: johnjbarton, Assigned: jorendorff)
References
Details
(Whiteboard: [firebug-p1][firebug-blocks] fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
6.38 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
I built FF 3.7 this way: $ hg qpop -a patch queue now empty bartonjj@ENKI-PCSHIH /g/mozilla/mozilla-central/src $ hg update -v a7a756bc341b resolving manifests 0 files updated, 0 files merged, 0 files removed, 0 files unresolved bartonjj@ENKI-PCSHIH /g/mozilla/mozilla-central/src $ hg qpush -v (working directory not at tip) applying bugzilla-338224 patching file nsprpub/config/rules.mk nsprpub/config/rules.mk now at: bugzilla-338224 bartonjj@ENKI-PCSHIH /g/mozilla/mozilla-central/src $ hg qpush -v applying 508562-lessStrict patching file dom/base/nsJSEnvironment.cpp patching file modules/libpref/src/init/all.js dom/base/nsJSEnvironment.cpp modules/libpref/src/init/all.js now at: 508562-lessStrict ----------------------------------------- Then installed http://getfirebug.com/releases/firebug/1.5X/firebug-1.5X.0a26.xpi http://getfirebug.com/releases/chromebug/chromebug-1.5.0a4.xpi Then ran with command line flag -chromebug. I get one assert about Lucida font, then crash, no crash reporter. ---------------------- VC++ halts here first: ntdll.dll!_DbgBreakPoint@0() > xpcom_core.dll!Break(const char *) Line 489 C++ xpcom_core.dll!NS_DebugBreak_P(unsigned int, const char *, const char *, const char *, int) Line 354 C++ thebes.dll!FontFamily::FindStyleVariations() Line 289 C++ thebes.dll!gfxWindowsPlatform::RunLoader() Line 962 C++ thebes.dll!gfxFontInfoLoader::LoaderTimerFire() Line 675 C++ thebes.dll!gfxFontInfoLoader::LoaderTimerCallback(nsITimer *, void *) Line 666 C++ xpcom_core.dll!nsTimerImpl::Fire() Line 427 C++ xpcom_core.dll!nsTimerEvent::Run() Line 521 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int, int *) Line 527 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread *, int) Line 230 C++ gkwidget.dll!nsBaseAppShell::Run() Line 170 C++ toolkitcomps.dll!nsAppStartup::Run() Line 182 C++ ------------------------- Then it halts here: ntdll.dll!_DbgBreakPoint@0() > mozjs.dll!JS_Assert(const char *, const char *, int) Line 65 C++ mozjs.dll!fun_resolve(JSContext *, JSObject *, int, unsigned int, JSObject * *) Line 1430 C++ mozjs.dll!js_LookupPropertyWithFlags(JSContext *, JSObject *, int, unsigned int, JSObject * *, JSProperty * *) Line 3967 C++ mozjs.dll!js_LookupProperty(JSContext *, JSObject *, int, JSObject * *, JSProperty * *) Line 3895 C++ mozjs.dll!JSObject::lookupProperty(JSContext *, int, JSObject * *, JSProperty * *) Line 256 C++ mozjs.dll!fun_enumerate(JSContext *, JSObject *) Line 1394 C++ mozjs.dll!js_Enumerate(JSContext *, JSObject *, JSIterateOp, int *, int *) Line 5070 C++ mozjs.dll!JSObject::enumerate(JSContext *, JSIterateOp, int *, int *) Line 294 C++ mozjs.dll!InitNativeIterator(JSContext *, JSObject *, JSObject *, unsigned int) Line 168 C++ mozjs.dll!js_ValueToIterator(JSContext *, unsigned int, int *) Line 416 C++ mozjs.dll!js_Interpret(JSContext *) Line 486 C++ mozjs.dll!js_Invoke(JSContext *, unsigned int, int *, unsigned int) Line 1373 C++ mozjs.dll!js_fun_apply(JSContext *, unsigned int, int *) Line 2046 C++ mozjs.dll!js_Interpret(JSContext *) Line 2269 C++ mozjs.dll!js_Invoke(JSContext *, unsigned int, int *, unsigned int) Line 1373 C++ xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *, unsigned short, const XPTMethodDescriptor *, nsXPTCMiniVariant *) Line 1696 C++ xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short, const XPTMethodDescriptor *, nsXPTCMiniVariant *) Line 571 C++ xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase *, unsigned int, unsigned int *, unsigned int *) Line 114 C++ xpcom_core.dll!SharedStub() Line 142 C++ jsd3250.dll!jsds_ExecutionHookProc(JSDContext *, JSDThreadState *, unsigned int, void *, int *) Line 683 C++ jsd3250.dll!jsds_ExecutionHookProc(JSDContext *, JSDThreadState *, unsigned int, void *, int *) Line 683 C++ jsd3250.dll!jsd_CallExecutionHook(JSDContext *, JSContext *, unsigned int, unsigned int (JSDContext *, JSDThreadState *, unsigned int, void *, int *)*, void *, int *) Line 177 C jsd3250.dll!jsd_DebugErrorHook(JSContext *, const char *, JSErrorReport *, void *) Line 371 C mozjs.dll!js_ReportErrorAgain(JSContext *, const char *, JSErrorReport *) Line 1650 C++ mozjs.dll!ReportError(JSContext *, const char *, JSErrorReport *) Line 1278 C++ mozjs.dll!js_ReportErrorNumberVA(JSContext *, unsigned int, const JSErrorFormatString * (void *, const char *, const unsigned int)*, void *, const unsigned int, int, char *) Line 1605 C++ mozjs.dll!JS_ReportErrorNumber(JSContext *, const JSErrorFormatString * (void *, const char *, const unsigned int)*, void *, const unsigned int, ...) Line 5546 C++ mozjs.dll!js_ReportUncaughtException(JSContext *) Line 1307 C++ mozjs.dll!JS_ReportPendingException(JSContext *) Line 5760 C++ xpc3250.dll!nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject(XPCCallContext &, JSObject *, const nsID &) Line 371 C++ xpc3250.dll!nsXPCWrappedJSClass::DelegatedQueryInterface(nsXPCWrappedJS *, const nsID &, void * *) Line 797 C++ xpc3250.dll!nsXPCWrappedJS::QueryInterface(const nsID &, void * *) Line 186 C++ xpcom_core.dll!nsXPTCStubBase::QueryInterface(const nsID &, void * *) Line 54 C++ xpcom_core.dll!nsQueryInterface::operator()(const nsID &, void * *) Line 47 C++ xpc3250.dll!nsCOMPtr<nsIClassInfo>::assign_from_qi(nsQueryInterface, const nsID &) Line 1189 C++ xpc3250.dll!nsCOMPtr<nsIClassInfo>::operator=(nsQueryInterface) Line 659 C++ xpc3250.dll!XPCWrappedNative::GetNewOrUsed(XPCCallContext &, nsISupports *, XPCWrappedNativeScope *, XPCNativeInterface *, nsWrapperCache *, int, XPCWrappedNative * *) Line 404 C++ xpc3250.dll!XPCConvert::NativeInterface2JSObject(XPCLazyCallContext &, int *, nsIXPConnectJSObjectHolder * *, nsISupports *, const nsID *, XPCNativeInterface * *, nsWrapperCache *, JSObject *, int, int, unsigned int *) Line 1202 C++ xpc3250.dll!NativeInterface2JSObject(XPCLazyCallContext &, JSObject *, nsISupports *, const nsID *, int, int *, nsIXPConnectJSObjectHolder * *) Line 1226 C++ xpc3250.dll!nsXPConnect::WrapNative(JSContext *, JSObject *, nsISupports *, const nsID &, nsIXPConnectJSObjectHolder * *) Line 1257 C++ xpc3250.dll!nsJSCID::GetService(nsISupports * *) Line 901 C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports *, unsigned int, unsigned int, nsXPTCVariant *) Line 102 C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext &, XPCWrappedNative::CallMode) Line 2690 C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext &, XPCWrappedNative::CallMode) Line 2690 C++ xpc3250.dll!XPC_WN_CallMethod(JSContext *, JSObject *, unsigned int, int *, int *) Line 1756 C++ mozjs.dll!js_Invoke(JSContext *, unsigned int, int *, unsigned int) Line 1365 C++ mozjs.dll!js_Interpret(JSContext *) Line 2303 C++ mozjs.dll!js_Invoke(JSContext *, unsigned int, int *, unsigned int) Line 1373 C++ mozjs.dll!js_InternalInvoke(JSContext *, JSObject *, int, unsigned int, unsigned int, int *, int *) Line 1428 C++ mozjs.dll!js_InternalGetOrSet(JSContext *, JSObject *, int, int, JSAccessMode, unsigned int, int *, int *) Line 1464 C++ mozjs.dll!JSScopeProperty::get(JSContext *, JSObject *, JSObject *, int *) Line 781 C++ mozjs.dll!js_NativeGet(JSContext *, JSObject *, JSObject *, JSScopeProperty *, unsigned int, int *) Line 4281 C++ mozjs.dll!js_GetPropertyHelper(JSContext *, JSObject *, int, unsigned int, int *) Line 4462 C++ mozjs.dll!js_Interpret(JSContext *) Line 1556 C++ mozjs.dll!js_Invoke(JSContext *, unsigned int, int *, unsigned int) Line 1373 C++ mozjs.dll!js_InternalInvoke(JSContext *, JSObject *, int, unsigned int, unsigned int, int *, int *) Line 1428 C++ mozjs.dll!JS_CallFunctionValue(JSContext *, JSObject *, int, unsigned int, int *, int *) Line 5096 C++ gklayout.dll!nsJSContext::CallEventHandler(nsISupports *, void *, void *, nsIArray *, nsIVariant * *) Line 2096 C++ gklayout.dll!nsGlobalWindow::RunTimeout(nsTimeout *) Line 8032 C++ gklayout.dll!nsGlobalWindow::TimerCallback(nsITimer *, void *) Line 8369 C++ xpcom_core.dll!nsTimerImpl::Fire() Line 427 C++ xpcom_core.dll!nsTimerEvent::Run() Line 521 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int, int *) Line 527 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread *, int) Line 230 C++ gkwidget.dll!nsBaseAppShell::Run() Line 170 C++ toolkitcomps.dll!nsAppStartup::Run() Line 182 C++ xul.dll!XRE_main(int, char * *, const nsXREAppData *) Line 3420 C++ firefox.exe!NS_internal_main(int, char * *) Line 156 C++ firefox.exe!wmain(int, unsigned short * *) Line 110 C++ firefox.exe!__tmainCRTStartup() Line 583 C firefox.exe!wmainCRTStartup() Line 403 C kernel32.dll!_BaseProcessStart@4() ---- Then the app exits.
Reporter | ||
Comment 1•15 years ago
|
||
The last bit of the OS console in case it helps: WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file g:/mozill a/mozilla-central/src/content/base/src/nsFrameLoader.cpp, line 193 pldhash: for the table at address 034F1AB8, the given entrySize of 48 probably f avors chaining over double hashing. ++DOCSHELL 034F1A50 == 12 ++DOMWINDOW == 17 (02DC5550) [serial = 17] [outer = 00000000] WARNING: Subdocument container has no frame: file g:/mozilla/mozilla-central/src /layout/base/nsDocumentViewer.cpp, line 2340 ++DOMWINDOW == 18 (09F90BC8) [serial = 18] [outer = 05E4F580] WARNING: Subdocument container has no frame: file g:/mozilla/mozilla-central/src /layout/base/nsDocumentViewer.cpp, line 2340 ++DOMWINDOW == 19 (0A02B710) [serial = 19] [outer = 05E2B108] ++DOMWINDOW == 20 (0A00A0F0) [serial = 20] [outer = 05DA0AF0] ++DOMWINDOW == 21 (0A08D1A0) [serial = 21] [outer = 05F276F0] ++DOMWINDOW == 22 (0A09DBB8) [serial = 22] [outer = 02DC5520] ChromeBugOpener: no element with id='menu_OpenChromeBugAlways' window moved to offscreen position ++DOMWINDOW == 23 (07FE4FE0) [serial = 23] [outer = 02DC5520] Assertion failure: !js_IsInternalFunctionObject(obj), at g:/mozilla/mozilla-cent ral/src/js/src/jsfun.cpp:1430
Whiteboard: [firebug-p1]
![]() |
||
Comment 2•15 years ago
|
||
JS_Assert is fatal if the assert fails; that's what you're seeing here. What's |obj| on that assertion failure in jsfun.cpp? Specifically, what's the output of calling js_DumpObject(obj) there?
Comment 3•15 years ago
|
||
In that topmost js_Interpret, what was script->filename/script->lineno? Best to call js_FramePCToLineNumber(cx,fp) there too. /be
Assignee | ||
Comment 4•15 years ago
|
||
This assertion was added in bug 518103, here: http://hg.mozilla.org/mozilla-central/rev/e6f2cbdfce26 It asserts, roughly, that nobody does SetProperty(funobj, "prototype", x) where funobj is an InternalFunctionObject. Scripts can't do it because such objects are never exposed to scripts. Unfortunately I think they are exposed via the JSAPI when you do JS_LookupProperty to get a slot value without calling the getter. Not sure about that, but if so, we made a breaking change to the API by adding this rule. Unintentional I think. In any case, that's not the root cause of this bug. fun_enumerate is calling js_LookupProperty without providing flags, when cx->resolveFlags happens to have JSPROP_ASSIGNING. That is a bug for sure, but I don't think that's the root cause either. It seems to me that if we get into fun_enumerate with an InternalFunctionObject, we probably have already gone wrong. I wonder how we got here. jjb, can you dump the JS stack? js_DumpStackFrame(cx->fp)
Blocks: 471214
Reporter | ||
Comment 5•15 years ago
|
||
(In reply to comment #2) >... Specifically, what's the > output of calling js_DumpObject(obj) there? (In reply to comment #3) >... call js_FramePCToLineNumber(cx,fp) there too. (In reply to comment #4) > I wonder how we got here. jjb, can you dump the JS stack? > js_DumpStackFrame(cx->fp) I don't know what you want me to do in these cases. Edit the source to call these functions somewhere and run the program?
![]() |
||
Comment 6•15 years ago
|
||
No, call those functions from the debugger while the relevant stack frame is set as the current one. How you do that depends on the debugger you're using.
Reporter | ||
Comment 7•15 years ago
|
||
js_DumpObject(obj): object 068E0540 class 006EDD18 Function properties: proto <unnamed function at 027C1770 (JSFunction at 027C1770)> parent <Call object at 068E0520> private 068E1850 slots: 3 (reserved) = undefined 4 (reserved) = undefined -------------------------------------------------- js_DumpStackFrame(cx->fp) JSStackFrame at 03099B00 <function parseParts at 068E0580 (JSFunction at 068E18C0)> file chrome://fb4cb/content/domplate.js line 688 pc = 0351D546 current op: new slots: 03099B60 sp: 03099B94 = slots + 13 03099B60: <RegExp object at 08FE15E0> 03099B64: 0 03099B68: <Array object at 08FE1600> 03099B6C: <Array object at 08FE1620> 03099B70: "" 03099B74: <Array object at 08FE1640> 03099B78: undefined 03099B7C: <function push at 027C2540 (JSFunction at 027C2540)> 03099B80: <Array object at 08FE1600> 03099B84: <function Variable at 068E0540 (JSFunction at 068E1850)> 03099B88: null 03099B8C: "pageTitle" 03099B90: <Array object at 08FE1660> argv: 03099AF0 (argc: 1) this: <Call object at 068E0520> rval: undefined flags: none scopeChain: (JSObject *) 068E0520 displaySave: (JSStackFrame *) 03099A88 JSStackFrame at 03099A88 <function parseValue at 068E05A0 (JSFunction at 068E18F8)> file chrome://fb4cb/content/domplate.js line 716 pc = 0351DD62 current op: call slots: 03099AE8 sp: 03099AF4 = slots + 3 03099AE8: <function parseParts at 068E0580 (JSFunction at 068E18C0)> 03099AEC: <Call object at 068E0520> 03099AF0: "$pageTitle" argv: 03099A74 (argc: 1) this: <Call object at 068E0520> rval: undefined flags: none scopeChain: (JSObject *) 068E0520 displaySave: (JSStackFrame *) 03099A04 JSStackFrame at 03099A04 <function parseChildren at 068E05C0 (JSFunction at 068E1930)> file chrome://fb4cb/content/domplate.js line 721 pc = 0351E4DA current op: call slots: 03099A64 sp: 03099A78 = slots + 5 03099A64: 0 03099A68: undefined 03099A6C: <function parseValue at 068E05A0 (JSFunction at 068E18F8)> 03099A70: <Call object at 068E0520> 03099A74: "$pageTitle" argv: 030999E8 (argc: 4) this: <Call object at 068E0520> rval: undefined flags: none scopeChain: (JSObject *) 068E0520 displaySave: (JSStackFrame *) 03099978 JSStackFrame at 03099978 <unnamed function at 068E07E0 (JSFunction at 068E1188)> file chrome://fb4cb/content/domplate.js line 67 pc = 035274FF current op: call slots: 030999D8 sp: 030999F8 = slots + 8 030999D8: "$pageTitle" 030999DC: false 030999E0: <function parseChildren at 068E05C0 (JSFunction at 068E1930)> 030999E4: <Call object at 068E0520> 030999E8: <Object object at 08FE1540> 030999EC: 0 030999F0: <Array object at 08FE15C0> 030999F4: <Array object at 08FE15A0> argv: 03099968 (argc: 1) this: <Object at 08FE1520> rval: undefined flags: computed_this scopeChain: (JSObject *) 068E0520 displaySave: (JSStackFrame *) 0309978C JSStackFrame at 030998FC <function anonymous at 068F2578 (JSFunction at 068F2578)> file chrome://fb4cb/content/domplate.js line 1077 pc = 035153DA current op: call slots: 0309995C sp: 0309996C = slots + 4 0309995C: <Object at 08FE1520> 03099960: <unnamed function at 068E07E0 (JSFunction at 068E1188)> 03099964: <Object at 08FE1520> 03099968: <Object object at 08FE1540> argv: 0309986C (argc: 1) argsobj: <Object object at 08FE1540> this: <With object at 08FE30C0> rval: undefined flags: none scopeChain: (JSObject *) 02C52420 displaySave: (JSStackFrame *) 0012F204 JSStackFrame at 0012EAAC <unnamed function at 06912640 (JSFunction at 025B30E0)> file chrome://fb4cb/content/firebug.js line 3 pc = 05CDA25C current op: call slots: 03099834 sp: 03099870 = slots + 15 03099834: <With object at 08FE30C0> 03099838: <unnamed function at 08FE14A0 (JSFunction at 025C31F8)> 0309983C: <unnamed function at 068E0740 (JSFunction at 068E10A8)> 03099840: <ChromeWindow object at 02C52420> 03099844: <Object at 08FE2E40> 03099848: <Object at 08FE14C0> 0309984C: <function anonymous at 068F2150 (JSFunction at 068F2150)> 03099850: <With object at 08FE30C0> 03099854: <Object at 08FE14E0> 03099858: <function anonymous at 068F21F8 (JSFunction at 068F21F8)> 0309985C: <With object at 08FE30C0> 03099860: <Object at 08FE1500> 03099864: <function anonymous at 068F2578 (JSFunction at 068F2578)> 03099868: <With object at 08FE30C0> 0309986C: "$pageTitle" argv: 03099834 (argc: 0) callobj: <Call object at 08FE30A0> varobj: <Call object at 08FE30A0> this: <Object at 06912660> rval: undefined flags: rooted_argv scopeChain: (JSObject *) 08FE30C0 JSStackFrame at 0309978C <unnamed function at 068D2040 (JSFunction at 05861C78)> file chrome://fb4cb/content/lib.js line 63 pc = 0591034C current op: apply slots: 030997EC sp: 03099804 = slots + 6 030997EC: 2 030997F0: <unnamed function at 06912640 (JSFunction at 025B30E0)> 030997F4: <Object at 06912660> 030997F8: <function apply at 027C1850 (JSFunction at 027C1850)> 030997FC: <unnamed function at 06912640 (JSFunction at 025B30E0)> 03099800: <Object at 06912660> argv: 03099774 (argc: 0) this: <function fbXPCOMUtils at 02DA1280 (JSFunction at 02521700)> rval: undefined flags: none scopeChain: (JSObject *) 03623DC0 displaySave: (JSStackFrame *) 030996F4 JSStackFrame at 030996F4 <unnamed function at 035C0180 (JSFunction at 05863118)> file chrome://fb4cb/content/chrome.js line 74 pc = 0343F7EE current op: call slots: 03099754 sp: 03099774 = slots + 8 03099754: <Object at 06CA1B20> 03099758: undefined 0309975C: undefined 03099760: undefined 03099764: undefined 03099768: undefined 0309976C: <unnamed function at 068D2040 (JSFunction at 05861C78)> 03099770: <function fbXPCOMUtils at 02DA1280 (JSFunction at 02521700)> argv: 030996DC (argc: 0) this: <Object at 035C0140> rval: undefined flags: none scopeChain: (JSObject *) 035C00C0 displaySave: (JSStackFrame *) 03099674 JSStackFrame at 03099674 <unnamed function at 035C0160 (JSFunction at 058630E0)> file chrome://fb4cb/content/chrome.js line 55 pc = 0343EA42 current op: call slots: 030996D4 sp: 030996DC = slots + 2 030996D4: <unnamed function at 035C0180 (JSFunction at 05863118)> 030996D8: <Object at 035C0140> argv: 03099648 (argc: 1) this: <Object at 035C0140> rval: undefined flags: computed_this scopeChain: (JSObject *) 035C00C0 JSStackFrame at 0012F204 <function at 06CA1AC0 (JSFunction at 08F52E38)> file chrome://firebug/content/bindings.xml line 32 pc = 075D39F5 current op: call slots: 03099640 sp: 0309964C = slots + 3 03099640: <unnamed function at 035C0160 (JSFunction at 058630E0)> 03099644: <Object at 035C0140> 03099648: <XULElement object at 08F503C0> argv: 03099640 (argc: 0) this: <XULElement object at 08F503C0> rval: undefined flags: computed_this rooted_argv scopeChain: (JSObject *) 08F503C0 -------------------------------------------- js_FramePCToLineNumber(cx, cx->fp) 0x000002be all in mozjs.dll!fun_resolve(JSContext *, JSObject *, int, unsigned int, JSObject * *) Line 1430
Reporter | ||
Comment 8•15 years ago
|
||
Looks like from the JS stack that we are here: http://code.google.com/p/fbug/source/browse/branches/firebug1.5/content/firebug/domplate.js#702 and 0x02be == 702
Reporter | ||
Comment 9•15 years ago
|
||
No, forget those last two posts.
Reporter | ||
Comment 10•15 years ago
|
||
Ok below is the js call stack at the point of assert, looks like we are here: http://code.google.com/p/fbug/source/browse/branches/firebug1.5/content/firebug/lib.js#225 Assertion failure: !js_IsInternalFunctionObject(obj), at g:/mozilla/mozilla-cen ral/src/js/src/jsfun.cpp:1430 JSStackFrame at 07FFFB1C <unnamed function at 068E0E20 (JSFunction at 058820E0)> file chrome://fb4cb/content/lib.js line 206 pc = 035BD321 current op: iter slots: 07FFFB7C sp: 07FFFB88 = slots + 3 07FFFB7C: "[" 07FFFB80: undefined 07FFFB84: <Iterator object at 02380780> argv: 07FFFB0C (argc: 1) this: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> rval: undefined flags: none scopeChain: (JSObject *) 03641CC0 displaySave: (JSStackFrame *) 07FFFA94 JSStackFrame at 07FFFA94 <unnamed function at 068F2DA0 (JSFunction at 05893070)> file chrome://fb4cb/content/lib.js line 2363 pc = 058C5A8A current op: call slots: 07FFFAF4 sp: 07FFFB10 = slots + 7 07FFFAF4: undefined 07FFFAF8: undefined 07FFFAFC: <function exec at 068F3070 (JSFunction at 068F3070)> 07FFFB00: <RegExp object at 068E0C60> 07FFFB04: <unnamed function at 068E0E20 (JSFunction at 058820E0)> 07FFFB08: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> 07FFFB0C: <unnamed function at 06B10188 (JSFunction at 06B10188)> argv: 07FFFA84 (argc: 1) this: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> rval: undefined flags: computed_this scopeChain: (JSObject *) 03641CC0 displaySave: (JSStackFrame *) 07FFFA04 JSStackFrame at 07FFFA04 <unnamed function at 068F2DC0 (JSFunction at 058930A8)> file chrome://fb4cb/content/lib.js line 2375 pc = 058F428F current op: call slots: 07FFFA64 sp: 07FFFA88 = slots + 9 07FFFA64: <Array object at 02380740> 07FFFA68: undefined 07FFFA6C: undefined 07FFFA70: undefined 07FFFA74: undefined 07FFFA78: undefined 07FFFA7C: <unnamed function at 068F2DA0 (JSFunction at 05893070)> 07FFFA80: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> 07FFFA84: <unnamed function at 06B10188 (JSFunction at 06B10188)> argv: 07FFF9E8 (argc: 2) this: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> rval: undefined flags: computed_this scopeChain: (JSObject *) 03641CC0 displaySave: (JSStackFrame *) 07FFF974 JSStackFrame at 07FFF974 <unnamed function at 024A12C0 (JSFunction at 06920038)> file chrome://fb4cb/content/sourceFile.js line 347 pc = 05AA3C7D current op: call slots: 07FFF9D4 sp: 07FFF9F0 = slots + 7 07FFF9D4: undefined 07FFF9D8: <unnamed function at 06B10188 (JSFunction at 06B10188)> 07FFF9DC: undefined 07FFF9E0: <unnamed function at 068F2DC0 (JSFunction at 058930A8)> 07FFF9E4: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> 07FFF9E8: <unnamed function at 06B10188 (JSFunction at 06B10188)> 07FFF9EC: <XPCWrappedNative_NoHelper object at 02381D00> argv: 07FFF94C (argc: 3) this: <Object at 02380640> rval: undefined flags: none scopeChain: (JSObject *) 024A2EE0 displaySave: (JSStackFrame *) 07FFF8C8 JSStackFrame at 07FFF8C8 <unnamed function at 068F2B60 (JSFunction at 05880B28)> file chrome://fb4cb/content/lib.js line 1997 pc = 058E7AF9 current op: call slots: 07FFF928 sp: 07FFF958 = slots + 12 07FFF928: undefined 07FFF92C: <Object at 06C424E0> 07FFF930: <String object at 06C42500> 07FFF934: <Object at 02380640> 07FFF938: 128 07FFF93C: undefined 07FFF940: undefined 07FFF944: <unnamed function at 024A12C0 (JSFunction at 06920038)> 07FFF948: <Object at 02380640> 07FFF94C: <XPCWrappedNative_NoHelper object at 02381E00> 07FFF950: <Object at 02380E20> 07FFF954: <XPCWrappedNative_NoHelper object at 02381D00> argv: 07FFF89C (argc: 2) this: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> rval: undefined flags: none scopeChain: (JSObject *) 03641CC0 displaySave: (JSStackFrame *) 07FFF824 JSStackFrame at 07FFF824 <unnamed function at 068F2B40 (JSFunction at 05880AF0)> file chrome://fb4cb/content/lib.js line 1967 pc = 058E71C3 current op: call slots: 07FFF884 sp: 07FFF8A4 = slots + 8 07FFF884: <Object at 023803A0> 07FFF888: undefined 07FFF88C: undefined 07FFF890: undefined 07FFF894: <unnamed function at 068F2B60 (JSFunction at 05880B28)> 07FFF898: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> 07FFF89C: <XPCWrappedNative_NoHelper object at 02381D00> 07FFF8A0: <Object at 02380E20> argv: 07FFF804 (argc: 2) this: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> rval: undefined flags: computed_this scopeChain: (JSObject *) 03641CC0 displaySave: (JSStackFrame *) 07FFF790 JSStackFrame at 07FFF790 <unnamed function at 08F53760 (JSFunction at 06CE35E8)> file chrome://fb4cb/content/debugger.js line 1004 pc = 068080DA current op: call slots: 07FFF7F0 sp: 07FFF80C = slots + 7 07FFF7F0: <Object at 02380E20> 07FFF7F4: undefined 07FFF7F8: <Object at 024703C0> 07FFF7FC: <unnamed function at 068F2B40 (JSFunction at 05880AF0)> 07FFF800: <function fbXPCOMUtils at 02DB0060 (JSFunction at 05793818)> 07FFF804: <XPCWrappedNative_NoHelper object at 02381D00> 07FFF808: <Object at 02380E20> argv: 07FFF754 (argc: 2) this: <Object at 08F53DE0> rval: undefined flags: computed_this scopeChain: (JSObject *) 08F41EA0 displaySave: (JSStackFrame *) 0012D144 JSStackFrame at 0012C9EC <unnamed function at 068D2A20 (JSFunction at 058A0A80)> file file:///C:/Documents%20and%20Settings/bartonjj/Application%20Data/Mozilla/ irefox/Profiles/getchromebug/extensions/firebug@software.joehewitt.com/componen s/firebug-service.js line 1080 pc = 0352BD0D current op: call slots: 07FFF744 sp: 07FFF75C = slots + 6 07FFF744: <Object at 08F53DE0> 07FFF748: undefined 07FFF74C: <unnamed function at 08F53760 (JSFunction at 06CE35E8)> 07FFF750: <Object at 08F53DE0> 07FFF754: <XPCWrappedNative_NoHelper object at 02381D00> 07FFF758: <Object at 02381C60> argv: 07FFF738 (argc: 3) this: <Object at 068D0420> rval: undefined flags: computed_this rooted_argv scopeChain: (JSObject *) 03653080 displaySave: (JSStackFrame *) 0012EC60 JSStackFrame at 0012D144 <unnamed function at 09DA1220 (JSFunction at 068B0850)> file file:///C:/Documents%20and%20Settings/bartonjj/Application%20Data/Mozilla/ irefox/Profiles/getchromebug/extensions/firebug@software.joehewitt.com/componen s/firebug-service.js line 2664 pc = 034FB1D8 current op: apply slots: 07FFF6F0 sp: 07FFF708 = slots + 6 07FFF6F0: undefined 07FFF6F4: undefined 07FFF6F8: <function apply at 058A2690 (JSFunction at 058A2690)> 07FFF6FC: <unnamed function at 068D2A20 (JSFunction at 058A0A80)> 07FFF700: <Object at 068D0420> 07FFF704: <Object object at 02381D40> argv: 07FFF6E4 (argc: 3) argsobj: <Object object at 02381D40> this: <Object at 09DA1200> rval: undefined flags: rooted_argv scopeChain: (JSObject *) 03653080 displaySave: (JSStackFrame *) 0012F480 JSStackFrame at 0012E550 <function getService at 09D32200 (JSFunction at 02333DC8)> sp: 07FFF708 slots: 00000000 argv: 07FFF6C4 (argc: 1) this: <nsJSCID object at 08DE0380> rval: undefined flags: computed_this rooted_argv scopeChain: (JSObject *) 06B11380 JSStackFrame at 0012EC60 <unnamed function at 06B304E0 (JSFunction at 06B10188)> file file:///G:/mozilla/mozilla-central/src/obj-i686-pc-mingw32/dist/bin/module /utils.js line 125 pc = 0601E98E current op: call slots: 07FFF6B8 sp: 07FFF6C8 = slots + 4 07FFF6B8: <Object at 06B30480> 07FFF6BC: <function getService at 09D32200 (JSFunction at 02333DC8)> 07FFF6C0: <nsJSCID object at 08DE0380> 07FFF6C4: <nsJSIID object at 08DE0420> argv: 07FFF6B8 (argc: 0) callobj: <Call object at 02381F20> argsobj: <Object object at 02381F00> varobj: <Call object at 02381F20> this: <Object at 06B30480> rval: undefined flags: computed_this rooted_argv scopeChain: (JSObject *) 02381F20 JSStackFrame at 0012F480 <unnamed function at 0E7A0DA0 (JSFunction at 06A03508)> file chrome://browser/content/browser.js line 3641 pc = 05B2A518 current op: getprop slots: 07FFF694 sp: 07FFF698 = slots + 1 07FFF694: <Object at 06B30480> argv: 07FFF690 (argc: 1) this: <ChromeWindow object at 02D23640> rval: undefined flags: rooted_argv scopeChain: (JSObject *) 02D23640
Reporter | ||
Comment 11•15 years ago
|
||
So this related to bug 499568. liveMarkService failed and Firebug's onError handler is printing the stack frame. It uses jsdIScript.functionObject.getWrappedValue() and tries to convert it to a string. It tries this that and the other thing, finally trying to just list the propreties. But whatever this thing is, its property iterator crashes.
Reporter | ||
Comment 12•15 years ago
|
||
When I comment out the lines around http://code.google.com/p/fbug/source/browse/branches/firebug1.5/content/firebug/lib.js#225 the crash does not occur.
Reporter | ||
Comment 13•15 years ago
|
||
Any idea on a test on an object to know that the |for (p in o)| will crash before it does?
![]() |
||
Comment 14•15 years ago
|
||
It sounds to me like the right thing here is figuring out why you have an object you're not "supposed" to have and fixing that in jsd, not adding workarounds for the case when you have such an object....
Assignee | ||
Comment 15•15 years ago
|
||
Looks like it's coming from jsdIScript.functionObject. http://code.google.com/p/fbug/source/browse/branches/firebug1.5/content/firebug/sourceFile.js#543 called from http://code.google.com/p/fbug/source/browse/branches/firebug1.5/content/firebug/lib.js#2041
![]() |
||
Comment 16•15 years ago
|
||
And where did the jsdIScript come from? Also, isn't the function in the jsdIScript always the non-clone function? Is that even supposed to be exposed to js normally?
Assignee | ||
Comment 17•15 years ago
|
||
jsdScript::GetFunctionObject calls JS_GetFunctionObject, passing a JSFunction pointer it got from either (1) the "new script" debugger hook; or as in this case (2) JS_GetFrameFunction. For some time (probably long after JSD was initially written) JS_GetFunctionObject was documented as completely unsafe and probably nonsensical. However, I recently discovered that there are some use cases for it, and the warning has been weakened accordingly: https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_GetFunctionObject but it is still clearly bad news in this case. I think it is OK that the JSFunction leaks here. The call to JS_GetFunctionObject looks like a bug to me.
Reporter | ||
Comment 18•15 years ago
|
||
(In reply to comment #16) > And where did the jsdIScript come from? From jsdIStackFrame, we are in onError() and trying to show the functions on the stack.
![]() |
||
Comment 19•15 years ago
|
||
When you say "show the functions", what do you really mean? Would it perhaps be a good idea to have a jsdIFunction that holds a JSFunction and can answer whichever function-related questions you have instead of Firebug trying to interrogate the JS Function object to glean information?
Comment 20•15 years ago
|
||
Optimizing to join function objects (bug 471214) means stack inspection may leak refs to joined function objects -- so we need to do something. Choices: 1. De-optimize ahead of time aggressively. 2. Clone the function object on stack inspection to unjoin. (1) seems infeasible or onerous almost to the point of being infeasible. (2) has options: 2a. Clone on every jsdIScript.functionObject since we don't know whether the jsdIScript came from a jsdIStackFrame for an activation of a joined function object. 2b. Clone when jsdIStackFrame is used to get at anything (including a jsdIScript) that could lead to the joined function object leaking, and store the clone back to overwrite the joined function object. (2b) looks good to me at a glance, but I'm not a jsd peer. /be
Comment 21•15 years ago
|
||
(In reply to comment #17) > jsdScript::GetFunctionObject calls JS_GetFunctionObject, passing a JSFunction > pointer it got from either (1) the "new script" debugger hook; or as in this > case (2) JS_GetFrameFunction. Let's fix this in jsdbgapi.cpp, via plan (2b) if I'm not missing something. No way to go wrong in jsd this way. > For some time (probably long after JSD was initially written) > JS_GetFunctionObject was documented as completely unsafe and probably > nonsensical. However, I recently discovered that there are some use cases for > it, and the warning has been weakened accordingly: > > https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_GetFunctionObject > > but it is still clearly bad news in this case. I think it is OK that the > JSFunction leaks here. The call to JS_GetFunctionObject looks like a bug to me. Indeed. Do we have a bug to get rid of this troublesome old API aspect (I mean the whole JSFunction * problem)? /be
![]() |
||
Comment 22•15 years ago
|
||
If 2b will fix this problem, sounds good to me (though I still think Firebug wants a jsdIFunction to fix its ridiculous performance issues with large functions).
Reporter | ||
Comment 23•15 years ago
|
||
There is also 3) Don't call jsdIScript.functionObject. I *think* that Bug 506961 will make Firebug's use of this thing obsolete. Just to beware, Firebug uses jsdIStackFrame a lot, eg on all script compiles.
Assignee | ||
Comment 24•15 years ago
|
||
(In reply to comment #21) > > The call to JS_GetFunctionObject looks like a bug to me. > > Indeed. Do we have a bug to get rid of this troublesome old API aspect (I mean > the whole JSFunction * problem)? What exactly is the problem though? JS_GetFunctionObject and JS_CallFunction are broken. Anything else? We can't fix this with the internal semantics of `JSFunction *` as they are now. We could have rename JSFunction -> JSFunctionImpl internally and make the now-JSAPI-only type `JSFunction *` mean whatever we want (probably: a pointer to a JSObject of class js_FunctionClass that isn't an internal function object). Is that what you have in mind? I'll be happy to file and fix such a bug.
Comment 25•15 years ago
|
||
(In reply to comment #24) > What exactly is the problem though? JS_GetFunctionObject and JS_CallFunction > are broken. Anything else? That's it (and enough, doncha think? Not looking for more problem than is there, but the problem has been hurting us for too long). > We can't fix this with the internal semantics of `JSFunction *` as they are > now. We could have rename JSFunction -> JSFunctionImpl internally and make the > now-JSAPI-only type `JSFunction *` mean whatever we want (probably: a pointer > to a JSObject of class js_FunctionClass that isn't an internal function > object). > > Is that what you have in mind? I'll be happy to file and fix such a bug. I swear we had such a bug. Igor? /be
Assignee | ||
Comment 26•15 years ago
|
||
This fixes only the two bugs identified in comment 4: JS_Lookup* APIs expose internal function objects; and fun_enumerate calls lookupProperty without setting flags.
Assignee: general → jorendorff
Attachment #407118 -
Flags: review?(brendan)
Comment 27•15 years ago
|
||
Comment on attachment 407118 [details] [diff] [review] v1 >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp >--- a/js/src/jsapi.cpp >+++ b/js/src/jsapi.cpp >@@ -3181,16 +3181,21 @@ LookupResult(JSContext *cx, JSObject *ob > *vp = JSVAL_VOID; > return JS_TRUE; > } > > JSBool ok = JS_TRUE; > if (OBJ_IS_NATIVE(obj2)) { > JSScopeProperty *sprop = (JSScopeProperty *) prop; > >+ if (sprop->isMethod()) { >+ *vp = OBJECT_TO_JSVAL(obj2); >+ return OBJ_SCOPE(obj2)->methodReadBarrier(cx, sprop, vp); Wait, *vp should be the method value, not the object in which the method property was found. Or am I in need of another coffee? /be
Assignee | ||
Comment 28•15 years ago
|
||
Comment on attachment 407118 [details] [diff] [review] v1 Dah, you are correct. I'll do a jsapi-test for penance.
Attachment #407118 -
Flags: review?(brendan)
Assignee | ||
Comment 29•15 years ago
|
||
Unfortunately the jsapi-test isn't testing what I want it to test. I have this code: // Make x.f a method. jsvalRoot x(cx); EVAL("x = {f: function () { return 2; }};", x.addr()); JSObject *xobj = JSVAL_TO_OBJECT(x); // This lookup should not return an internal function object. jsvalRoot r(cx); CHECK(JS_LookupProperty(cx, xobj, "f", r.addr())); JSObject *funobj = JSVAL_TO_OBJECT(r); CHECK(!js_IsInternalFunctionObject(funobj)); But js_IsInternalFunctionObject returns false even though I reverted my patch and so funobj is in fact an internal function object. I confirmed in gdb that sprop->isMethod() and funobj == (JSObject *) sprop->getter. return funobj == fun && (fun->flags & JSFUN_LAMBDA) && !funobj->getParent(); Here funobj is parented to the global object, I guess because of COMPILE_N_GO. So the third part of the test fails. I'm skeptical of the second part too; some non-LAMBDA functions need cloning even in COMPILE_N_GO mode. Offhand, I would change the second part to FUN_INTERPRETED(fun) and try to change the compiler to make the third part true. Thoughts? I would like to get this landed and fix js_IsInternalFunctionObject in a followup bug.
Comment 30•15 years ago
|
||
(In reply to comment #29) > But js_IsInternalFunctionObject returns false even though I reverted my patch > and so funobj is in fact an internal function object. I confirmed in gdb that > sprop->isMethod() and funobj == (JSObject *) sprop->getter. > > return funobj == fun && (fun->flags & JSFUN_LAMBDA) && > !funobj->getParent(); > > Here funobj is parented to the global object, I guess because of COMPILE_N_GO. Yes, that's intentional. COMPILE_N_GO means you don't need to clone certain functions. They are pre-bound to the right (one and only, the one to "go" using) global; their scope can't change due to multiple executions using other globals. This predates the joined function object / method barrier work by a number of years. > I'm skeptical of the second part too; > some non-LAMBDA functions need cloning even in COMPILE_N_GO mode. But not due to the method barrier, since only JSOP_LAMBDA followed by JSOP_SETMETHOD or JSOP_INITMETHOD optimizes to join function objects and unjoin via the barrier. > Offhand, I > would change the second part to FUN_INTERPRETED(fun) and try to change the > compiler to make the third part true. Thoughts? I don't think either is correct. /be
Assignee | ||
Comment 31•15 years ago
|
||
Fix the bug Brendan found in v1 and add a test. Also add a JS_UNLOCK_OBJ that was missing in v1. I made the jsapi-test more elaborate than what I posted in comment 29. Now it *definitely* produces methods that need to be cloned. The jsapi-test ends with: CHECK(!js_IsInternalFunctionObject(funobj)); CHECK(GET_FUNCTION_PRIVATE(cx, funobj) != (JSFunction *) funobj); Both are necessary here, and I claim both are correct; see the patch for the rest of the story. If I revert just the part of the patch that affects JS_LookupProperty (in jsapi.cpp), the first check passes but the second check fails. With the full patch, of course, both pass. It looks like COMPILE_N_GO means "never NULL out the parent of a function". If that's correct, we could change that to "only NULL out the parent of internal function objects" and that would make js_IsInternalFunctionObject more accurate.
Attachment #407118 -
Attachment is obsolete: true
Attachment #407391 -
Flags: review?(brendan)
Comment 32•15 years ago
|
||
Comment on attachment 407391 [details] [diff] [review] v2 (In reply to comment #31) > CHECK(!js_IsInternalFunctionObject(funobj)); > CHECK(GET_FUNCTION_PRIVATE(cx, funobj) != (JSFunction *) funobj); > > Both are necessary here, and I claim both are correct; see the patch for the > rest of the story. If I revert just the part of the patch that affects > JS_LookupProperty (in jsapi.cpp), the first check passes but the second check > fails. With the full patch, of course, both pass. Making sure I am following you: without the JS_LookupProperty patch the js_IsInternalFunctionObject(funobj) returned false but (you argue, convincingly) for bad reason, leaving the unjoined function object to leak. There's no JSRESOLVE_ASSIGNING in the resolve call's flags in this jsapi-test. But why is the fun_enumerate patch needed? > It looks like COMPILE_N_GO means "never NULL out the parent of a function". If > that's correct, we could change that to "only NULL out the parent of internal > function objects" and that would make js_IsInternalFunctionObject more > accurate. js_IsInternalFunctionObject is new, and used only in assertions. If these are botching, either it is buggy as you suggest, or something older (COMPILE_N_GO, or COMPILE_N_GO + joined function objects) is buggy -- or of course both. We should suspect everything, although I'm more suspicious of the newcomer. Let's see: 1. COMPILE_N_GO does exactly what you say: without it we leave compiler-created function objects with the proto and parent they get based on the scopeChain parameter to the compiler (similarly for regexps). This is correct because the JSOPTION_COMPILE_N_GO user promises not to re-execute, period (not just promises not to re-execute with a different scope-chain). 2. js_IsInternalFunctionObject starts with the "joined function object" identity test: JSFunction *fun = (JSFunction *) funobj->getPrivate(); return funobj == fun ... and then adds && (fun->flags & JSFUN_LAMBDA) ... which looks wrong now that you point it out (again ;-). It certainly avoids false alarms on function definitions. But what has it to do with COMPILE_N_GO? Not a lot. Finally, js_IsInternalFunctionObject finishes the hack && return value with && !funobj->getParent(); which is true if !(COMPILE_N_GO). So what was that JSFUN_LAMBDA test for? 3. The concerns to separate here are (a) scope chain at compile-time vs. runtime; (b) mutation/identity detection of joined function objects. 3(a) is what COMPILE_N_GO is about. We prebind compiler-created functions' parents to the compile-time scope chain, and have a fast path at runtime to avoid cloning. But this fast no-clone path is *only* for JSOP_DEFFUN and (if possible; outer function may be heavyweight so parent doesn't match) JSOP_DEFLOCALFUN. Before the method barrier to clone joined function objects (3(b)), JSOP_LAMBDA always cloned. Predecessor opcodes from pre-upvar2 days did not always clone lambdas -- these were bugs, but not scoping bugs, rather mutation/identity bugs. 3(b) arises only for JSOP_LAMBDA followed by JSOP_{INIT,SET}METHOD. So the bug is in js_IsInternalFunctionObject. It is testing JSFUN_LAMBDA to exclude function definitions, but forgetting that compile-n-go lambdas have non-null parents, same as compile-n-go defined functions. This results in the false negative that your jsapi-test in comment 29 found. The fix, if we need a further fix, is not to null parent more often in compile-n-go mode, since that may hurt cases where the 3(a) optimization wins. Rather, js_IsInternalFunctionObject (a debug helper only, not for non-DEBUG redundant logic w.r.t. 3(b)'s method barrier) needs to work harder to do its belt-and-braces DEBUG-only best. How to do this? We want to prebind function definitions in compile-n-go mode, when the script prolog runs once (and only once). But these effects remain after the main script runs, so during that run or after, an embedding could lookup or get a defined compile-n-go function by its name. There's no method barrier -- 3(b) doesn't apply. This is the reason I included the middle && (fun->flags & JSFUN_LAMBDA) clause. Does this make sense? I'm going to stop to get your review. > bool exec(const char *bytes, const char *filename, int lineno) { > jsvalRoot v(cx); > return JS_EvaluateScript(cx, global, bytes, strlen(bytes), filename, lineno, v.addr()) || > fail(bytes, filename, lineno); >- return true; > } > > #define EVAL(s, vp) do { if (!evaluate(s, __FILE__, __LINE__, vp)) return false; } while (false) > > bool evaluate(const char *bytes, const char *filename, int lineno, jsval *vp) { > return JS_EvaluateScript(cx, global, bytes, strlen(bytes), filename, lineno, vp) || > fail(bytes, filename, lineno); Pre-existing nits: make the || operands start in the same column per prevailing style? >+ JSAutoTempValueRooter root(cx, sprop); Cool, I've wanted that variant too. >+ JS_UNLOCK_OBJ(cx, obj2); When I saw *vp set wrong I stopped reviewing -- good catch. /be
Attachment #407391 -
Flags: review?(brendan) → review+
Comment 33•15 years ago
|
||
Sorry for being long-winded. I still have hopes that if we impose the method barrier where it is needed, js_IsInternalFunctionObject will not give false negatives. But it is not doing exactly what its name suggests. And we are not covering all of the logic with assertions. This is hard to do. The upvar2 work let JSOP_DEFLOCALFUN avoid cloning a function fun for which FUN_NULL_CLOSURE(fun) is false, i.e. a flat closure or unoptimized closure or interpreted function. Flat closures are cloned when formed. It's not so obvious why unoptimized local closures will be cloned -- the parent mismatch logic must be right, which depends on the upvar2 analysis and optimizations. The unoptimized closure cases break down like so: 1. Lightweight local function: if parent matches compile-time parent, no need to clone for scoping reasons (3(a) again), and no need for mutation/identity reasons (3(b)) because the function cannot escape as a funarg. Why the local function can't escape in this case needs explanation. Assume that it could escape. By the premise that it is not a null or flat closure, it must be scoped by a Call object (either for the outer, if there's only one outer function, or an outer if more than one nesting outer -- some could be null closures). But again this means parent won't match the compile-time global object scopeChain parameter, contradicting the assumption. So the only case where a local unoptimized function could escape is when there's no Call object scoping it, but then it must be a null closure (flat currently forces outer Call but in any event clones). So this can't happen. 2. Heavyweight local function: heavyweight taints upward, so the outer function has a Call object, so parent can't match the compile-time parent. Beefing up assertions here would be great. Better name or assertion helper in lieu of js_IsInternalFunctionObject might be justified. Coverage is the goal, not necessarily classifying "internal" vs. "external". Hope this helps, it helped me to reconstruct (upvar2 was a long haul and I've started to recycle some brain-RAM already). /be
Assignee | ||
Comment 34•15 years ago
|
||
Thanks for the review and comments. (In reply to comment #33) > Beefing up assertions here would be great. Better name or assertion helper in > lieu of js_IsInternalFunctionObject might be justified. Coverage is the goal, > not necessarily classifying "internal" vs. "external". Maybe we need a classification of functions in terms of where we can assert they won't appear. Some function objects are not even safe to call: uncloned flat closures, any uncloned function created by the compiler in non-COMPILE_N_GO mode (i.e. functions with no parent). Some functions are safe to call, under controlled conditions, but must not be "exposed to script" (i.e. escape so that the script can do arbitrary things to them): uncloned methods, uncloned null closures that use JSOP_GETUPVAR. But the two live in quite different cages, so it may make sense to distinguish between them and assert differently in different places. This suggests three predicates: IsUnclonedFunctionObject, IsUnclonedMethodObject, IsUnclonedUpvarClosure. Offhand I don't know how to write any of them. Is there any other set of functions that could use extra assertion coverage? I'll land this tomorrow regardless. (Can't watch the tree right now.)
Reporter | ||
Comment 35•15 years ago
|
||
(In reply to comment #34) ... > Maybe we need a classification of functions in terms of where we can assert > they won't appear. And JS debugger support for understanding those functions that can appear. (And no Boris I don't know enough yet to open a bug asking for this ;-).
Assignee | ||
Comment 36•15 years ago
|
||
> But why is the fun_enumerate patch needed? A separate bug, see comment 4. It was calling lookupProperty without passing flags, so the flags were being inferred incorrectly from context.
Comment 37•15 years ago
|
||
(In reply to comment #36) > > But why is the fun_enumerate patch needed? > > A separate bug, see comment 4. It was calling lookupProperty without passing > flags, so the flags were being inferred incorrectly from context. Saw that, but I didn't see how your jsapi-test tickled the separate bug. /be
Assignee | ||
Comment 38•15 years ago
|
||
Hmm. I couldn't reproduce the separate bug. It seems certain that what I said in comment 36 is correct, but I couldn't come up with a jsapi-test that actually triggers it. It's moot now anyway: bug 523686 deleted fun_enumerate. Pushed v2. http://hg.mozilla.org/tracemonkey/rev/3536f360c04a jjb, is it fixed?
Reporter | ||
Comment 39•15 years ago
|
||
I'll let you know when the patch lands on m-c
Assignee | ||
Comment 40•15 years ago
|
||
OK. I'll mark this fixed-in-tracemonkey out of optimism. Also wanted-1.9.2?
Flags: wanted1.9.2?
Whiteboard: [firebug-p1] → [firebug-p1] fixed-in-tracemonkey
Comment 41•15 years ago
|
||
probably wanted but requires approval. Reading back to try and understand this bug is non-trivial. I haven't seen a crash with Chromebug and Firebug in 1.9.2 so I'm wondering if this is required?
Flags: blocking1.9.2?
Reporter | ||
Comment 42•15 years ago
|
||
Only debug builds crash.
Comment 43•15 years ago
|
||
This is definitely wanted and probably blocking. I think this patch probably fixed (already blocking+P2) bug 505001.
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Assignee | ||
Comment 44•15 years ago
|
||
(In reply to comment #41) > Reading back to try and understand this bug is non-trivial. OK. The story so far: Bug A: The JS_Lookup* API functions can expose "internal function objects", which *look* like regular JS function objects but are actually a totally different kind of thing. They cannot be used generally as a JSObject without risking a crash. That their type is JSObject at all is some kind of really bad joke. Bug B: Enumerating such an internal function object can trigger a silly assertion. Bug 524968: All the JSAPI functions having to do with the JSFunction type expose internal function objects to unsuspecting API users by design. In particular, the two function JS_GetFunctionObject and JS_CallFunction can only be used safely in special cases. This runs deeper than the JS_Lookup* thing. It's a longstanding API design mistake and we're interested in fixing it, but it's nontrivial and Bug 525009: in the meantime, jsd is still using JS_GetFunctionObject unsafely. Bug 524981: JS_GetFrameFunction and JS_GetFrameFunctionObject also expose internal function objects, thanks to yet another separate bug. It's now possible to call a method even though the (safe) function object has been optimized away entirely; only the (unsafe) internal function object exists. These JSDBGAPI functions don't yet take that into account. Even if bug 524986 or bug 525009 were fixed, this would still pose a hazard. The patch here fixes bugs A and B. Firebug was tripping over bug 525009 and subsequently bug B. That's why I think the patch probably fixes this particular crash.
Flags: blocking1.9.2+ → blocking1.9.2?
Updated•15 years ago
|
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Comment 45•15 years ago
|
||
That is an excellent précis. Thanks, Jason. any plans on back-porting this to mozilla-central and eventually 1.9.2? Any risks in doing that? Jason has provided a test to go along with this excellent patch.
Whiteboard: [firebug-p1] fixed-in-tracemonkey → [firebug-p1][firebug-blocks][need-approval-1.9.2] fixed-in-tracemonkey
Comment 46•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3536f360c04a
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 47•15 years ago
|
||
I don't think this should be problem on 1.9.2, unblocking
Flags: blocking1.9.2+ → blocking1.9.2-
Reporter | ||
Comment 48•15 years ago
|
||
(In reply to comment #44) ... > The patch here fixes bugs A and B. Firebug was tripping over bug 525009 and > subsequently bug B. That's why I think the patch probably fixes this particular > crash. Jason, does your analysis in comment 44 mean: 1) jsdIScript.functionObject should not be used, or 2) #1 plus other stuff in the JS script access API for jsd (jdsIDebuggerService et al) should not be used? I also need to know if jdsIScript.functionObject is commonly correct and does not crash in FF 3.5? I know that Boris does not believe it is correct, but my question here is about common cases. We need to decide if we remove the calls in Firebug 1.5 which must support Firefox 3.5.
Assignee | ||
Comment 49•15 years ago
|
||
Sorry I didn't make this clear. The rules are, * Use jsdIScript.functionObject only internally; don't expose that object to any user code. * Whatever you do, don't call it. Bug 525009 existed in FF 3.5. Bug B was new, which is why we suddenly started flunking an assertion on startup. With this fix, Firebug should be fine. Reopen if it's not.
Updated•15 years ago
|
Whiteboard: [firebug-p1][firebug-blocks][need-approval-1.9.2] fixed-in-tracemonkey → [firebug-p1][firebug-blocks] fixed-in-tracemonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•