JS nested function environment capture broken

VERIFIED FIXED in mozilla1.4beta

Status

()

Core
JavaScript Engine
P1
major
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.4beta
js1.5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

15 years ago
Test case for the JS shell coming up.  The problem is that call_enumerate does
not refresh the values of call object properties that already were resolved. 
Instead it assumes that all properties have yet to be resolved.  Patch coming
right after the test.

/be
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.4beta
(Assignee)

Comment 1

15 years ago
Created attachment 121093 [details]
js shell test case

Variation showing two environment captures coming up next.

/be
(Assignee)

Comment 2

15 years ago
Created attachment 121094 [details]
variation on last test case
(Assignee)

Comment 3

15 years ago
Created attachment 121095 [details] [diff] [review]
proposed fix

If call_enumerate looks up a property not yet resolved, it'll be defined with
the current value from the appropriate stack frame vector (argv or vars).

But if, as in the test cases, call_enumerate is called when capturing an
environment (from js_PutCallObject on return from a heavyweight function),
*and* that heavyweight called an inner function that referenced the outer arg
or var, then the outer frame has a call object that's already resolved the call
property reflecting the stack slot.  We must copy from the stack slot in this
case, and we can't distinguish it from the first case within call_enumerate, so
we just copy each enumerated property's value from the appropriate slot.

The extra load, store, and address computations are tiny in the first case, I
think.	I'm not worried about any optimizations until we fix this bad old bug,
in any event!

/be
(Assignee)

Updated

15 years ago
Attachment #121095 - Flags: review?(shaver)
Comment on attachment 121095 [details] [diff] [review]
proposed fix

I'm _stunned_ that we didn't see this before. r=shaver.
Attachment #121095 - Flags: review?(shaver) → review+
(Assignee)

Comment 5

15 years ago
Fixed -- thanks!

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 6

15 years ago
Verified FIXED.

Testcases added to JS testsuite:

      mozilla/js/tests/js1_5/Scope/regress-202678-001.js
      mozilla/js/tests/js1_5/Scope/regress-202678-002.js

Both pass in SpiderMonkey with the above patch applied -
Status: RESOLVED → VERIFIED
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.