Bug 4166 - Function object does not support caller property
Summary: Function object does not support caller property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 10489
  Show dependency treegraph
 
Reported: 2005-07-27 11:22 PDT by Jeff Watkins
Modified: 2006-11-18 17:23 PST (History)
6 users (show)

See Also:


Attachments
Test case (1.04 KB, text/html)
2005-08-06 23:51 PDT, Mark Rowe (bdash)
no flags Details
proposed patch + test (3.32 KB, patch)
2006-03-28 06:57 PST, Arthur Langereis
ggaren: review-
Details | Formatted Diff | Diff
Patch (25.97 KB, patch)
2006-10-12 16:34 PDT, Vladimir Olexa (vladinecko)
ggaren: review-
Details | Formatted Diff | Diff
Patch Revised (26.15 KB, patch)
2006-10-12 19:30 PDT, Vladimir Olexa (vladinecko)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Watkins 2005-07-27 11:22:29 PDT
the following code does not work correctly because the function object either
doesn't have a caller property or the value is always null:

/** Output the string along with the name of the calling function to the
    trace DIV.
 **/
function trace( string )
{
    var callingFn= trace.caller;
    var callingFnText;
    var callingFnName;
    var paren;
    var traceElement= document.getElementById( "trace" );

    if (callingFn)
    {
        callingFnText= callingFn.toString();
        paren= callingFnText.indexOf( "(" );
        callingFnName= callingFnText.substring( 9, paren );
    }
    else
    {
        callingFnName= "global";
    }

    var text= callingFnName + ": " + string;
    traceElement.appendChild( document.createTextNode( text ) );
}
Comment 1 Mark Rowe (bdash) 2005-08-06 23:50:39 PDT
Confirmed with ToT.
Comment 2 Mark Rowe (bdash) 2005-08-06 23:51:07 PDT
Created attachment 3253 [details]
Test case
Comment 3 Arthur Langereis 2005-10-14 02:31:16 PDT
I have an implementation of this at home. I'll update my source and check if it still works and make a 
patch soon.

BTW, here's another testcase:
http://www.xfinitegames.com/safari/callstack.html
Comment 4 Gavin Kistner 2006-01-17 07:08:35 PST
FWIW, the caller property is not part of the official ECMAScript-262 spec. There is a 'callee' property of 
arguments (section 10.1.8), but no caller property. This is, however, a very useful property that should be 
supported for compatibility with other browsers that support it.
Comment 5 Jesse Costello-Good 2006-01-17 20:32:19 PST
I am evaluating Safari for a port of TIBCO(R) General Interface, a mature AJAX platform that is currently IE 
only. Fixing this bug would make the port more feasible. 
Comment 6 Arthur Langereis 2006-03-28 06:57:13 PST
Created attachment 7359 [details]
proposed patch + test

This is a proposed patch to support the caller property in JSC. The caller property is set to either the calling function or jsNull if the function was called from the root context, mirroring Mozilla's behavior wrt this property.

The test checks for both cases. Also, another test of mine that builds a callstack works with this patch:
http://www.xfinitegames.com/safari/callstack.html
Comment 7 Geoffrey Garen 2006-03-28 10:33:45 PST
Comment on attachment 7359 [details]
proposed patch + test

Arthur,

This is the first patch I've seen you author for the WebKit OpenSource project. Welcome!

Allow me to further welcome you with an r-. :)

Issues that require an r-:

1. ChangeLog. Each patch requires one. The prepare-ChangeLog script will do some of the work for you. Then you need to fill in an explanation of why you changed what you changed.

2. Copyright. To keep things legal, you need to add your name to the copyright notice of any file you changed. You can find examples of this scattered throughout our source code.

3. Modifying the caller property. The test doesn't verify that the property cannot be deleted. Also, should the property be read-only? Since Mozilla invented this, what does Firefox do?

4. The code adds the caller property as a permanent fixture to the function object's property map, but the Mozilla doc says, "The caller property is available only within the body of a function. If used outside a function declaration, the caller property is null." So I think the code is wrong in a case like this:

foo();
alert(foo.caller) // should be "null"

At the very least you would have to delete the property after executing the function. However,

5. Adding and deleting an extra property during every function call may have a negative effect on performance. 

