Closed
Bug 1316832
Opened 8 years ago
Closed 8 years ago
Assertion failure: self->contains(cx, aprop), at js/src/vm/Shape.cpp:1059
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: gkw, Assigned: shu)
References
Details
(Keywords: assertion, bugmon, testcase, Whiteboard: [jsbugmon:testComment=3,origRev=d38d06f85ef5])
Attachments
(3 files, 1 obsolete file)
8.44 KB,
text/plain
|
Details | |
2.89 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
![]() |
Reporter | |
Comment 1•8 years ago
|
||
![]() |
Reporter | |
Comment 2•8 years ago
|
||
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)
![]() |
Reporter | |
Comment 3•8 years ago
|
||
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]
Assignee | ||
Comment 4•8 years ago
|
||
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.
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,testComment=3,origRev=d38d06f85ef5] → [jsbugmon:update,testComment=3,origRev=d38d06f85ef5,ignore]
Comment 5•8 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision bad312aefb42).
![]() |
Reporter | |
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,testComment=3,origRev=d38d06f85ef5,ignore] → [jsbugmon:testComment=3,origRev=d38d06f85ef5,bisect]
Updated•8 years ago
|
Whiteboard: [jsbugmon:testComment=3,origRev=d38d06f85ef5,bisect] → [jsbugmon:testComment=3,origRev=d38d06f85ef5]
Comment 6•8 years ago
|
||
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.
![]() |
Reporter | |
Comment 7•8 years ago
|
||
This is probably unrelated and the underlying bug is probably still present.
Assignee | ||
Comment 8•8 years ago
|
||
See comment 4 for bug.
Attachment #8816644 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(shu)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8816644 -
Attachment is obsolete: true
Attachment #8816644 -
Flags: review?(jwalden+bmo)
Comment 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 14•8 years ago
|
||
(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)
Assignee | ||
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Updated•8 years ago
|
Assignee: nobody → shu
Comment 17•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•