Closed Bug 978802 Opened 8 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox30 --- affected
firefox44 --- fixed

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:])

Attachments

(2 files, 1 obsolete file)

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();
Blocks: 912928
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision fcf19894d9f3).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
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
> 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?
Flags: needinfo?(bhackett1024)
Bug 995336 made some changes to where we trigger the arguments usage analysis, so yeah it could have fixed this.
Flags: needinfo?(bhackett1024)
This doesn't reproduce anymore and the signature also doesn't show up in FuzzManager, so I assume it's fixed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee: nobody → jcoppeard
I can reproduce this so re-opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch bug978802-arguments-object (obsolete) — Splinter Review
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.
Attachment #8670265 - Flags: review?(jdemooij)
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?
Attachment #8670265 - Flags: review?(jdemooij)
(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?
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
Great, here's a patch.
Attachment #8670265 - Attachment is obsolete: true
Attachment #8670755 - Flags: review?(jdemooij)
Attachment #8670755 - Flags: review?(jdemooij) → review+
I needed add an allow-unhandlable-oom directive in jit-test/tests/baseline/bug847425.js to make this test pass.
https://hg.mozilla.org/mozilla-central/rev/2d03295efd5f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.