Since caller is a specially funny property, a better approach might be to provide a special funny accessor in FunctionImp::getOwnPropertySlot. That would avoid the twp property map hits. The argumentsGetter and lengthGetter methods are examples. In callerGetter, you could get at the calling context through exec->context()->callingContext(). You could recurse up through calling contexts to (1) find the caller function and (2) determine if the current function were executing (since you would need to return jsNull if it weren't.)

Other issues:

1. NULL. Our preferred style is to use 0 for NULL in c/c++ code, and nil for NULL in objc code.

2. if (curImp). Can a Context ever have a NULL imp? I don't think so. No other code checks for such a thing.
Comment 8 Geoffrey Garen 2006-03-31 12:21:47 PST
Actually, it looks like argumentsGetter is exactly the code we want for callerGetter. Copy + paste = done.
Comment 9 Vladimir Olexa (vladinecko) 2006-10-12 16:34:19 PDT
Created attachment 11056 [details]
Patch
Comment 10 Geoffrey Garen 2006-10-12 18:32:47 PDT
Comment on attachment 11056 [details]
Patch

A few minor quibbles, then I think this is ready to land:

- Our style guidelines call for "* " and "& " rather than " *" and " &".

- Your test case is very confusting. If it started failing, it would take me a while to figure out why. Two suggestions: (1) choose 'inner' and 'outer' or 'parent' and 'child' -- don't use both; (2) Make your shouldBe explain what they test more clearly. For example, shouldBe('innerHasCaller', 'false'), shouldBe('innerHasCallerWhenCalledFromGlobalCode', 'false'), shouldBe('innerHasCallerWhenCalledFromFunction'), 'true').
Comment 11 Vladimir Olexa (vladinecko) 2006-10-12 19:30:17 PDT
Created attachment 11059 [details]
Patch Revised

- removed {} from one-statement if
- moved *'s and &'s
- made the test case more understandable
Comment 12 Dave Hyatt 2006-10-12 20:03:31 PDT
Just a heads up that this needs very close security scrutiny.  Mozilla had a number of security issues surrounding the caller property.  This property is really dangerous.
Comment 13 Edward Rudd 2006-10-12 20:59:42 PDT
I will put this to some tests tomorrow, with the web app I am developing that makes use of the caller property.  Is there a list of the security issues that mozilla has had w/ respect to this property so they can be reviewed and checked against this code??
Comment 14 Vladimir Olexa (vladinecko) 2006-10-13 07:35:38 PDT
(In reply to comment #12)
> Just a heads up that this needs very close security scrutiny.  Mozilla had a
> number of security issues surrounding the caller property.  This property is
> really dangerous.
> 

I think you're referring to the __caller__ property they removed for security reasons since it returned the activation object of the caller (and thus allowed for stack reconstruction). They have a note about it in the specs for the caller property on a function. They also had arguments.caller which became obsolete with this function.caller property. I couldn't find any security issue that would relate to function.caller specifically, but maybe I'm overlooking something. 
Comment 15 Geoffrey Garen 2006-10-13 11:28:28 PDT
From my reading of the Bugzilla bug [https://bugzilla.mozilla.org/show_bug.cgi?id=65683] and the Mozilla documentation [http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Function:caller], the issue was with __caller__, not caller. Also, it pertained to "trusted" JS in the Firefox XUL UI calling "untrusted" remote JS in a webpage, which we never do.
Comment 16 Geoffrey Garen 2006-10-13 11:35:40 PDT
Comment on attachment 11059 [details]
Patch Revised

Some of the *'s and >'s are still wrong. 

Also, 'childHasCallerWhenCalledFromGlobalCode' is incorrect -- it's really 'childHasCallerWhenExecutingGlobalCode'.

Anyway, I think we can land this.
Comment 17 Edward Rudd 2006-10-16 14:42:04 PDT
Sweet.
Running SVN 17068 with attachment 11059 [details] , My framework now correctly works in safari (aside from a few rendering issues not related to this bug)

So this patch works in a real world app using this functionality.
Comment 18 Maciej Stachowiak 2006-10-31 04:58:01 PST
Moving to webkit-unassigned to make clear anyone can land this - it's not Geoff's patch.
Comment 19 Alexey Proskuryakov 2006-10-31 10:24:29 PST
Committed revision 17483 (moved the stars and renamed childHasCallerWhenCalledFromGlobalCode).
Comment 20 Jesse Costello-Good 2006-11-18 17:23:10 PST
Thanks to the Safari team for being so responsive regarding requests for better advanced AJAX support. Safari is quickly becoming a 1st class AJAX platform.