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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: 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)

Comment 1

[Luke Wagner [:luke]]

XML!!!!!

Comment 2

[Luke Wagner [:luke]]

err, E4X!!!!!!

Comment 4

[Luke Wagner [:luke]]

Attachment: rage

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.

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Luke Wagner [:luke]]

Attachment: better patch

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?

Comment 7

[Jeff Walden [:Waldo]]

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...)

Comment 8

[Luke Wagner [:luke]]

Rage, indeed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/065dc9204d09

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/065dc9204d09

Comment 10

[juan becerra [:juanb]]

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.

Comment 11

[u279076]

Gary, any assistance you can give QA in verifying this fix?

Comment 12

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

(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>

Comment 13

[u279076]

Thanks Gary, would it be possible to have you spotcheck this against Beta?

Comment 14

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

(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>

Comment 15

[u279076]

Thank you very much Gary.

Comment 16

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

(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)

Comment 17

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/e4x/bug731724.js.