Closed Bug 731724 Opened 8 years ago Closed 8 years ago

"Assertion failure: !callobj.arguments().isMagic(JS_UNASSIGNED_ARGUMENTS),"

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox13 --- verified

People

(Reporter: gkw, Assigned: luke)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [qa+] js-triage-done)

Attachments

(2 files, 1 obsolete file)

Attached file stack
function g(s) {
    return eval(s)
}
f = g("(function(){({x:function::arguments})})")
f()

asserts js debug shell on m-c changeset 8ec22af2fe91 without any CLI arguments at Assertion failure: !callobj.arguments().isMagic(JS_UNASSIGNED_ARGUMENTS),

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   87894:bd71047c9b4d
user:        Luke Wagner
date:        Tue Feb 07 12:34:29 2012 -0800
summary:     Bug 724790 - get rid of the hasOverriddenArgs funny business (r=waldo)
XML!!!!!
err, E4X!!!!!!
Duplicate of this bug: 731745
Attached patch rage (obsolete) — Splinter Review
Ah, function::x.  How nice... http://triumph.ytmnd.com.

Someone else already made every occurrence of TOK_DBLDOT in Parser.cpp TCF_FUN_HEAVYWEIGHT except this one case, so I was *this* close to not getting punched in the face.  Easy fix, though.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #601876 - Flags: review?(jwalden+bmo)
Comment on attachment 601876 [details] [diff] [review]
rage

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

I was slightly disappointed that your link was not actually to this, as I had predicted:

http://goodevil.ytmnd.com/

I am not sure which reference would have left me more speechless.
Attachment #601876 - Flags: review?(jwalden+bmo) → review+
Attached patch better patchSplinter Review
Oh wait.  I remember why I used "containsEval()" and not (flags & TCF_FUN_HEAVYWEIGHT): a function can be heavyweight for not-bad things like containing a nested closure that accesses upvars.  Thus, the previous patch would have significantly deoptimized 'arguments' for such functions.

This patch goes the other way by leaving mayOverwriteArguments alone and instead conservatively saying that all the uses of :: may overwrite arguments.  If I grep correctly, there are only three places that do this; do you know of any other weird cases Waldo?
Attachment #601876 - Attachment is obsolete: true
Attachment #602062 - Flags: review?(jwalden+bmo)
Comment on attachment 602062 [details] [diff] [review]
better patch

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

Clarify the comment next to TCF_FUN_LOCAL_ARGUMENTS to indicate that this doesn't mean the function *definitely* has an arguments local.  (I'm sad about weakening the meaning of the flag that way, but the existing code structure makes hard to detect only function::arguments as a PrimaryExpression.  Hate hate hate...)
Attachment #602062 - Flags: review?(jwalden+bmo) → review+
Rage, indeed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/065dc9204d09
Whiteboard: js-triage-needed → js-triage-done
Target Milestone: --- → mozilla13
http://hg.mozilla.org/mozilla-central/rev/065dc9204d09
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: js-triage-done → [qa+] js-triage-done
I wasn't able to verify this. It does not assert on the js shell with the latest, but I also wasn't able to see the original problem.
Gary, any assistance you can give QA in verifying this fix?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> Gary, any assistance you can give QA in verifying this fix?

Verifying fixed.

Asserts on m-c changeset 8ec22af2fe91 32-bit Mac OS X 10.7 debug js shell:

js> function g(s) {
    return eval(s)
}
js> f = g("(function(){({x:function::arguments})})")
(function () {({x: function::arguments});})
js> f()
Assertion failure: !callobj.arguments().isMagic(JS_UNASSIGNED_ARGUMENTS), at /Users/skywalker/Desktop/jsfunfuzz-mozilla-central-vIPRYv-8ec22af2fe91-87948/compilePath/js/src/vm/ScopeObject.cpp:284
Bus error: 10

Does not assert on m-c changeset f28d1ec8bd33 (tip as of now) 32-bit Mac OS X 10.7 debug js shell:

js> function g(s) {
    return eval(s)
}
js> f = g("(function(){({x:function::arguments})})")
(function () {({x: function::arguments});})
js> f()
js>
Status: RESOLVED → VERIFIED
Thanks Gary, would it be possible to have you spotcheck this against Beta?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> Thanks Gary, would it be possible to have you spotcheck this against Beta?

Does not assert on mozilla-beta changeset 42f99a6eeb62 (tip as of now) 32-bit Mac OS X 10.7 debug js shell:

js> function g(s) {
    return eval(s)
}
js> f = g("(function(){({x:function::arguments})})")
(function () {({x: function::arguments});})
js> f()
js>
Thank you very much Gary.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #15)
> Thank you very much Gary.

You're welcome! (in the process I discovered bug 759880 on mozilla-beta as well)
A testcase for this bug was automatically identified at js/src/jit-test/tests/e4x/bug731724.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.