521010 - [jsd] add support for comparing function objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[John J. Barton]

Boris Zbarsky wrote:
> On 10/7/09 2:01 AM, John J. Barton wrote:
>> I'd like to investigate to see I can find a hack-around. So I'd like to
>> look at the code for operator=== when these objects come in. Can anyone
>> give me a hint on which file to start looking?
>
> js_StrictlyEqual in js/src/jsinterp.cpp
>
> That said, are you sure your case isn't hitting one of the callers of js_CloneFunctionObject?  Obvious such callers from a brief skim of the source if I'm reading it right would be scripts and event handlers for chrome:// XUL, functions declared inside other functions, functions that are handed around as objects, and whatever JSScope::methodReadBarrier is.  The jsd code seems to be keeping track of the JSFunction in the JSDScript, and the JSFunction is shared amongst the clones; when it gets script.functionObject it gets the JSFunction's canonical object, which is the object all the clones are clones of.  But any given clone will not be === to this, or even ==.
>
> If you really want a way of testing whether two functions are "the same" in terms of having the same JSFunction, you presumably need an API for that on JSD (and even then note that they could have different properties set on the function objects themselves by script and could behave differently based on that by examining arguments.callee properties or something, right?

The specific question I want to answer is:
  What is the property name bound to the functionObject of the current frame.script?

This provides an importantly different "function name" that (I think) will be valuable in understanding code. In particular the jsdIScript functionName is mostly worthless (Firebug does not even use it) since lots of JS code has constructs like
   X.Y = function() {}
or
   X.Y = { foo: function() {} }
where the jsdIScript funtionName will be 'anonymous'.

My model for how this works -- which is clearly wrong -- is that for
   X.Y = function y() {}
the property 'Y' of object 'X' is a reference to the function object 'y'.  Consequently if I see a stack |frame| running 'y' then the frame.script.functionObject would be 'y' and X.Y === y.

If under the covers the is clone this or that, I don't want to know about it ;-) .

jjb


-------------
On 10/7/09 10:47 AM, John J. Barton wrote:
> The specific question I want to answer is:
> What is the property name bound to the functionObject of the current
> frame.script?

There is no "frame.script" as part of JS itself.  You're looking under the covers.  I understand what your use case is, but the point is that you don't have the tools you need for this.

> My model for how this works -- which is clearly wrong -- is that for
> X.Y = function y() {}
> the property 'Y' of object 'X' is a reference to the function object
> 'y'.

Each time you execute that line, you get a new "function object y". However they all share the same script (this is an optimization, since scripts are not observable from the language).  Therefore, if you're getting something from the "script", you get one particular object, which happens to not match any of the values of X.Y.

> If under the covers the is clone this or that, I don't want to know
> about it ;-) .

What you want, as I said in my previous mail, is a debugger API function that compares two function objects for "equality" by seeing whether they have the same JSFunction.

-Boris
------------

Comment 1

[Boris Zbarsky [:bzbarsky]]

So how would you like the new API to look (and which object would you like it to hang off of)?

Comment 2

[:Gijs (he/him)]

This is an interesting idea. Venkman currently works around the issue you describe (I think? I'm not entirely clear on what's going on...) by just parsing the source and looking for patterns like: foo.bar = function(), uses a regexp, and then assigns the function the "prettier" name "bar".

In your usecase, how do you get hold of the object foo (or, in your specific example, "X")? :-)

Comment 3

[John J. Barton]

(In reply to comment #2)
> This is an interesting idea. Venkman currently works around the issue you
> describe (I think? I'm not entirely clear on what's going on...) by just
> parsing the source and looking for patterns like: foo.bar = function(), uses a
> regexp, and then assigns the function the "prettier" name "bar".

Firebug also does this, probably because it was copied from Venkman. But it is  unreliable because it is not a real parser and it also gives a different answer than we would get from looking at the property binding during execution:
   Z.foo = X.Y;
   ...
   bar(Z.foo);
   ...
 
> 
> In your usecase, how do you get hold of the object foo (or, in your specific
> example, "X")? :-)

For simple cases, it is "frame.thisValue". Otherwise you have to look through the scopes to find an object with the property that matches the function I guess.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fixing the component (again!), and would still like an answer to comment 1.

Comment 5

[:Gijs (he/him)]

