1424946 - Insert a new PerHandlerParser<ParseHandler> class between ParserBase and GeneralParser<ParseHandler, CharT> in the Parser inheritance hierarchy, where handler-centric yet character-agnostic functions may live

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

Summary sez it all.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

I went through all the functions currently (as of prior patches in my queue) in GeneralParser and Parser, and I reassigned all the ones I could to baser classes (mostly PerHandlerParser, but also some ParserBase) as appropriate.  Each move (or set of function moved) will be its own patch, for greatest readability.  Each patch should be ~30s to review as a consequence, except maybe for one of them that moved a few things out of Parser<ParseHandler, CharT> into PerHandlerParser (now that that's possible, albeit with some slight trickiness that you'll understand when you see it).  This patch mostly just introduces the class and does nothing else.

The |template| goo on |alloc.new_| is necessary because |alloc| is a dependent name (because it's found in a template-dependent base).  However, if memory serves, I manage to remove it in future patches.  So don't panic over it just now!

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::checkOptions into a baser class. NOT REVIEWED YET

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::newFunctionBox into a baser class

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::trace into a baser class

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::noteDestructuredPositionalFormalParameter into a baser class

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::{note,has}UsedName into a baser class

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::propagateFreeNamesAndMarkClosedOverBindings into a baser class

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::hasUsedFunctionSpecialName into a baser class

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::declareFunctionThis into a baser class

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Move various GeneralParser::newName-style functions into a baser class

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::declareDotGeneratorName into a baser class

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::finishLexicalScope into a baser class

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::finishFunctionScopes into a baser class

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Move various abstract syntax-parsing functions into PerHandlerParser, out of their current location in Parser specializations

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::finishFunction into a baser class

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::declareFunctionArgumentsObject into a baser class

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::leaveInnerFunction into a baser class

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::prefixAccessorName into a baser class

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::processExport into a baser class

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::processExportFrom into a baser class

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::nextTokenContinuesLet into a baser class

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::checkAndMarkSuperScope into a baser class

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::identifierReference into a baser class

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::stringLiteral into a baser class

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Move GeneralParser::noSubstitutionTaggedTemplate into a baser class

Comment 26

[Tooru Fujisawa [:arai]]

Comment on attachment 8936499 [details] [diff] [review]
Move GeneralParser::{note,has}UsedName into a baser class

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

I'll review remaining patches tonight or tomorrow morning.

Comment 27

[Tooru Fujisawa [:arai]]

Comment on attachment 8936507 [details] [diff] [review]
Move various abstract syntax-parsing functions into PerHandlerParser, out of their current location in Parser specializations

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

::: js/src/frontend/Parser.h
@@ +361,5 @@
> +    //   Return whether the last syntax parse was aborted due to unsupported
> +    //   language constructs.
> +    // If ParseHandler is FullParseHandler:
> +    //   Return whether the last syntax parse was aborted due to unsupported
> +    //   language constructs.

this sounds wrong, compared to the actual implementation that just returns false

@@ +367,5 @@
> +
> +    // If ParseHandler is SyntaxParseHandler:
> +    //   Clear whether the last syntax parse was aborted.
> +    // If ParseHandler is FullParseHandler:
> +    //   Clear whether the last syntax parse was aborted.

the actual implementation does nothing.

Comment 28

[Jeff Walden [:Waldo]]

When ParseHandler = SyntaxParseHandler, |handler| contains a JSAtom* -- a GC pointer.

When a |handler| containing such pointer, is in a class that (maybe just privately) inherits JS::AutoGCRooter, the pointer is presumed to be rooted by the class's trace hook, so no GC hazard is reported.

When |handler| (and that pointer) is *not* in such class -- for example, when it's in PerHandlerParser<ParseHandler> that inherits ParserBase which itself privately inherits JS::AutoGCRooter -- the JSAtom* taints its containing classes as being unrooted pointers.  So suddenly every Parser is an unrooted type.  And calling any function that can GC, while a Parser is on the stack, will fall over.

Every parser must have |checkOptions()| called on it before it's destroyed, and that function can report warnings and such -- which is to say, it can implicitly GC.  But the JSAtom* in SyntaxParseHandler is safe because of the AutoKeepAtoms in ParserBase, which will live beyond the JSAtom* found nested in |SyntaxParseHandler handler;|.  So really, there's no *true* GC hazard here, only a rooting-analysis lapse.  (Of sorts -- arguably indirect inheritance of JS::AutoGCRooter is dangerous to presume safe, because people could add new derived classes that the base-class trace function wouldn't trace correctly.)

There's a significant advantage to having JS::AutoGCRooter in ParserBase and not in PerHandlerParser -- see bug 1424951's discussion of where AutoGCRooter lives.  So we don't want to move it back to where it was.  Because of this, it's unclear just what we want to do to address this.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3667804652
Insert a new PerHandlerParser<ParseHandler> class between ParserBase and GeneralParser<ParseHandler, CharT> in the Parser inheritance hierarchy, where handler-centric yet character-agnostic functions may live.  r=arai, r=sfink on the JS_HAZ_ROOTED use
https://hg.mozilla.org/integration/mozilla-inbound/rev/e934b45172ed
Move GeneralParser::checkOptions into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d12eae2c379
Move GeneralParser::newFunctionBox into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb563243c9a
Move GeneralParser::trace into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bb12747280
Move GeneralParser::noteDestructuredPositionalFormalParameter into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cccf115ff4a1
Move GeneralParser::{note,has}UsedName into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3e41d59542
Move GeneralParser::propagateFreeNamesAndMarkClosedOverBindings into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d562cbf6db2
Move GeneralParser::hasUsedFunctionSpecialName into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/f012bdaf0de4
Move GeneralParser::declareFunctionThis into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f00afc34324
Move various GeneralParser::newName-style functions into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb31ebb78e0
Move GeneralParser::declareDotGeneratorName into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/371794850430
Move GeneralParser::finishLexicalScope into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2878c19d261
Move GeneralParser::finishFunctionScopes into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe57fd2ed05
Move various abstract syntax-parsing functions into PerHandlerParser, out of their current location in Parser specializations.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4daa95c88908
Move GeneralParser::finishFunction into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de7ca55eae4
Move GeneralParser::declareFunctionArgumentsObject into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7119db72646
Move GeneralParser::leaveInnerFunction into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/b971a6efd247
Move GeneralParser::prefixAccessorName into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/a334be8b1d87
Move GeneralParser::processExport into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c250bd5c100d
Move GeneralParser::processExportFrom into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/5996e1f1de79
Move GeneralParser::nextTokenContinuesLet into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f925aeead2
Move GeneralParser::checkAndMarkSuperScope into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/aebd63da51c4
Move GeneralParser::identifierReference into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c33d6cfcd8
Move GeneralParser::stringLiteral into a baser class.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a33f7555936
Move GeneralParser::noSubstitutionTaggedTemplate into a baser class.  r=arai

Comment 30

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1c3667804652
https://hg.mozilla.org/mozilla-central/rev/e934b45172ed
https://hg.mozilla.org/mozilla-central/rev/8d12eae2c379
https://hg.mozilla.org/mozilla-central/rev/6eb563243c9a
https://hg.mozilla.org/mozilla-central/rev/a3bb12747280
https://hg.mozilla.org/mozilla-central/rev/cccf115ff4a1
https://hg.mozilla.org/mozilla-central/rev/4d3e41d59542
https://hg.mozilla.org/mozilla-central/rev/5d562cbf6db2
https://hg.mozilla.org/mozilla-central/rev/f012bdaf0de4
https://hg.mozilla.org/mozilla-central/rev/2f00afc34324
https://hg.mozilla.org/mozilla-central/rev/cdb31ebb78e0
https://hg.mozilla.org/mozilla-central/rev/371794850430
https://hg.mozilla.org/mozilla-central/rev/f2878c19d261
https://hg.mozilla.org/mozilla-central/rev/cfe57fd2ed05
https://hg.mozilla.org/mozilla-central/rev/4daa95c88908
https://hg.mozilla.org/mozilla-central/rev/8de7ca55eae4
https://hg.mozilla.org/mozilla-central/rev/f7119db72646
https://hg.mozilla.org/mozilla-central/rev/b971a6efd247
https://hg.mozilla.org/mozilla-central/rev/a334be8b1d87
https://hg.mozilla.org/mozilla-central/rev/c250bd5c100d
https://hg.mozilla.org/mozilla-central/rev/5996e1f1de79
https://hg.mozilla.org/mozilla-central/rev/d2f925aeead2
https://hg.mozilla.org/mozilla-central/rev/aebd63da51c4
https://hg.mozilla.org/mozilla-central/rev/87c33d6cfcd8
https://hg.mozilla.org/mozilla-central/rev/4a33f7555936

Comment 31

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #28)
> implicitly GC.  But the JSAtom* in SyntaxParseHandler is safe because of the
> AutoKeepAtoms in ParserBase, which will live beyond the JSAtom* found nested
> in |SyntaxParseHandler handler;|.  So really, there's no *true* GC hazard
> here, only a rooting-analysis lapse.  (Of sorts -- arguably indirect
> inheritance of JS::AutoGCRooter is dangerous to presume safe, because people
> could add new derived classes that the base-class trace function wouldn't
> trace correctly.)

That description makes me think that AutoGCRooter-caused immunity *should* be inherited in cases where you don't add any additional unrooted pointer members. I'm not sure how messy that would be to implement; it shouldn't be too bad. But first -- would it have solved this problem without requiring annotations?

Comment 32

[Jeff Walden [:Waldo]]

From what I understand of this, it seems like it would have, yes.  Could be mistaken, but yeah -- assuming the trace-function doesn't trace things outside of inherited fields, yeah, that would be adequate.