Closed
Bug 23346
Opened 25 years ago
Closed 25 years ago
'call()' inside a function invokes Function.prototype.call
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
M14
People
(Reporter: martin.honnen, Assigned: brendan)
Details
(Keywords: js1.5)
Attachments
(16 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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.
Updated•25 years ago
|
Assignee: mccabe → brendan
Comment 1•25 years ago
|
||
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.
Assignee | ||
Updated•25 years ago
|
Status: NEW → ASSIGNED
Target Milestone: M13
Assignee | ||
Comment 2•25 years ago
|
||
This should be fixed before JS1.5 freezes.
/be
Assignee | ||
Updated•25 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 3•25 years ago
|
||
P1 for JS1.5 deadline fix.
/be
Updated•25 years ago
|
Whiteboard: [JS15]
Removing [JS15] from Status Summary, putting js1.5 in new Keyword field :-)
Assignee | ||
Updated•25 years ago
|
OS: Windows 98 → other
Hardware: PC → All
Assignee | ||
Updated•25 years ago
|
OS: other → All
Comment 5•25 years ago
|
||
added testcase ecma_3/ExecutionContexts/regress-23346.js
Assignee | ||
Updated•25 years ago
|
Target Milestone: M13 → M14
Assignee | ||
Comment 6•25 years ago
|
||
JS1.5 is slipping past M13, sez rginda.
/be
Assignee | ||
Comment 7•25 years ago
|
||
Assignee | ||
Comment 8•25 years ago
|
||
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
Assignee | ||
Comment 9•25 years ago
|
||
Assignee | ||
Comment 10•25 years ago
|
||
Better summary.
/be
Summary: calling function named "call" inside a function calls Function.prototype.call → 'call()' inside a function invokes Function.prototype.call
Assignee | ||
Comment 11•25 years ago
|
||
Looking for an r= tomorrow.
/be
Assignee | ||
Comment 12•25 years ago
|
||
Comment 13•25 years ago
|
||
Trying the patch out now.
Comment 14•25 years ago
|
||
Things look bad, 11 new failures.
Attaching the linux debug test results.
(The only failure without the patch is the one for this bug.)
Comment 15•25 years ago
|
||
Assignee | ||
Comment 16•25 years ago
|
||
Assignee | ||
Comment 17•25 years ago
|
||
Rob, can you revert jsparse.c and apply the latest attachment? Thanks,
/be
Assignee | ||
Comment 18•25 years ago
|
||
Comment 19•25 years ago
|
||
Assignee | ||
Comment 20•25 years ago
|
||
Comment 21•25 years ago
|
||
Assignee | ||
Comment 22•25 years ago
|
||
Assignee | ||
Comment 23•25 years ago
|
||
Assignee | ||
Comment 24•25 years ago
|
||
Hoping to get a r= by tomorrow. Shaver? Bueller?
/be
Comment 25•25 years ago
|
||
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?
Comment 26•25 years ago
|
||
Assignee | ||
Comment 27•25 years ago
|
||
Assignee | ||
Comment 28•25 years ago
|
||
Assignee | ||
Comment 29•25 years ago
|
||
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
Assignee | ||
Comment 30•25 years ago
|
||
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
Assignee | ||
Comment 31•25 years ago
|
||
Assignee | ||
Comment 32•25 years ago
|
||
This last attachment is what I hope to check in later today, tree gods willing.
So testers and reviewers, please have at it.
/be
Comment 33•25 years ago
|
||
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?
Assignee | ||
Comment 34•25 years ago
|
||
>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
Comment 35•25 years ago
|
||
This new patch cleans up the regressions.
Assignee | ||
Comment 36•25 years ago
|
||
Assignee | ||
Comment 37•25 years ago
|
||
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
Comment 38•25 years ago
|
||
I say go for it.
Assignee | ||
Comment 39•25 years ago
|
||
Fix checked in. If there are any follow-on bugs, please mail me about them at
meer.net. Thanks.
/be
Assignee | ||
Comment 40•25 years ago
|
||
Closing.
/be
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•