(In reply to comment #4)
> Fixing the component (again!), and would still like an answer to comment 1.

Argh, sorry. This is odd, because I had a mid-air with you initially, copied my comment, hit back, hit refresh, re-pasted, and then hit commit. I guess the browser then keeps the selection of the dropdown for the component, or something? :-\ -- how do I properly avoid the mid-air?

As for which object... I would presume jsdIDebuggerService, but maybe John has a better idea?

Out of interest, if I load the same webpage twice (eg. in two tabs), does the same thing happen (ie one JSFunction for the scripts in both pages)?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> hit refresh

Presumably regular refresh (which preserves currently-set form values), rather than a forced one that throws them away?  Doing a force-refresh (shift-reload or on Windows Ctrl+F5) will avoid stomping on things.

> if I load the same webpage twice (eg. in two tabs), does the same thing happen

At the moment, no.  That might change.  It _does_ happen for XUL windows: if you open two browser windows they share the actual JSScripts.

Comment 7

[John J. Barton]

(In reply to comment #1)
> So how would you like the new API to look (and which object would you like it
> to hang off of)?

I don't know enough to answer so I'll make something up.

I gather that JSFunction is a unique but hidden property of a jsdIScript. So I think all considered, a function on jsdIDebuggerService itself:
   jsdIScript getScript(fn); // null if typeof(fn) != function
Then we can compare the non-hidden jdsIScript objects.

This function would solve numerous other problems in Firebug as well.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I gather that JSFunction is a unique but hidden property of a jsdIScript.

No.  It's a unique (in the sense that each JS function has one) but hidden property of JavaScript function objects.  While a JavaScript function always has a JSFunction it may or may not have an associated JSScript: "native" functions (the ones declared in the language itself using the |function| keyword) have one, but functions backed by C functions do not.  I guess in your case you know you have a native function...

I don't know exactly what the jsdIScript you get off the frame represents.  Brendan, it looks like that uses JS_GetFrameScript.  Will that get the per-function JSScript for the frame's callee, or will that get the overall JSScript?  I believe it's the former, right?

John, the APIs I basically see as possible here are either what you proposed, or a |functionsEqual| (or something) API on jsdIDebuggerService that takes two jsdIValues and returns true if they're both function objects and have the same JSFunction, or something hanging off jsdIValue perhaps...  Comparing jsdIScripts seems a bit magic to me, but might in fact work.

Comment 9

[John J. Barton]

(In reply to comment #8)
>  Comparing
> jsdIScripts seems a bit magic to me, but might in fact work.

Debuggers rely on jsdIScript.tag being unique.
jjb

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  How about I add a .script readonly property on jsdIValue, null unless it's a native function?

Comment 11

[John J. Barton]

(In reply to comment #10)
> OK.  How about I add a .script readonly property on jsdIValue, null unless it's
> a native function?

That would be great if we also had something like bug 506961. Otherwise it does not help because I have js object frameThis[p], not a jsdIValue.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Why does wrapValue not do what you want there?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Can you give this a whirl?

With this patch applied, I'd think that wrapValue().script should give you the jsdIScript you want...

Comment 14

[John J. Barton]

(In reply to comment #12)
> Why does wrapValue not do what you want there?

I have not been successful with wrapValue but I will try again.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #8)
> > I gather that JSFunction is a unique but hidden property of a jsdIScript.
> 
> No.  It's a unique (in the sense that each JS function has one) but hidden
> property of JavaScript function objects.  While a JavaScript function always
> has a JSFunction it may or may not have an associated JSScript: "native"
> functions (the ones declared in the language itself using the |function|
> keyword)

Please do not call those "native" functions. That goes against the Java meaning of "native", which we use as well in SpiderMonkey (and which Ecma uses with yet a different meaning; this also shows up, confusingly, in SpiderMonkey), meaning "built-in or otherwise implemented by native code."

We generally call these interpreted functions, scripted functions, user-defined functions, etc.

> have one, but functions backed by C functions do not.

Those would be "native functions" in conventional SpiderMonkey parlance, or "native methods" (Java or SpiderMonkey).

> I don't know exactly what the jsdIScript you get off the frame represents. 
> Brendan, it looks like that uses JS_GetFrameScript.  Will that get the
> per-function JSScript for the frame's callee, or will that get the overall
> JSScript?  I believe it's the former, right?

What is "the overall JSScript"?

Frames with non-null script members are activations of scripts or scripted functions, and the non-null script member always points to the script compiled for that frame's function, global, or eval code.

There is *no* embedding of inner function or eval scripts in outer scripts, so no "overall script" associated with a frame.

More in a bit.

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 405077 [details] [diff] [review]
Can you give this a whirl?

>+++ b/js/jsd/jsd_val.c
>@@ -652,16 +652,44 @@ jsd_GetValueClassName(JSDContext* jsdc, 
>         JS_BeginRequest(jsdc->dumbContext);
>         if(JS_GET_CLASS(jsdc->dumbContext, obj))
>             jsdval->className = JS_GET_CLASS(jsdc->dumbContext, obj)->name;

Note lack of overbraced prevailing style.

>         JS_EndRequest(jsdc->dumbContext);
>     }
>     return jsdval->className;
> }
> 
>+JSDScript*
>+jsd_GetScriptForValue(JSDContext* jsdc, JSDValue* jsdval)
>+{
>+    JSContext* cx = jsdc->dumbContext;
>+    jsval val = jsdval->val;
>+    JSFunction* fun;
>+    JSExceptionState* exceptionState;
>+    JSScript* script;
>+
>+    if (!jsd_IsValueFunction(jsdc, jsdval)) {
>+        return NULL;
>+    }

Overbraced.

>+
>+    JS_BeginRequest(cx);
>+    exceptionState = JS_SaveExceptionState(cx);
>+    fun = JS_ValueToFunction(cx, val);
>+    JS_RestoreExceptionState(cx, exceptionState);
>+    script = JS_GetFunctionScript(cx, fun);

Null-check fun here.

>+    JS_EndRequest(cx);
>+
>+    if (!script) {
>+        return NULL;
>+    }

Overbraced.

Seems ok otherwise. Who owns jsd, really? I hear timeless is in eastern Europe or Russia somewhere, not able to review promptly.

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Who owns jsd, really?

lumpy, last I checked.  Or rginda?  :(

I'll ask for r?timeless too, but don't think John should block on that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: With Brendan's comments addressed, and bug fixed

I decided to try testing this using this in jsconsole:

var f = function () {} ; var serv = Components.classes["@mozilla.org/js/jsd/debugger-service;1"].getService(Components.interfaces.jsdIDebuggerService); alert(serv.wrapValue(f).script)

and hit a fatal assert because I don't JSD_LOCK/UNLOCK_SCRIPTS around the jsd_FindJSDScript call.  This patch fixes that.  With this patch, the above works, and the resulting jsdIScript has the right functionSource and such.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Pushed that last patch as http://hg.mozilla.org/mozilla-central/rev/6958e86eead0

Comment 20

[Boris Zbarsky [:bzbarsky]]

The test for bug 507448 tests this patch.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 405193 [details] [diff] [review]
With Brendan's comments addressed, and bug fixed

It would be good to get this on branch so Firebug can make use of it...

Comment 22

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

this would be great to have in 3.6.

Comment 23

[timeless]

Comment on attachment 405193 [details] [diff] [review]
With Brendan's comments addressed, and bug fixed

just noting that i'm happy with this patch. i'm still 1500 bugmails behind (and will be traveling next week, but i'll be in CA, US, so people should be able to poke me if they have urgent problems)

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 405193 [details] [diff] [review]
With Brendan's comments addressed, and bug fixed

a192=beltzner

Comment 25

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b8db9e91c62a

Comment 26

[John J. Barton]

Unfortunately this feature does not work all of the time. In particular when I issue
 found = this.jsd.wrapValue(fn).script;
for fn in a Web page declared like:
function onExecuteTest()
{
 console.log("Hello World!");
} 
then script is null. It does work for extension functions in browser.xul.

Comment 27

[John J. Barton]

In the failing case we exit with NULL:
JSDScript*
jsd_GetScriptForValue(JSDContext* jsdc, JSDValue* jsdval)
{
    JSContext* cx = jsdc->dumbContext;
    jsval val = jsdval->val;
    JSFunction* fun;
    JSExceptionState* exceptionState;
    JSScript* script = NULL;
    JSDScript* jsdscript;

    if (!jsd_IsValueFunction(jsdc, jsdval))
        return NULL;
---------------^ HERE

Note that the js code checks the argument:
if (typeof(fn) == "function" || fn instanceof Function)
and the result is true.

I poked around and discovered that this.jsd.wrapValue(fn).jsParent looks like our function and it has the correct 'script' property. I don't know what the jsParent is but it gives the right result.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Your issue is that "fn" is a security wrapper around a content function, so not actually a scripted function (and hence doesn't have a script).  jsParent is ... not very well defined, but for security wrappers around functions the parent is the original function as a hack to make it possible to easily find the original function when the wrapper is called.

Can you please file a followup bug on this case and cc mrbkap and me on it?  It's possible that jsd should auto-unwrap here, if it doesn't open security holes.

Comment 29

[John J. Barton]

https://bugzilla.mozilla.org/show_bug.cgi?id=560751