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

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla44
x86
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30 affected, firefox44 fixed)

Details

(Whiteboard: [jsbugmon:])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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();
(Reporter)

Comment 1

4 years ago
Created attachment 8384632 [details]
[crash-signature] Machine-readable crash signature
(Reporter)

Updated

4 years ago
Blocks: 912928
status-firefox30: --- → affected
Whiteboard: [jsbugmon:update,bisect]
(Reporter)

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 2

4 years ago
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.
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
(Reporter)

Comment 3

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

Updated

3 years ago
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
(Reporter)

Comment 4

3 years ago
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
(Reporter)

Comment 5

3 years ago
> 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)
(Reporter)

Comment 7

2 years ago
This doesn't reproduce anymore and the signature also doesn't show up in FuzzManager, so I assume it's fixed.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Assignee: nobody → jcoppeard
(Assignee)

Comment 8

2 years ago
I can reproduce this so re-opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

2 years ago
Created attachment 8670265 [details] [diff] [review]
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.
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)
(Assignee)

Comment 11

2 years ago
(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)
(Assignee)

Comment 13

2 years ago
Created attachment 8670755 [details] [diff] [review]
bug978802-arguments-object v2

Great, here's a patch.
Attachment #8670265 - Attachment is obsolete: true
Attachment #8670755 - Flags: review?(jdemooij)

Updated

2 years ago
Attachment #8670755 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 14

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

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d03295efd5f
https://hg.mozilla.org/mozilla-central/rev/2d03295efd5f
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.