GenerationalGC: Crash [@ js::assertSameCompartment]

RESOLVED FIXED in mozilla29

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
mozilla29
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

Reporter

Description

6 years ago
Posted file lldb stack
The upcoming testcase crashes js debug shell (tested with a threadsafe deterministic 64-bit debug build) on m-c changeset 2581b84e0ca1 with --ion-eager at js::assertSameCompartment

My configure options are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-exact-rooting --enable-elf-hack --enable-stdcxx-compat --enable-warnings-as-errors --enable-signmar --disable-elf-hack --enable-js-diagnostics --with-intl-api=build --enable-ctypes --disable-shared-js --enable-jemalloc --enable-gcgenerational --with-ccache --enable-threadsafe
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Assignee

Comment 2

6 years ago
I've successfully reproduced this on MacOS and on linux, but haven't worked out what's wrong yet.

It's getting property '0' from an arguments object and it turns out to be a pointer into swept nursery.

It looks like arguments objects are traced and barriered properly...
Flags: needinfo?(jcoppeard)
Assignee

Comment 3

6 years ago
This has been a pain to track down, but I think BaselineCompiler::emitFormalArgAccess() needs a postbarrier when writing to an arguments object.
Assignee

Comment 4

6 years ago
It turns our there are a couple of places in Ion and Baseline where we need to postbarrier writes into arguments objects.  These are treated differently to writes to normal JS objects.

We also need to make the whole cell store buffer capable of marking arguments objects, which have a custom trace function to deal with their layout.

Terrence, I considered using the generic buffer for these and adding a separate path to add them from ion, but this is simpler and I don't think the performance overhead will be significant - does that sound resonable?
Assignee: general → jcoppeard
Status: NEW → ASSIGNED
Attachment #8345257 - Flags: review?(terrence)
Comment on attachment 8345257 [details] [diff] [review]
fix-args-fuzzbug

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

Good job tracking this down! Missing barriers in jit-code are unbelievably hard to find.

I think the approach looks good, but I'm going to punt on the jit bits.
Attachment #8345257 - Flags: review?(terrence)
Attachment #8345257 - Flags: review?(kvijayan)
Attachment #8345257 - Flags: review+
Flags: needinfo?(terrence)
Comment on attachment 8345257 [details] [diff] [review]
fix-args-fuzzbug

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

r=me with explanations for questions asked.

::: js/src/jit/BaselineCompiler.cpp
@@ +2349,5 @@
> +        Register reg = R2.scratchReg();
> +        masm.loadPtr(Address(BaselineFrameReg, BaselineFrame::reverseOffsetOfArgsObj()), reg);
> +
> +        // Fully sync the stack if post-barrier is needed.
> +        frame.syncStack(0);

It seems clearer to move the frame.syncStack(0) above the loadPtr for the args object.

::: js/src/jit/IonBuilder.cpp
@@ +9121,5 @@
>      if (info().argsObjAliasesFormals()) {
> +        if (NeedsPostBarrier(info(), val))
> +            current->add(MPostWriteBarrier::New(alloc(), current->argumentsObject(), val));
> +        current->add(MSetArgumentsObjectArg::New(alloc(), current->argumentsObject(),
> +                                                 GET_SLOTNO(pc), val));

It feels weird to me that the MPostWriteBarrier is happening before the actual write.  Can this be moved after the MSetArgumentsObjectArg, or does it need to happen before?
Attachment #8345257 - Flags: review?(kvijayan) → review+
Assignee

Comment 7

6 years ago
(In reply to Kannan Vijayan [:djvj] from comment #6)
> It seems clearer to move the frame.syncStack(0) above the loadPtr for the
> args object.

Agreed, I'll change that.

> It feels weird to me that the MPostWriteBarrier is happening before the
> actual write.  Can this be moved after the MSetArgumentsObjectArg, or does
> it need to happen before?

I don't think it matters either way, but all the other ones in Ion happen before the write so I did it like that for consistency.
https://hg.mozilla.org/mozilla-central/rev/7201bf07232e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.