Closed Bug 1405701 Opened 3 years ago Closed 3 years ago

new.target is incorrect in Baseline OSR eval frames

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
In EnterBaselineAtBranch we pass |undefined| thisv + newTarget to eval frames. Passing thisv to non-function frames is no longer necessary since the this-rewrite years ago. It's also an observable bug: new.target in OSR eval frames is currently always |undefined|.

The attached patch fixes this, simplifies EnterBaselineAtBranch, and adds a test.
Attachment #8915165 - Flags: review?(tcampbell)
Comment on attachment 8915165 [details] [diff] [review]
Patch

Review of attachment 8915165 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find!

Can you also make the two lines consistent?
- https://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/js/src/jit/Ion.cpp#2843
- https://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/js/src/jit/BaselineJIT.cpp#235

Possibly add a comment to each of jit::SetEnterJitData and jit::EnterBaselineAtBranch informing that the other must be kept in sync. These sorts of things are traps to people new to code (and even those who are).
Attachment #8915165 - Flags: review?(tcampbell) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8367a42358e2
Fix new.target in Baseline OSR eval frames. r=tcampbell
https://hg.mozilla.org/mozilla-central/rev/8367a42358e2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.