Closed Bug 522590 Opened 15 years ago Closed 15 years ago

FF 3.7 crash on startup with chromebug

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: johnjbarton, Assigned: jorendorff)

References

Details

(Whiteboard: [firebug-p1][firebug-blocks] fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
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]
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?
In that topmost js_Interpret, what was script->filename/script->lineno? Best to call js_FramePCToLineNumber(cx,fp) there too.

/be
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
(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?
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.
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
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
No, forget those last two posts.
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
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.
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.
Any idea on a test on an object to know that the |for (p in o)| will crash before it does?
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....
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?
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.
(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.
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?
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
(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
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).
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.
(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.
(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
Attached patch v1 (obsolete) (deleted) — Splinter Review
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 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
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)
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.
(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
Attached patch v2 (deleted) — Splinter Review
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 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+
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
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.)
(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 ;-).
> 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.
(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
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?
I'll let you know when the patch lands on m-c
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
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?
Only debug builds crash.
This is definitely wanted and probably blocking. I think this patch probably fixed (already blocking+P2) bug 505001.
Flags: blocking1.9.2? → blocking1.9.2+
(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?
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
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
http://hg.mozilla.org/mozilla-central/rev/3536f360c04a
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I don't think this should be problem on 1.9.2, unblocking
Flags: blocking1.9.2+ → blocking1.9.2-
(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.
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.
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.

Attachment

General

Created:
Updated:
Size: