782337 - Opt-crash [@ js::gc::PushMarkStack]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test crashes on mozilla-central revision b5ae446888f5 (options -m -n), opt-build only:


function g(a1, a2) {
  var d = (new g("ABCDEFGHIJK", "1234"));
}
try {
  g();
} catch(exc1) {}
gczeal(9, 2)
try {
  for (a = 0; a < 4; - a) {
    evaluate();
  }
} catch(exc1) {}
var ans = '';
for (var i = (1); i < 5; ++i) {
  ans += g(i) + (null );
}


I'm seeing valgrind errors like:

==15795== Invalid write of size 4
==15795==    at 0x81F514E: js::gc::PushMarkStack(js::GCMarker*, js::types::TypeObject*) (Heap.h:693)
==15795==    by 0x81F68C9: js::GCMarker::processMarkStackTop(js::SliceBudget&) (Marking.cpp:1187)
==15795==    by 0x81F6B3B: js::GCMarker::drainMarkStack(js::SliceBudget&) (Marking.cpp:1249)
==15795==    by 0x80AD514: IncrementalCollectSlice(JSRuntime*, long long, js::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:3862)
==15795==    by 0x80AF0A9: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4119)
==15795==    by 0x80B02DD: Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4227)
==15795==    by 0x80B052D: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:4512)
==15795==    by 0x80729FC: JSObject* js::gc::NewGCThing<JSObject>(JSContext*, js::gc::AllocKind, unsigned int) (jsgcinlines.h:446)
==15795==    by 0x80EC705: JSObject::create(JSContext*, js::gc::AllocKind, JS::Handle<js::Shape*>, JS::Handle<js::types::TypeObject*>, js::HeapSlot*) (jsgcinlines.h:501)
==15795==    by 0x80EC8D7: NewObject(JSContext*, js::Class*, js::types::TypeObject*, JSObject*, js::gc::AllocKind) (jsobj.cpp:2335)
==15795==    by 0x80EDD69: js::NewObjectWithGivenProto(JSContext*, js::Class*, JSObject*, JSObject*, js::gc::AllocKind) (jsobj.cpp:2385)
==15795==    by 0x809D09E: js_ErrorToException(JSContext*, char const*, JSErrorReport*, JSErrorFormatString const* (*)(void*, char const*, unsigned int), void*) (jsobjinlines.h:1441)
==15795==  Address 0x6afd038 is not stack'd, malloc'd or (recently) free'd


S-s until triaged.

Comment 1

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

I'll have a patch for this soon, but I thought I'd give the diagnosis first because it's relevant to bug 778724.

The bug here is that incremental GC doesn't interact correctly with VM stack scanning. In the test case, we scan the stack in the first GC slice. That overwrites dead values with valid ones. Then we return to JIT code, which is able to write out more dead values. In the last slice, we throw away analysis info. Then, in a later GC, we scan the stack again and assume that all stack values are valid because there's no analysis info. Hence we crash.

The fix I'm planning is to iterate over the stack in the last slice, before throwing away analysis info, and to clobber any remaining dead values. The extra iteration kinda sucks, but it will be easy to backport to Firefox 16.

Brian, I'm wondering how this interacts with bug 778724.

Comment 2

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

Attachment: patch to optionally disable marking validation

It turns out the reason this only reproduces in an opt build is that, after incremental marking is over, we do another complete marking pass to make sure nothing was missed. This pass calls MarkRuntime a second time, which re-clobbers the dead values--this time at the right time.

This patch adds a shell function to disable the validations, which makes it possible to write a test case for this bug that reproduces in debug builds.

Comment 3

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

Attachment: patch

This patch does a separate clobbering pass right before discarding analysis info.

Comment 4

[Brian Hackett [Laid off!]]

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

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

I think that with bug 778724 we will also need to markAndClobber whenever destroying a JITScript outside of GC.  During analysis purges we attach new info to JITScripts describing liveness info for the script, which will persist until the JITScript is destroyed.  After that destruction there is no trace of where the dead values in the frame are.  I think the best way to do this is just making destruction of JITScripts impossible outside of GC; there's only a few places this is possible, since normally we only discard individual JITChunks.

Comment 5

[u279076]

Does this need tracking for Firefox 17 or earlier?

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c8e8efc80a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1fae667a88

Comment 7

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

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 641025
User impact if declined: Crashes, security exploits.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c8e8efc80a8
https://hg.mozilla.org/mozilla-central/rev/ac1fae667a88

Comment 9

[Alex Keybl [:akeybl]]

Bug 641025 was landed in FF13, so not present in the ESR10.

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/646b88b844d2

Comment 11

[[e-mail]]

Ms2ger backed this out due to bustage:

http://hg.mozilla.org/releases/mozilla-aurora/rev/b471130dc112

Comment 12

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

Relanded:
https://hg.mozilla.org/releases/mozilla-aurora/rev/179a8a17a542

Comment 13

[Al Billings [:abillings - ex-MoCo]]

I assume 15 is affected here?

Comment 14

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

15 is unaffected because this bug relies on incremental GC.

Comment 15

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug782337.js.

Comment 16

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old