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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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);

Comment 1

[Christian Holler (:decoder)]

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

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:   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.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: 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!)

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30b241b25c29

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30b241b25c29

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Run inference before deciding to inline empty function

Patch, like it was committed to inbound for asking aurora-approval

Comment 8

[Hannes Verschore [:h4writer]]

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: /

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f4f81cc28ed