877936 - Assertion failure: CountArgSlots(script, fun) < SNAPSHOT_MAX_NARGS, at ion/Snapshots.cpp:301

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision f66d956d188e (run with --ion-eager):


rex = RegExp("()()()()()()()()()()(z)?(y)");
a = ["sub"];
a[230] = '' + "a"
f = Function.apply(null, a);
"xyz".replace(rex, f);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

This affects at least beta by now. Can someone look at this?

Comment 3

[Hannes Verschore [:h4writer]]

There is a limit SNAPSHOT_MAX_NARGS and we check for this. Upon deciding if we want to compile a script we check that the fun->nargs are lower than this limit. 
Now the assert checks if CountArgSlots is lower than that limit, but this adds "this" value, "scopechain" value, "return" value and sometimes argumentsobject value. So there is a decrepancy of 3-4. Or we need to lower the check in Ion.cpp, before compiling. Or we need to only check fun->nargs, instead of CountArgSlots().
This off course depends on the fact if this limit has a reason or is arbitrarely taken.

Looking...

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: bug877936-callargs

1) Increases the limit +4 (for this, return value, argumentsobj ...)
2) Checks for fun args and limits us too (Since we fill/padd args with undefined, this is also a way to go over the limit)
3) Add checks during inlining

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 821995 [details] [diff] [review]
bug877936-callargs

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

::: js/src/jit/Snapshots.cpp
@@ +300,5 @@
>  
>  void
>  SnapshotWriter::startFrame(JSFunction *fun, JSScript *script, jsbytecode *pc, uint32_t exprStack)
>  {
> +    JS_ASSERT(CountArgSlots(script, fun) < SNAPSHOT_MAX_NARGS + 4);

nit: comment explaining the magic 4

Comment 6

[Hannes Verschore [:h4writer]]

We already missed beta, though there is no security issue by not uplifting. So I'll leave that.
I can uplift for aurora (very easy patch), but I don't think it is needed...

@dvander: thanks for quick review
https://hg.mozilla.org/integration/mozilla-inbound/rev/86650bc9c33d

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/86650bc9c33d

Comment 8

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature