1496331 - Assertion failure: bi.kind() == BindingKind::FormalParameter, at js/src/frontend/EmitterScope.cpp:623

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Tooru Fujisawa [:arai]]

Attachment: param.binjs

tested on m-i 21b67d2084a6

Configure flags: --enable-warnings-as-errors --disable-optimize --enable-debug

Runtime flag: -B param.binjs

Result:
Assertion failure: bi.kind() == BindingKind::FormalParameter, at js/src/frontend/EmitterScope.cpp:623

Comment 1

[Tooru Fujisawa [:arai]]

encoded from js/src/jit-test/tests/basic/testBug772328.js

I'll add all testcase in bug 1495611, so no need to add it here

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Do not alter declaration kind from FormalParameter to BodyLevelFunction.

as regular Parser does, we should check the current declaration kind before altering.
https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/js/src/frontend/Parser.cpp#1516-1549

in this case, it was altering FormalParameter to BodyLevelFunction, which shouldn't happen.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9016166 [details] [diff] [review]
Do not alter declaration kind from FormalParameter to BodyLevelFunction.

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

::: js/src/frontend/BinSource.cpp
@@ +241,5 @@
>  
>      if (parseContext_ && syntax == FunctionSyntaxKind::Statement) {
>          auto ptr = parseContext_->varScope().lookupDeclaredName(atom);
>          MOZ_ASSERT(ptr);
> +        // FIXME: Should be merged with ParseContext::tryDeclareVarHelper.

Can you file a bug?

@@ +244,5 @@
>          MOZ_ASSERT(ptr);
> +        // FIXME: Should be merged with ParseContext::tryDeclareVarHelper.
> +        DeclarationKind declaredKind = ptr->value()->kind();
> +        if (DeclarationKindIsVar(declaredKind)) {
> +            MOZ_ASSERT(declaredKind != DeclarationKind::VarForAnnexBLexicalFunction);

Mmmmh... Isn't that something that an ill-formed file could achieve?

::: js/src/frontend/Parser.h
@@ +1699,5 @@
>  JSFunction*
>  AllocNewFunction(JSContext* cx, HandleAtom atom, FunctionSyntaxKind kind, GeneratorKind generatorKind, FunctionAsyncKind asyncKind,
>                   HandleObject proto, bool isSelfHosting = false, bool inFunctionBox = false);
>  
> +bool

Could you take the opportunity to document this method?

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #3)
> @@ +244,5 @@
> >          MOZ_ASSERT(ptr);
> > +        // FIXME: Should be merged with ParseContext::tryDeclareVarHelper.
> > +        DeclarationKind declaredKind = ptr->value()->kind();
> > +        if (DeclarationKindIsVar(declaredKind)) {
> > +            MOZ_ASSERT(declaredKind != DeclarationKind::VarForAnnexBLexicalFunction);
> 
> Mmmmh... Isn't that something that an ill-formed file could achieve?

the actual implementation for annex B will be handled in bug 1496334
I'm going to keep it an assertion for now.

Comment 5

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a298e5b921f7dde9e3e36f25fae60a950e94a20b
Bug 1496331 - Do not alter declaration kind from FormalParameter to BodyLevelFunction. r=Yoric

Comment 6

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a298e5b921f7

Comment 7

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I need some additional info regarding how to use the runtime flag (-B param.binjs) specified in comment 0, in order to reproduce/verify the current issue.

Comment 8

[Tooru Fujisawa [:arai]]

it's a bit complicated to verify this on Nightly (given it needs a web server that serves the attached binjs file with special MIME-Type).

If you're fine with testing with the debug build of JS shell, it's simple, here's the STR:
  1. download debug build of JS shell from treeherder
  2. download the attached .binjs file as "a.binjs"
  3. run the shell with `js -B a.binjs` command

If you can prepare a private local web server that can have special MIME-Type, here's the STR with Nightly:
  1. download debug build of Nightly
  2. set dom.script_loader.binast_encoding.enabled pref to true in Nightly
  3. download the attached .binjs file as "a.binjs"
  4. create a file named "index.html" with the following content:
       <script src="a.binjs" async></script>
  5. put a.binjs and index.html in the web server
  6. associate .binjs extension with "application/javascript-binast" MIME-Type in the web server
  7. open index.html from the web server with Nightly

this fixes assertion failure which can result in wrong pointer access or something.

Comment 9

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I tried both of your STR versions, but without managing to notice a difference between the affected build and the fixed version:
- downloaded the debug build of the JS shell (target.jsshell.zip) from the corresponding treeherder location, extracted the shell, saved the attached .binjs file and ran the shell with the specified command.
- installed the web server (tried with both Cheyenne and XAMP), set the "dom.script_loader.binast_encoding.enabled" pref to true in the corresponding Fx debug build, saved the attached .binjs file and moved it in the web server's upload folder (e.g. xampp\htdocs\) along with the index.html file, asociated .binjs extension with "application/javascript-binast" (e.g. set "application/javascript-binast .binjs" in the server's configuration file), then opened index.html using the debug build of Fx.
Both affected build and the fixed one had the same behavior and also, no "Assertion failure: bi.kind() == BindingKind::FormalParameter" was noticed in terminal or Browser Console. 
Could you please confirm if the fix was properly implemented on 64 version?

Comment 10

[Tooru Fujisawa [:arai]]

confirmed on macOS
failure on debug build of js shell from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=c1ee6c65f85b0ea68006a3802db0937220d55da3&selectedJob=205636718
fixed on debug build of js shell from https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=4c11ab0cd98950983cfc957f579ace6c3e918a43&selectedJob=205637088

Comment 11

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Thank you, :arai!
I will update the corresponding flags, per comment 10.