Closed Bug 925777 Opened 6 years ago Closed 6 years ago

Crash [@ js::types::UseNewTypeForClone]

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- disabled
firefox26 --- disabled
firefox27 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- disabled
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [fuzzblocker][qa-])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file 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>
Flags: needinfo?(shu)
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.
Whiteboard: [fuzzblocker]
Ran the test case 10,000 iterations without crashes, so I'm assuming this is the right fix.
Attachment #816040 - Flags: review?(bhackett1024)
Assignee: general → shu
Status: NEW → ASSIGNED
Dammit how do I make bzexport clear needinfo
Flags: needinfo?(shu)
Pre-emptive review comment about the (JSFunction *) cast: C++ barfs on the deleted copy constructor in CompilerRoot otherwise.
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.
Attachment #816040 - Flags: review?(bhackett1024)
(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.
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.
Attachment #816101 - Flags: review?(bhackett1024)
Attachment #816040 - Attachment is obsolete: true
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!
Attachment #816101 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/0ce5fda9fbe2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Keywords: verifyme
Keywords: verifyme
Whiteboard: [fuzzblocker] → [fuzzblocker][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.