Closed Bug 1316832 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, jsbugmon, testcase, Whiteboard: [jsbugmon:testComment=3,origRev=d38d06f85ef5])

Attachments

(3 files, 1 obsolete file)

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.
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?
Blocks: 1263355
Flags: needinfo?(shu)
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)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,testComment=3,origRev=d38d06f85ef5]
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.
Whiteboard: [jsbugmon:update,testComment=3,origRev=d38d06f85ef5] → [jsbugmon:update,testComment=3,origRev=d38d06f85ef5,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision bad312aefb42).
Whiteboard: [jsbugmon:update,testComment=3,origRev=d38d06f85ef5,ignore] → [jsbugmon:testComment=3,origRev=d38d06f85ef5,bisect]
Whiteboard: [jsbugmon:testComment=3,origRev=d38d06f85ef5,bisect] → [jsbugmon:testComment=3,origRev=d38d06f85ef5]
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.
This is probably unrelated and the underlying bug is probably still present.
See comment 4 for bug.
Attachment #8816644 - Flags: review?(jwalden+bmo)
Flags: needinfo?(shu)
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.
Attachment #8816645 - Flags: review?(jwalden+bmo)
Attachment #8816644 - Attachment is obsolete: true
Attachment #8816644 - Flags: review?(jwalden+bmo)
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) {
  ...
})();
Attachment #8816645 - Flags: review?(jwalden+bmo) → review+
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9057b86e7b
Do not consider non-final duplicated positional parameter names to be closed over. (r=Waldo)
https://hg.mozilla.org/mozilla-central/rev/5a9057b86e7b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Don't we need to backport this to aurora?
Flags: needinfo?(shu)
(In reply to Hannes Verschore [:h4writer] from comment #13)
> Don't we need to backport this to aurora?

Yeah, guess we do.
Flags: needinfo?(shu)
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
Attachment #8827601 - Flags: approval-mozilla-aurora?
Comment on attachment 8827601 [details] [diff] [review]
bug1316832-uplift.patch

fix crash in debug builds, aurora52+
Attachment #8827601 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → shu
You need to log in before you can comment on or make changes to this bug.