If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: billm)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Also, it seems that, with this fixed, the test "if (cx->fp()->getBlockChain() == lexicalBlock) return ARECORD_STOP" in record_JSOP_LEAVEBLOCK could be removed.
(Assignee)

Comment 2

7 years ago
Created attachment 471354 [details] [diff] [review]
patch to fix this

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)
(Reporter)

Comment 3

7 years ago
Comment on attachment 471354 [details] [diff] [review]
patch to fix this

Cool, thanks.  Need a checkin?
Attachment #471354 - Flags: review?(lw) → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 4

7 years ago
(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
(Reporter)

Comment 6

7 years ago
(In reply to comment #5)
Good catch

http://hg.mozilla.org/tracemonkey/rev/391c5c261186
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
Depends on: 593596
Assignee: general → wmccloskey
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 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.