Closed Bug 1214013 Opened 4 years ago Closed 4 years ago

Stop parsing toplevel scripts statement at a time

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: shu, Unassigned)

Details

Attachments

(4 files)

It's too hard to be correct w.r.t. new redeclaration early error semantics for global scripts since we parse them a statement at a time. If a syntax parsing error happens in some inner thing to a statement, we clear the outermost ParseContext.

Hacking around this to ensure redeclaration early errors are correct will be hacky and difficult to understand.

I don't think we have a realistic, maintainable way forward without killing this optimization. This will be both a time and space regression, unfortunately.
Attachment #8672866 - Flags: review?(efaustbmo)
Attached patch Add fuzz test.Splinter Review
Attachment #8672869 - Flags: review?(efaustbmo)
Comment on attachment 8672866 [details] [diff] [review]
Parse global scripts non-incrementally.

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

\o/

::: js/src/frontend/BytecodeCompiler.cpp
@@ +601,3 @@
>              if (!pn && !handleStatementParseFailure(scopeChain, evalCaller, pc, globalsc))
>                  return nullptr;
>          } while (!pn);

As an aside, I'm not sure I understand this loop. It's the same as it was before. What if there's some non-syntactic non-solvable condition? What leads us to believe it will get better? Can that even happen?
Attachment #8672866 - Flags: review?(efaustbmo) → review+
Comment on attachment 8672867 [details] [diff] [review]
Remove drainGlobalOrEvalBindings and use generateBindings for all kinds of scripts.

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

This is a nice simplification.
Attachment #8672867 - Flags: review?(efaustbmo) → review+
Attachment #8672868 - Flags: review?(efaustbmo) → review+
Attachment #8672869 - Flags: review?(efaustbmo) → review+
You need to log in before you can comment on or make changes to this bug.