945453 - GenerationalGC: Crash [@ js::assertSameCompartment]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: 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

Comment 2

[Jon Coppeard (:jonco)]

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...

Comment 3

[Jon Coppeard (:jonco)]

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

Comment 4

[Jon Coppeard (:jonco)]

Attachment: fix-args-fuzzbug

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?

Comment 5

[Terrence Cole [: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.

Comment 6

[Kannan Vijayan [:djvj]]

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?

Comment 7

[Jon Coppeard (:jonco)]

(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.

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7201bf07232e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7201bf07232e