Closed Bug 23346 Opened 20 years ago Closed 20 years ago

'call()' inside a function invokes Function.prototype.call

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: martin.honnen, Assigned: brendan)

Details

(Keywords: js1.5)

Attachments

(16 files)

83.47 KB, patch
Details | Diff | Splinter Review
88.01 KB, patch
Details | Diff | Splinter Review
36.77 KB, patch
Details | Diff | Splinter Review
7.33 KB, text/html
Details
6.21 KB, patch
Details | Diff | Splinter Review
1.75 KB, patch
Details | Diff | Splinter Review
4.93 KB, text/html
Details
121.93 KB, patch
Details | Diff | Splinter Review
4.54 KB, text/html
Details
323.32 KB, patch
Details | Diff | Splinter Review
109.33 KB, patch
Details | Diff | Splinter Review
3.96 KB, text/plain
Details
18.51 KB, patch
Details | Diff | Splinter Review
18.51 KB, patch
Details | Diff | Splinter Review
323.45 KB, patch
Details | Diff | Splinter Review
109.63 KB, patch
Details | Diff | Splinter Review
When defining a function named
  call
and calling that inside another function not the function named call is called
but Function.prototype.call which thereby recursively calls the calling
function.
Brendan says it's a bug.

Here is a client side example

<HTML>
<HEAD>
<STYLE>

</STYLE>
<SCRIPT>
function f () {
  alert('f called');
  if (confirm ('Call call?'))
    call();
}
function call () {
  alert('call called');
}
f()
</SCRIPT>
</HEAD>
<BODY>

</BODY>
</HTML>

as I don't have access to pure C engine currently. But using confirm at least
allows to prevent infinite recursion in the bug demo.
Assignee: mccabe → brendan
Reassigning to Brendan.

The issue here is not 'call' per se, but that unqualified references within a
function body pick up names from Function.prototype.
Status: NEW → ASSIGNED
Target Milestone: M13
This should be fixed before JS1.5 freezes.

/be
Priority: P3 → P1
P1 for JS1.5 deadline fix.

/be
Whiteboard: [JS15]
Keywords: js1.5
Whiteboard: [JS15]
Removing [JS15] from Status Summary, putting js1.5 in new Keyword field :-)
OS: Windows 98 → other
Hardware: PC → All
OS: other → All
added testcase ecma_3/ExecutionContexts/regress-23346.js
Target Milestone: M13 → M14
JS1.5 is slipping past M13, sez rginda.

/be
rginda, can you try my patch?  It works on the top of trunk as of right now.  I 
could use all the testsuite magic you can throw at it.  Thanks,

/be
Attached patch proposed fixSplinter Review
Better summary.

/be
Summary: calling function named "call" inside a function calls Function.prototype.call → 'call()' inside a function invokes Function.prototype.call
Looking for an r= tomorrow.

/be
Trying the patch out now.
Things look bad, 11 new failures.
Attaching the linux debug test results.
(The only failure without the patch is the one for this bug.)
Rob, can you revert jsparse.c and apply the latest attachment?  Thanks,

/be
Hoping to get a r= by tomorrow.  Shaver?  Bueller?

/be
I applied the patch and compiled js/src only (it doesn't look like any apis 
changed).
xpcshell runs, but mozilla asserts while compiling navigator.js with the 
attached stack. It is blowing the assert in 
_js_LookupProperty's 
  JS_LOCK_OBJ(cx, obj) of 
JS_ASSERT(scope->count > 0 && scope->count <= 4)
with a count of 5.
Are you out of balance somewhere? or did you do something that makes it need a 
higher count? in this debug scope stack thingy?
Should be all better now (unbalanced locking shows up in debug builds with the 
object's scope, *(JSScope*)obj->map, with a count > 4 and file and line all 
stuck at the same offending JS_LOCK_OBJ or js_LookupProperty call -- the one 
that lacks a matching JS_UNLOCK_OBJ (or OBJ_DROP_PROPERTY after looking up a 
property successfully).

/be
Jband, I think this patch (with the jsemit.c fix) will also make your backtrace 
stuff work.  Now any function activation that doesn't have a call object can get 
one via js_GetCallObject safely (because it must not contain a with statement, a 
debugger statement, a direct call to eval, an assignment or mutation of the 
arguments variable, or other things that make functions heavyweight).

/be

/be
This last attachment is what I hope to check in later today, tree gods willing. 
So testers and reviewers, please have at it.

/be
brendan,

I looked at all this. I understand less than I'd like. It looks ok to me. The 
'debugger' statement handler for my stack dumper does not fail on XUL JS now. 
The cases where we'd be calling to make a new call object at an arbitrary 
time from within a debugger are harder to test. I tested making a new call 
object for a function that does not have one (but only in the non-shared case) 
and that seems to work. Is it really OK to override the existing fp->varobj with 
the new callobj?

Also, did you mean to whack the comment in the function_props declaration and 
the block of code in fun_getProperty?
>I looked at all this. I understand less than I'd like. It looks ok to me. The 
>'debugger' statement handler for my stack dumper does not fail on XUL JS now.

Great!  Thanks.
 
>The cases where we'd be calling to make a new call object at an arbitrary 
>time from within a debugger are harder to test.

These changes ensure that only a lightweight function might lack a call object 
at an arbitrary time during function activation, when the debugger API would 
make one.  At that time, it's ok to make one because the scope chain and varobj 
are set to the function's parent, but no variables can be created by the 
function dynamically (the only way that could happen is via eval, but direct 
eval calls make the calling function heavyweight).

>I tested making a new call 
>object for a function that does not have one (but only in the non-shared case) 
>and that seems to work. Is it really OK to override the existing fp->varobj 
>with the new callobj?

Yes, see above.  fp->varobj won't be used by bytecode executing in a lightweight 
by definition.  Any bytecode generated that would use varobj and want it to be 
the call object was generated by jsemit.c code that flags the function as heavy 
(therefore, it gets a call object when invoked).  Am I making sense yet?

>Also, did you mean to whack the comment in the function_props declaration and 
>the block of code in fun_getProperty?

Yes, I meant to whack those things.  Now that function objects never proxy for 
call objects, there is no need to make 'arguments' readonly via a magic setter. 
Also, there's no need to convert 'arity' to __arity__'s slot, etc.  No more __ 
name mangling to avoid collisions along the scope chain, because either the fun 
is lightweight and there is no call object, or it's heavy, and the Call instance 
has a fine, read-write 'arguments' property.

r=jband@netscape.com, where's shaver?  Bueller?

/be
This new patch cleans up the regressions.
Shaver's in NYC at LWE.  I'm checking in, r=jband@netscape.com.  Thanks to John 
and Rob.  Someone stop me fast if there's a prob.

/be
I say go for it.
Fix checked in.  If there are any follow-on bugs, please mail me about them at 
meer.net.  Thanks.

/be
Closing.

/be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Marking Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.