IonMonkey: Crash [@ js::types::TypeSet::constraintsPurged] or Opt-Crash [@ js::types::TypeSet::isSubset]

RESOLVED FIXED in Firefox 21

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: h4writer)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla22
x86_64
Linux
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 fixed, firefox22 fixed)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
The following testcase crashes on mozilla-central revision d57a813c77a4 (run with --ion-eager):


function g(f) {}
function f(b) {
  g.apply(null, arguments);
  f(false);
}
f(true);
(Reporter)

Comment 1

5 years ago
Crash trace:

==60679== Invalid read of size 4
==60679==    at 0x5E03D2: js::types::TypeSet::constraintsPurged() (jsinfer.h:505)
==60679==    by 0x5E0433: js::types::TypeSet::toStackTypeSet() (jsinfer.h:724)
==60679==    by 0x5E347C: js::types::TypeScript::ArgTypes(JSScript*, unsigned int) (jsinferinlines.h:820)
==60679==    by 0xC3BEE5: js::ion::IonBuilder::makeInliningDecision(js::AutoObjectVector&) (IonBuilder.cpp:3213)
==60679==    by 0xC418D4: js::ion::IonBuilder::jsop_funapplyarguments(unsigned int) (IonBuilder.cpp:4062)
==60679==    by 0xC40A24: js::ion::IonBuilder::jsop_funapply(unsigned int) (IonBuilder.cpp:3962)
==60679==    by 0xC2C346: js::ion::IonBuilder::inspectOpcode(JSOp) (IonBuilder.cpp:938)
==60679==    by 0xC2B04F: js::ion::IonBuilder::traverseBytecode() (IonBuilder.cpp:689)
==60679==    by 0xC2A71D: js::ion::IonBuilder::buildInline(js::ion::IonBuilder*, js::ion::MResumePoint*, js::ion::CallInfo&) (IonBuilder.cpp:488)
==60679==    by 0xC38A39: js::ion::IonBuilder::inlineScriptedCall(JS::Handle<JSFunction*>, js::ion::CallInfo&) (IonBuilder.cpp:2962)
==60679==    by 0xC3D88A: js::ion::IonBuilder::inlineScriptedCalls(js::AutoObjectVector&, js::AutoObjectVector&, js::ion::CallInfo&) (IonBuilder.cpp:3478)
==60679==    by 0xC42334: js::ion::IonBuilder::jsop_call(unsigned int, bool) (IonBuilder.cpp:4118)
==60679==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
Whiteboard: [jsbugmon:update,bisect]
(Reporter)

Updated

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

Comment 2

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

The first bad revision is:
changeset:   122313:64a2c3fb2052
user:        Hannes Verschore
date:        Tue Feb 19 11:33:42 2013 +0100
summary:     Bug 836274: Disable funapply inlining when typeset of callee is tighter than caller, r=nbp

This iteration took 121.091 seconds to run.
(Reporter)

Updated

5 years ago
Crash Signature: [@ js::types::TypeSet::constraintsPurged] → [@ js::types::TypeSet::constraintsPurged] [@ js::types::TypeSet::isSubset]
Summary: IonMonkey: Crash [@ js::types::TypeSet::constraintsPurged] → IonMonkey: Crash [@ js::types::TypeSet::constraintsPurged] or Opt-Crash [@ js::types::TypeSet::isSubset]
(Assignee)

Comment 3

5 years ago
Created attachment 718325 [details] [diff] [review]
Run inference before deciding to inline empty function

Bug is introduced by 837014. We don't have have TI information for the empty function, but still do the inlining. Funapply tests the types and fails on retrieving the type information.

We can fix it by making sure an empty function always has TI information by running inference on it. Not sure if this is allowed, but testing it locally it just works. Fixes this assert + keeps score of deltablue.

(Note: the changes to IonBuilder are a precaution. I noticed I didn't set targetScript correct. This is not exploitable, because when inlining funapply there is only 1 target at max. But still it is plain wrong. Therefore fixing this too!)
Assignee: general → hv1989
Attachment #718325 - Flags: review?(jdemooij)
Comment on attachment 718325 [details] [diff] [review]
Run inference before deciding to inline empty function

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

Good catch.

::: js/src/ion/IonBuilder.cpp
@@ +3163,4 @@
>          if (!target->isInterpreted())
>              return false;
>  
> +        RootedScript targetScript(cx, target->nonLazyScript());

Nit: as Sean mentioned on IRC, these can be JSFunction * and JSScript * since we suppress GC during compilation.

@@ +3199,5 @@
>  
>      JSOp op = JSOp(*pc);
>      for (size_t i = 0; i < targets.length(); i++) {
> +        RootedFunction target(cx, targets[i]->toFunction());
> +        RootedScript targetScript(cx, target->nonLazyScript());

Same here.
Attachment #718325 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/30b241b25c29
Blocks: 837014
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/30b241b25c29
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Assignee)

Comment 7

5 years ago
Created attachment 719028 [details] [diff] [review]
Run inference before deciding to inline empty function

Patch, like it was committed to inbound for asking aurora-approval
Attachment #718325 - Attachment is obsolete: true
Attachment #719028 - Flags: review+
(Assignee)

Comment 8

5 years ago
Comment on attachment 719028 [details] [diff] [review]
Run inference before deciding to inline empty function

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 837014

User impact if declined: crashes when ionmonkey tries to inline empty function

Testing completed (on m-c, etc.): 2 days on m-i, 1 day on m-c

Risk to taking this patch (and alternatives if risky): Almost none.

String or UUID changes made by this patch: /
Attachment #719028 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #719028 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f4f81cc28ed
status-firefox21: --- → fixed
status-firefox22: --- → fixed
You need to log in before you can comment on or make changes to this bug.