684623 - Crash [@ js::types::TypeSet::unknown]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test crashes on mozilla-central revision a351ae35f2c4 (with shell build fix from mozilla-inbound rev fff3dc9478ce). Use options -m -n -a:


gczeal(2);
var N = 100*1000;
function build(N) {
  for (var i = 0; i != N; ++i) {
    var f = Function('some_arg'+i, ' return /test/;');
    var re = f();
  }
}
var chainTop = build(N);


Backtrace:

==28772== Invalid read of size 4
==28772==    at 0x4582FC: js::types::TypeSet::unknown() const (jsinfer.h:381)
==28772==    by 0x45A775: js::types::TypeSet::addType(JSContext*, js::types::Type) (jsinferinlines.h:937)
==28772==    by 0x4DED56: TypeConstraintPropagateThis::newType(JSContext*, js::types::TypeSet*, js::types::Type) (jsinfer.cpp:1253)
==28772==    by 0x45A591: js::types::TypeCompartment::resolvePending(JSContext*) (jsinferinlines.h:722)
==28772==    by 0x4EDFB2: js::types::TypeSet::add(JSContext*, js::types::TypeConstraint*, bool) (jsinfer.cpp:417)
==28772==    by 0x4DD8D2: js::types::TypeSet::addPropagateThis(JSContext*, JSScript*, unsigned char*, js::types::Type) (jsinfer.cpp:711)
==28772==    by 0x4E4802: js::analyze::ScriptAnalysis::analyzeTypesBytecode(JSContext*, unsigned int, js::analyze::ScriptAnalysis::TypeInferenceState&) (jsinfer.cpp:3420)
==28772==    by 0x4E637D: js::analyze::ScriptAnalysis::analyzeTypes(JSContext*) (jsinfer.cpp:3927)
==28772==    by 0x4EDAD4: JSScript::ensureRanInference(JSContext*) (jsinferinlines.h:1247)
==28772==    by 0x6E0967: js::mjit::Compiler::checkAnalysis(JSScript*) (Compiler.cpp:193)
==28772==    by 0x6E16F0: js::mjit::Compiler::performCompilation(js::mjit::JITScript**) (Compiler.cpp:513)
==28772==    by 0x6E0835: js::mjit::Compiler::compile() (Compiler.cpp:162)
==28772==  Address 0xdadadadadaef5f72 is not stack'd, malloc'd or (recently) free'd

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

GC hazard regression from bug 679887.  This bug imposed the requirement that type objects must be marked either through MarkTypeObject or through a JSObject which has them as a type (type objects have back pointers to their singletons and script for interpreted functions).  However, MarkKind(), which the conservative scanner uses, did not go through this and when marking type objects could end up not marking the type's associated singleton or script.

http://hg.mozilla.org/projects/jaegermonkey/rev/584386505971

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 558361 [details] [diff] [review]
patch

This looks basically fine. There is one annoying problem. The caller of MarkKind is expected to call JS_SET_TRACING_NAME beforehand. If you call MarkTypeObject from MarkKind, the name that the caller set then gets overwritten with "type_stack". I guess you could add a version of MarkTypeObject that doesn't take a name. I think this is what we do for strings. I apologize for the ugliness here.

I'm a bit worried that this sort of bug is going to keep popping up. Do you remember how much this optimization was worth out of everything in bug 679887? It kind of breaks one's expectations about what marking is supposed to do.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #3)
> I'm a bit worried that this sort of bug is going to keep popping up. Do you
> remember how much this optimization was worth out of everything in bug
> 679887? It kind of breaks one's expectations about what marking is supposed
> to do.

I mostly added this to avoid calling js_TraceScript on type objects for interpreted functions, as before bug 674251 there wasn't a way to test if this had already been done.  Now that bug 674251 is in this is less important.

I think this is more of a boneheaded error than an interface problem likely to repeat, but it depends on what you think the interface for marking actually is.  My impression is that Mark() itself is an internal helper function for jsgcmark.cpp (the patch removes it from jsgcmark.h, and it isn't called anywhere else) so that this bug is a jsgcmark-internal one, and that clients should go through MarkTypeObject/MarkObject/etc., MarkKind or JS_TraceChildren.

Comment 6

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/mozilla-central/rev/584386505971