Reproduces in d8, and bisects to 5f782db9541eca72dc6cbe3c6f5e8dc52ff7c16f.

Indeed, reproes with --no-turbo and --no-lazy.

So, we're deoptimizing an eagerly parsed arrow function which has ({ x } = { x: 1 }) as a param list.

No idea what goes wrong though... :(

Even though this is related to deoptimization happening and --no-turbo, maybe there's some other code path even with turbo that uncovers the same problem.

A guess: The problem is related to the fact that when eager parsing an arrow function in DoParseFunction (*), the non-simple parameter handling is fundamentally different from when the arrow function was parsed the first time. Maybe that creates some inconsistency which then bites us during deopt time.

(*) This happens during deoptimization when --no-turbo

(so I can see the reproduction and spend some time on this)

I've discussed this with Marja.

I think the best solution we have for this right now is to always re-parse arrow functions before they're run. The AST produced the first time around is unreliable, and
it seems very tricky to make it reliable.

`({ x } = { x: 1 })` may or may not be a destructuring assignment, it's unknown until `=>` is seen. However, `({ x } = { y } = { x: 1, y: 2 })` is always at least 1 destructuring assignment, possibly 2, depending on if the `=>` token is seen after. There is a scheme in the parser to remove queued up destructuring assignments (there is a queue of rewritable expressions which are lazily turned into proper AST nodes depending on what they turn out to need to do, and in some cases, the ones queued up for arrow functions are removed from the queue to avoid a crash). This strategy is not sound, because the destructuring assignments which need to be removed is not all-or-nothing, and needs to happen for non-lazy functions as well (as seen by this bug).

at any rate, the result of all of this is, the eagerly parsed arrow function has a different frame-size from the reparsed-for-OSR arrow function, because the reparsed version does not include a duplicate TEMPORARY local variable, while the eagerly parsed version does.

Long-term, we will fare a lot better with a backtracking parser that eliminates the need for the AST node rewriting. Short term, always reparsing is the simple fix, but I think there's a slightly more complicated (but less costly) fix, I'll give that a try.

Also, I believe this doesn't cause problems for TurboFan because when the arrow function is recompiled for OSR, it isn't re-parsed (at least since AstGraphBuilder is gone).

But, I think with lazily parsed arrow functions, a similar situation could happen with turbofan enabled. I don't know exactly how to write that regression test, but I'd be happy to add it to my CL to ensure that that case is fixed as well.

My vote is to WONTFIX this as it doesn't hurt turbofan (provided it turbofan never requires a re-parsed AST/scope for optimization), and rolling out crankshaft seems to already be in progress.

Benedikt, how important is it to fix --no-turbo for these corner cases that most likely don't occur on the web anywhere at this point?

The fixes I proposed don't quite work. To fix the remaining problems, the parameter destructuring in PatternRewriter would need more information to be able to rewrite destructuring assignments within its binding pattern, or queue up those destructuring assignments for later rewriting. Specifically, it would lack the correct scope to evaluate the assignment parts within (not the scope passed to PatternRewriter), as far as I can tell. To fix this, RewritableExpression could have an associated scope member which would provide this information, but I think this complicates other things.

It's fixable, I'm just not sure it's worth it, when instead a focus can be on removing CS or implementing backtracking to eliminate all of these complicated interactions between different AST-rewriting systems.

Thanks for investigating, Caitlin.

Afaics, with I + TF we're never in a situation where we eager-parse a previously eager-parsed function and do OSR. (Can somebody confirm?)

We do reparse previously parsed functions since we sometimes throw away the byte code (not sure when it happens exactly, just observed it in the tests). But when doing that reparsing, we're not in the middle of executing that function.

In the CL you said it maybe can happen for lazy funcs too, but I don't suppose so, since for them we already need to eager parse before executing and the frame size should come from that eager parse.

Michi is clearly the expert here.

ClusterFuzz has detected this issue as fixed in range 478310:478323.

Detailed report: https://clusterfuzz.com/testcase?key=5625401752944640

Fuzzer: inferno_js_fuzzer
Job Type: linux_ubsan_vptr_d8
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  fixed_size_above_fp + (stack_slots * kPointerSize) - CommonFrameConstants::kFixe
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_d8&range=462021:462035
Fixed: https://clusterfuzz.com/revisions?job=linux_ubsan_vptr_d8&range=478310:478323

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5625401752944640


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 5625401752944640 is verified as fixed, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Keeping this open; the Crankshaft-removing commit seems to have "fixed" this, but really, it's still a problem.

I don't think this is properly fixed without supporting re-parsing these cover grammars. Without that, it's very likely for the built scope info to be incorrect in some way, and attempts to fix it by moving stuff around in the scopes seems to show this in some cases.

But, as far as this particular bug is concerned, I think this should be counted as fixed, with a new bug opened for fine-tuning the parsing issue. I'm only commenting on this because it's still open, and I'm going through my open issues.

Marja, wdyt?

Agreed that this is not properly fixed. But let's reuse this bug for properly fixing it - I'm re-assigning this to Adam (who's the most likely person to work on fixing it).

I'm ok with using this for tracking purposes, but perhaps it should be retitled? Marja, can you choose a title that you think matches the specific issue here?

Revisiting this, I doubt it's something I'd fix anytime soon. Passing over to verwaest, in case his fresh eyes see something easy. I recommend talking to marja@ in person to get an overview of this bug, it's rather tricky to derive the root problem from reading the thread.