1033115 - Crash [@ js::jit::AssertValidObjectPtr] or Assertion failure: obj->compartment() == cx->compartment(), at jit/VMFunctions.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Crash and assert stacks

x = []
x[8] = ((function() {})());
for each(let a in [0, 0]) {
    x.reducePar(function() {
        return [0];
    });
}

asserts js debug shell on m-c changeset b6408c32a170 with --ion-eager --ion-offthread-compile=off at Assertion failure: obj->compartment() == cx->compartment(), at jit/VMFunctions.cpp but also sometimes crashes at js::jit::AssertValidObjectPtr

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140609215111" and the hash "a98e6f80344c".
The "bad" changeset has the timestamp "20140609220510" and the hash "573458d10426".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a98e6f80344c&tochange=573458d10426

(initially setting s-s and sec-high as a compartment assert doesn't sound good, but please feel free to change this as necessary.)

Lars, is bug 933313 a likely regressor?

Comment 1

[Lars T Hansen [:lth]]

Reproduction:
 - Reproduces on first try on Linux64 debug build (--enable-debug --disable-optimize).
 - Also reproduces (slightly different crash but clearly same cause) with --thread-count=1.

Immediate observations:
 - The problem is that obj references some other objects that contain fields that are filled with
   the JS_POISONED_FORKJOIN_CHUNK poison (0xbdbdbd...bd), suggesting some uninitialized fields.
 - The shape, and the shape's base shape, all contain a number of poisoned fields.
 - obj->compartment() == obj->lastProperty()->base()->compartment(), and that value is poisoned.
 - Actually obj itself has poisioned fields - it looks uninitialized.

Conditions for crashing:
 - The for-each statement must create a binding: if it is "for each(let a in ...)" 
   or "for each(var a in ...)" or "for each(a in ...)" without an existing global
   binding for a then we crash.  If there is an existing binding for a then we do not crash.
 - The array we're iterating over must be sparse and relatively large; 9 elements seems to
   be the smallest
 - The type in the array must be some unknown type
 - The kernel must return an object value

Room for variation:
 - The object being iterated over can be a plain object, it need not be an array
 - The object returned from the kernel could be any object value, eg {}

=> With one thread, at the time of the crash we are out of the parallel section and we've
   evacuated no objects.

=> Yet clearly, since the kernel returns an object value, there should have been at least one
   object evacuated.

The bug is almost certainly that when running the evacuation from PJS, the return value from the parallel section is not considered a root.  And indeed, that is a bug in Array.js in ArrayReducePar: the array "subreductions" should be passed to ForkJoin as an assignment target, but it is not.

I'll keep this as a security issue for now, since in a release build there would likely be a wild pointer into the discarded PJS nursery from the subreductions array.  It's possible that some sort of heap spray attack could be used, though not likely, given that many object types cannot be allocated in the nursery.

Comment 2

[Lars T Hansen [:lth]]

Attachment: Patch, with test case

Pass the intermediate array as a root to ForkJoin().

Comment 3

[Lars T Hansen [:lth]]

Gary, in response to your question, this bug was indeed introduced by bug 933313, which created the requirement that the result array has to be passed as an argument to the ForkJoin() primitive.

Comment 5

[Lars T Hansen [:lth]]

Attachment: Patch, with two test cases

Added another test case from bug 1033080.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8449261 [details] [diff] [review]
Patch, with two test cases

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

Good catch.

Comment 7

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/297806051a83

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/297806051a83

Comment 9

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 10

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

I assume that this is disabled in ESR31?