978802 - Assertion failure: frame.script()->needsArgsObj(), at vm/ArgumentsObject.cpp:238

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 4cb766685b73 (run with --fuzzing-safe):


try {
gcparam("maxBytes", gcparam("gcBytes") + 4*1024);
var max = 400;
function f(b) {
    if (b) {
        f(b - 1);
    } else {
        g = {};
    }
    g.apply(null, arguments);
}
f(max - 1);
} catch(exc0) {}
f();

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/e0c3de95b861
user:        Luke Wagner
date:        Wed May 01 11:04:06 2013 -0700
summary:     Bug 865960 - JS OOM should throw instead of silently stopping execution (r=billm)

This iteration took 338.028 seconds to run.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision fcf19894d9f3).

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Fix Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "bad" changeset has the timestamp "20140425114406" and the hash "83ea7136632f".
The "good" changeset has the timestamp "20140425120234" and the hash "aa534ca9cea5".

Likely fix window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=83ea7136632f&tochange=aa534ca9cea5

Comment 5

[Christian Holler (:decoder)]

> aa534ca9cea5 Brian Hackett — Bug 995336 - Use IonBuilder for arguments usage analysis, r=jandem.

This looks somewhat relevant. Brian, did this fix the failure above (which looks like OOM to me), or did it just cover the problem?

Comment 6

[Brian Hackett [Laid off!]]

Bug 995336 made some changes to where we trigger the arguments usage analysis, so yeah it could have fixed this.

Comment 7

[Christian Holler (:decoder)]

This doesn't reproduce anymore and the signature also doesn't show up in FuzzManager, so I assume it's fixed.

Comment 8

[Jon Coppeard (:jonco)]

I can reproduce this so re-opening.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: bug978802-arguments-object

The problem is that JSScript::argumentsOptimizationFailed() clears JSScript::needsArgsObj_ if allocation failes, but there are a couple of places we assert this is always true.  I think we can fix this by also checking the NEEDS_ARGS_OBJ flag in the baseline script as this will always be set in this case.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8670265 [details] [diff] [review]
bug978802-arguments-object

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

I think it'd be nice to ensure the flags are in sync by doing

  script->baselineScript()->setNeedsArgsObj(false);

Where we do script->needsArgsObj_ = false (or maybe we should add a JSScript setter that sets both).

Does that fix this?

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #10)

That makes sense, but doesn't fix the problem.  The problem is that we assert JSScript::needsArgsObj() in ArgumentsObject::createExpected() which is called for a baseline-compiled JSOP_ARGUMENTS.  So if we set needsArgsObj_ to false then we will assert the next time this executes.

I don't understand what these argument object optimisations are doing.  BaselineCompiler::emit_JSOP_ARGUMENTS() does emit a guard on the baseline script's NEEDS_ARGS_OBJ flag if the JSScript's flag is false to start with, since it can change from false to true.  Maybe it should also check where the flag changes in the other direction?  But then I'm not sure what it should return.

> Comment on attachment 8670265 [details] [diff] [review]
> bug978802-arguments-object
> 
> Review of attachment 8670265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it'd be nice to ensure the flags are in sync by doing
> 
>   script->baselineScript()->setNeedsArgsObj(false);
> 
> Where we do script->needsArgsObj_ = false (or maybe we should add a JSScript
> setter that sets both).
> 
> Does that fix this?

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #11)
> I don't understand what these argument object optimisations are doing. 
> BaselineCompiler::emit_JSOP_ARGUMENTS() does emit a guard on the baseline
> script's NEEDS_ARGS_OBJ flag if the JSScript's flag is false to start with,
> since it can change from false to true.  Maybe it should also check where
> the flag changes in the other direction?  But then I'm not sure what it
> should return.

Hm good point. NewArgumentsObjectInfo could do

  if (!frame->script()->...) {
      // Find out.
      res.setMagic(JS_OPTIMIZED_ARGUMENTS);
      return true;
  }

But it's not really nice. This might be one of the cases where crashing on OOM is cleaner. argumentsOptimizationFailed is not hot and it's just allocating an ArgumentsObject, so it's unlikely to crash much I think.

Comment 13

[Jon Coppeard (:jonco)]

Attachment: bug978802-arguments-object v2

Great, here's a patch.

Comment 14

[Jon Coppeard (:jonco)]

I needed add an allow-unhandlable-oom directive in jit-test/tests/baseline/bug847425.js to make this test pass.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d03295efd5f

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2d03295efd5f