1316832 - Assertion failure: self->contains(cx, aprop), at js/src/vm/Shape.cpp:1059

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision d38d06f85ef5 (build with --32 --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

(function(x, x) {
    eval("\
        var y = 1;\
        function f() {\
        	return delete y;\
        }\
        f();\
    ");
}())()

Backtrace:

#0  js::NativeObject::removeProperty (this=0xf65125c8, cx=0xf7149000, id_=...) at js/src/vm/Shape.cpp:1059
#1  0x08752552 in js::NativeDeleteProperty (cx=0xf7149000, obj=..., id=..., result=...) at js/src/vm/NativeObject.cpp:2559
#2  0x081427ad in js::DeleteProperty (cx=0xf7149000, obj=..., id=..., result=...) at js/src/jsobjinlines.h:234
#3  0x087528c7 in js::DeleteNameOperation (cx=0xf7149000, name=..., scopeObj=..., res=...) at js/src/vm/Interpreter.cpp:4610
/snip

For detailed crash information, see attachment.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/cb6fc6d38f8d
user:        Shu-yu Guo
date:        Thu Aug 25 01:28:47 2016 -0700
summary:     Bug 1263355 - Rewrite the frontend: bindings. (r=jorendorff,Waldo)

changeset:   https://hg.mozilla.org/mozilla-central/rev/18bec78f348e
user:        Shu-yu Guo
date:        Thu Aug 25 01:28:47 2016 -0700
summary:     Bug 1263355 - Report memory metrics for Scopes. (r=njn)

Shu-yu, is bug 1263355 a likely regressor?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision d38d06f85ef5 (build with --32 --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

(function(x, x) {
    eval("\
        var y = 1;\
        function f() {\
            return delete y;\
        }\
        f();\
    ");
}())()

(There was an accidental tab in the testcase in comment 0)

Comment 4

[Shu-yu Guo [:shu]]

Bug is that the environment shape for functions with duplicated parameters end up containing multiple shapes with the same name. This messes up when going into dictionary mode. Need to think of the right way to fix.

Comment 5

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision bad312aefb42).

Comment 6

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8f96a0ce280f
user:        Yura Zenevich
date:        Mon Aug 29 11:06:48 2016 -0400
summary:     Bug 527003 - make a11y service store info about its consumers. Only shut down a11y when there are no more consumers remaining. r=surkov, tbsaunde

This iteration took 239.921 seconds to run.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is probably unrelated and the underlying bug is probably still present.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: Never consider duplicated positional parameter names to be closed over.

See comment 4 for bug.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Do not consider non-final duplicated positional parameter names to be closed over.

Previous patch was incorrect. It had it backwards, considering the first
parameter to be closed over. Instead, only the final duplicate should be closed
over.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8816645 [details] [diff] [review]
Do not consider non-final duplicated positional parameter names to be closed over.

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

Not sure why, but this testcase doesn't fail for me -- and it hits the call issue I flag as the second comment on the test.  Is this really 32-bit-only, as comment 0 seems to imply?  Can't see why this wouldn't reproduce in my default 64-bit debug build.

::: js/src/frontend/Parser.cpp
@@ +1716,5 @@
> +            // If the parameter name has duplicates, only the final parameter
> +            // name should be on the environment, as otherwise the environment
> +            // object would have multiple, same-named properties.
> +            if (hasDuplicateParams) {
> +                for (size_t j = pc->positionalFormalParameterNames().length() - 1; j != i; j--) {

Prefer |j > i| so that the loop terminates if something goes really bonkers and |j == i| were to be skipped over.

@@ +1717,5 @@
> +            // name should be on the environment, as otherwise the environment
> +            // object would have multiple, same-named properties.
> +            if (hasDuplicateParams) {
> +                for (size_t j = pc->positionalFormalParameterNames().length() - 1; j != i; j--) {
> +                    if (pc->positionalFormalParameterNames()[j] == name && j != i) {

Isn't |j != i| here redundant with the condition guarding entering the loop?

@@ +1718,5 @@
> +            // object would have multiple, same-named properties.
> +            if (hasDuplicateParams) {
> +                for (size_t j = pc->positionalFormalParameterNames().length() - 1; j != i; j--) {
> +                    if (pc->positionalFormalParameterNames()[j] == name && j != i) {
> +                        MOZ_ASSERT(j > i);

This would be redundant with the better form of the condition.

::: js/src/jit-test/tests/parser/bug-1316832.js
@@ +5,5 @@
> +        function f() {\
> +            return delete y;\
> +        }\
> +        f();\
> +    ");

Use `` instead of backslashes, for readability.

@@ +6,5 @@
> +            return delete y;\
> +        }\
> +        f();\
> +    ");
> +}())()

So we call the function.  Makes sense.  But then we call *that* value?  Looks like the function doesn't actually return anything.  Why can't this be

(function(x, x) {
  ...
})();

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9057b86e7b
Do not consider non-final duplicated positional parameter names to be closed over. (r=Waldo)

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a9057b86e7b

Comment 13

[Hannes Verschore [:h4writer]]

Don't we need to backport this to aurora?

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Hannes Verschore [:h4writer] from comment #13)
> Don't we need to backport this to aurora?

Yeah, guess we do.

Comment 15

[Shu-yu Guo [:shu]]

Attachment: bug1316832-uplift.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1263355
[User impact if declined]: crashes in rare cases in debug builds with functions with duplicate parameter names, and possible incorrect behavior in non-debug builds
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: Bug fix only
[String changes made/needed]: None

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8827601 [details] [diff] [review]
bug1316832-uplift.patch

fix crash in debug builds, aurora52+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0693440a2059