Closed Bug 590006 Opened 11 years ago Closed 11 years ago

escaping closures on trace don't get block objects in their scope chain

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: billm)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

If we create a closure on trace that needs a scope chain, that scope chain doesn't contain block objects.  Thus, in the following code, the reference to 'local' is undefined when called after f() returns.

function f() {
    var ret;
    for (var i = 0; i < 10; ++i) {
        let (local = 3) {
            ret = function() { print(local++); }
        }
    }
    return ret;
}
f()();  // test.js:5: ReferenceError: local is not defined

I think the fix is to abort trace when creating a function object where the scope chain would contain block objects.
Also, it seems that, with this fixed, the test "if (cx->fp()->getBlockChain() == lexicalBlock) return ARECORD_STOP" in record_JSOP_LEAVEBLOCK could be removed.
This patch aborts tracing if we record a LAMBDA instruction when the block chain is non-null. I also removed the useless lexicalBlock check. The patch fixes the test case above and passes trace-tests.

I added a similar check to the LAMBDA_FC instruction. I can't figure out a way to make a flat closure that actually accesses its scope chain, but it still seems prudent to have the check there.
Attachment #471354 - Flags: review?(lw)
Comment on attachment 471354 [details] [diff] [review]
patch to fix this

Cool, thanks.  Need a checkin?
Attachment #471354 - Flags: review?(lw) → review+
(In reply to comment #3)
> Comment on attachment 471354 [details] [diff] [review]
> patch to fix this
> 
> Cool, thanks.  Need a checkin?

Yes, thanks.
Comment on attachment 471354 [details] [diff] [review]
patch to fix this

RETURN_STOP_A("reason"); is nicer than bare return ARECORD_STOP; for future JIT spew readers debugging "why isn't my let-based code tracing" situations.

/be
(In reply to comment #5)
Good catch

http://hg.mozilla.org/tracemonkey/rev/391c5c261186
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
Assignee: general → wmccloskey
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 568708
Duplicate of this bug: 545759
You need to log in before you can comment on or make changes to this bug.