1604988 - BinAST lazy parsing doesn't set flags correctly for arguments keyword

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P5)

Description

[Ted Campbell [:tcampbell]]

The normal text lazy parser has [1], but the BinAST parser does not. This causes the handling of [2] to mis-compute flags and the arguments analysis isn't run correctly. This leads to type confusion in IonMonkey.

Ideally we should enforce the same constraints in Bug 1604064 for BinAST as well since flag issues are often sec bugs. We should be able to spot fix this arguments case though so that Bug 1604069 can be landed.

[1] https://searchfox.org/mozilla-central/rev/6305f6935f496b3a302c7afcc579399a4217729c/js/src/frontend/Parser.cpp#1843-1845
[2] https://searchfox.org/mozilla-central/rev/6305f6935f496b3a302c7afcc579399a4217729c/js/src/frontend/ParseContext.cpp#552-555

(Marking sec-other because BinAST isn't enabled by default)

Comment 1

[Tooru Fujisawa [:arai]]

The problem here is that we don't parse lazy function body at all, while it has to set correct flag about whether the body contains this/arguments.

BinAST webidl contains isThisCaptured (that we don't use), but arguments usage isn't encoded.
I guess we can set that both this/arguments are used, regardless of the lazy function body? (if that just regress perf, without breaking any logic)

Comment 2

[Tooru Fujisawa [:arai]]

hmm, even if I made BinASTParserPerTokenizer<Tok>::finishLazyFunction to always note and declare dotThis/arguments and do the same thing as LazyScriptCreationData::create for copying flags, it crashes with Assertion failure: Lambda instruction returned object with unexpected type.

will investigate tomorrow.

Comment 3

[Tooru Fujisawa [:arai]]

Can you provide the detail about the type confusion?
I'm having trouble figuring out how it reaches Assertion failure: Lambda instruction returned object with unexpected type.

Comment 4

[Ted Campbell [:tcampbell]]

I've looked into this again. The probably for IonMonkey is that for the BinAST case, the definition of IsLikelyConstructorWrapper is inconsistent between a functions lazy and non-lazy form. This cause TI to break leading to that assertion.

The workaround I'll go with now is to ensure that the flag doesn't change during delazification (by not updating when BCE::emitterMode == LazyFunction). This unblocks Bug 1604069.

Longer term, we will be removing TI and this optimization so won't have need to lazy scripts to know if they use arguments or not. It is a bit unfortunate that the flags are inconsistent between lazy and non-lazy parsing for BinAST, but the focus will become on what SmooshMonkey needs.