Jason Orendorff [:jorendorff] Reporter
	Description • 15 years ago

JS_GetFunctionObject is a bad-news API:
https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_GetFunctionObject

The offending call is here:
http://hg.mozilla.org/mozilla-central/file/032a9a176ee7/js/jsd/jsd_xpc.cpp#l1290

Spun off from bug 522590 comment 17. Bug 524968 and bug 524981 are related.

I don't think this bug is a terribly high priority. Fixing bug 524968 would entail fixing this somehow (involving API changes to JSNewScriptHook, I think).

	Jason Orendorff [:jorendorff] Reporter
	Comment 1 • 15 years ago

A little explanation.

JS_GetFunctionObject is subject to a longstanding restriction that was never widely known or understood, and only recently documented: it is safe if the argument is a native function or a JS_Compile* result; otherwise you're taking your life in your hands.

It would be a long sidetrack to explain why, and anyway bug 524968 aims to change it and make JS_GetFunctionObject safe.

The danger in this bug is that an "internal function object", returned by JS_GetFunctionObject and handed back to JavaScript via jsd_xpc.cpp, might be called. This has always been broken. In years gone by, calling an internal function object might just work, or it might throw seemingly random ReferenceErrors etc. Since upvar, it is still more broken: it can crash.

	timeless
	Comment 2 • 15 years ago

i'm not in a hurry to fix this as 

http://mxr.mozilla.org/mozilla-central/source/js/jsd/jsd_xpc.cpp?rev=032a9a176ee7&mark=1290#1280

leads to:

http://mxr.mozilla.org/mozilla-central/ident?i=GetFunctionObject

which shows no consumers in the tree.

otoh, removing an api will just break consumers. and given that i got pushback the last time i tried to be helpful to consumers when i changed an iid, i'm not in a hurry to do it again.

	Jason Orendorff [:jorendorff] Reporter
	Comment 3 • 15 years ago

It's used by Firebug.

	timeless
	Comment 4 • 15 years ago

then tell them to stop?

you haven't provided an explanation of how i could usefully fix this function short of making it do nothing. if i do that, they'll have to stop using it anyway. you might as well save me some wasted code churn by telling them not to use it.

	Jason Orendorff [:jorendorff] Reporter
	Comment 5 • 15 years ago

Your code is calling an API function in an incorrect and unsafe way and consequently returning a value to scripts that can cause them to crash.

If you're OK with that, just WONTFIX this.

	timeless
	Comment 6 • 15 years ago

Let's try this again. If I remove the API or make it throw (or even make it return null constantly), Firebug breaks. Then Firebug complains. But Firebug is already broken, and either way Firebug needs to be fixed.

Later you go around and fix bug 524968, I'm assuming your fix means that I get to just put back the code you want me to remove. Then I have to go back and do that.

That's work for me on both sides, when in reality, I have one consumer using an API that doesn't work and probably can't work until bug 524968 is fixed.

jsdebug/jsd/jsdI have always enabled its consumers to shoot themselves in their feet. The breakpoint API is a very good example of that.

(SpiderMonkey itself will gladly let one shoot off one's head, or torso, or any random appendage.)

Now, if you told me "you can use X to figure out if the value returned by JS_GetFunctionObject satisfies 'is a native function OR a JS_Compile* result', I'd be vaguely willing to consider writing a patch and trying to find someone to review it.

Given that I rarely am able to get anyone to review my work (because everyone insists they don't understand jsd or SpiderMonkey or idl or mozilla or xpcom or why I asked them), I'm not inclined to expend effort (twice!) which doesn't benefit anyone.

	Rob Campbell [:rc] (:robcee)
	Comment 7 • 15 years ago

we talked about this a bit last week, and I understand timeless' frustration. As I understand it, we will no longer be able to access jsdIScript::functionObject properties from JS anymore. This breaks Firebug in a pretty fundamental way.

For mozilla-central, 3.7, we're going to need some way to regain this functionality, preferably in an acceptable, standardized way. We'll need help from the JS crew, timeless and whoever else can do this.

timeless, as for not being able to get the reviews you (and we!) need, I'll do what I can to help. As I spend more time in JSD, I hope to be able to provide some of those reviews myself, but for the time being, I'm still learning the ropes. Your patience and that of the platform gurus is appreciated.

thanks for bringing this to my attention.

	Brendan Eich [:brendan]
	Comment 8 • 15 years ago

Timeless: I'll review your jsd changes. FWIW I agree there are unsafe APIs already, so if this is no more unsafe (null pointer deref, or nearly-null), it can wait for the JS engine fix.

/be

	John J. Barton
	Comment 9 • 15 years ago

In Firebug 1.6 we will remove all of the jsdIScript.functionObject calls in favor of new API available in Firefox 3.6. We can't do that for Firebug 1.5 because it needs to support Firefox 3.5. We plan for Firebug 1.6 to replace Firebug 1.5 when Firefox 3.7 is released.

	Terrence Cole [:terrence]
	Comment 10 • 9 years ago

Jason, I guess this bug is defunct and can be closed?