Closed Bug 684623 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: decoder, Assigned: bhackett)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase, Whiteboard: fixed-in-jaegermonkey)

Attachments

(1 file)

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
Duplicate of this bug: 684585
Attached patch patchSplinter Review
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
Assignee: general → bhackett1024
Attachment #558361 - Flags: review?(wmccloskey)
Whiteboard: js-triage-needed → fixed-in-jaegermonkey
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.
Attachment #558361 - Flags: review?(wmccloskey) → review+
(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.
http://hg.mozilla.org/mozilla-central/rev/584386505971
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.