925777 - Crash [@ js::types::UseNewTypeForClone]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

x = new ParallelArray([10848], function() {});
x.filter(function(e, i, s) {
    if (i % 842 == 44) {
        return ((function() {})())
    }
})

crashes js opt shell on m-c changeset 64b497e6f593 with --baseline-eager --ion-parallel-compile=on intermittently at js::types::UseNewTypeForClone

Setting s-s just-in-case and setting needinfo from Shu-yu as MLambdaPar (a parallel JS function?) is on the stack, and the testcase involves ParallelArrays.

My configure flags are:

sh ./configure --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Comment 1

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

Setting fuzzblocker, as this is the first time I'm seeing a fairly reliable testcase (just run it 20+ times to crash), since first seeing it a few(?) weeks ago.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Fix race in constructing LambdaFunctionInfo OMT during PJS compilation. (r=?)

Ran the test case 10,000 iterations without crashes, so I'm assuming this is the right fix.

Comment 3

[Shu-yu Guo [:shu]]

Dammit how do I make bzexport clear needinfo

Comment 4

[Shu-yu Guo [:shu]]

Pre-emptive review comment about the (JSFunction *) cast: C++ barfs on the deleted copy constructor in CompilerRoot otherwise.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 816040 [details] [diff] [review]
Fix race in constructing LambdaFunctionInfo OMT during PJS compilation. (r=?)

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

::: js/src/jit/MIR.h
@@ +3980,1 @@
>      static MConcatPar *New(MDefinition *slice, MConcat *concat) {

These functions should have assertions instead of comments.  This comment also doesn't make much sense, is the other one OK and if not why not?  Also, OMTC is not a standard acronym in the JIT and shouldn't appear in comments.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #5)

> These functions should have assertions instead of comments.  This comment
> also doesn't make much sense, is the other one OK and if not why not?  Also,
> OMTC is not a standard acronym in the JIT and shouldn't appear in comments.

Well, both are "okay", but the blame rests in different contexts. If we are creating a Par variant from a sequential one, we shouldn't be allocating new things / doing thread-unsafe things. The blame is on that particular |New| function to ensure that it is threadsafe. In the other |New| function, the blame is on its callers, since all it does is pass along some pointers.

What can I assert here? It's not right to assert that we are always off main thread, and it's not right to assert that we are. If anything, I feel like the assertion should be in the LambdaFunctionInfo constructor.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Remove possibly thread unsafe constructors from Par variants of MIR. (r=?)

Here's one better than assertions. Due to OMTC we shouldn't be constructing Par
variants of MIR ex nihilo anyways, so I've removed the possibly unsafe
variants completely.

Comment 8

[Brian Hackett [Laid off!]]

Comment on attachment 816101 [details] [diff] [review]
Remove possibly thread unsafe constructors from Par variants of MIR. (r=?)

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

Thanks!

Comment 9

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce5fda9fbe2

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/0ce5fda9fbe2