545573 - partial flat closure source coordinate fencepost and upvar order-sensitive bugs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Attachment: fix with test

See patch for test and fix.

/be

Comment 1

[Brendan Eich [:brendan]]

This requires a partial flat closure, as the test shows.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix with tests, v2

TR::record_JSOP_LAMBDA_FC aborts if the compiler-created function object does not have this->globalObj as its parent, but then it relies on scopeChain() to compile the parent to pass to js_AllocFlatClosure. How can that be right when in block scope with a flat upvar bound to a let in that block?

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix, etc., v3

But broken on new trace-test/tests/closures/partial-flat-closure-1.js -- Jason has thoughts on how to fix.

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 426550 [details] [diff] [review]
fix, etc., v3

Two great catches here. I'm in favor of pushing the two fixes without the last test, and filing a separate bug for that.

Comment 5

[Jason Orendorff [:jorendorff]]

There are two separate bugs related to the last test.

1. TR::record_LAMBDA_FC creates a closure parented to the global instead of the current scope chain. It needs to call TR::scopeChain, as TR::record_LAMBDA does.

2. TR::scopeChain() returns the wrong scope chain when we're in a let block.

The easiest fix is to make TR::scopeChain() fallible and have it call js_GetScopeChain and walk the interpreter's stack chain (up to the entry frame) looking for Block objects. If it finds one, abort recording.

Comment 6

[Brendan Eich [:brendan]]

Ok, filed bug 545759 per comment 5. Fix without partial-flat-closure-1.js test is in tracemonkey:

http://hg.mozilla.org/tracemonkey/rev/c1a0484b8a23

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c1a0484b8a23