Last Comment Bug 746376 - IonMonkey: Assertion failure: !templateObject->hasDynamicSlots(), at ion/IonMacroAssembler.cpp:381 or Crash [@ js::HeapPtr::operator]
: IonMonkey: Assertion failure: !templateObject->hasDynamicSlots(), at ion/IonM...
Status: VERIFIED FIXED
[sg:critical] [jsbugmon:update,reconf...
: assertion, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: Sean Stangl [:sstangl]
:
Mentors:
Depends on:
Blocks: GC langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-04-17 15:57 PDT by Christian Holler (:decoder)
Modified: 2013-02-07 05:15 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
-
unaffected


Attachments
Testcase for shell (1.84 KB, text/javascript)
2012-04-17 15:57 PDT, Christian Holler (:decoder)
no flags Details
Fix. (17.45 KB, patch)
2012-05-09 14:09 PDT, Sean Stangl [:sstangl]
no flags Details | Diff | Splinter Review
Define MarkGCThing. (1.56 KB, patch)
2012-05-10 15:36 PDT, Sean Stangl [:sstangl]
wmccloskey: review+
Details | Diff | Splinter Review
Store and trace compiler roots. (17.35 KB, patch)
2012-05-10 15:37 PDT, Sean Stangl [:sstangl]
no flags Details | Diff | Splinter Review
Store and trace compiler roots. [v2] (20.73 KB, patch)
2012-05-11 15:41 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-04-17 15:57:52 PDT
Created attachment 615925 [details]
Testcase for shell

The attached testcase asserts on ionmonkey revision 67bf9a4a1f77 (run with --ion -n -m --ion-eager).
Comment 1 Christian Holler (:decoder) 2012-04-17 15:58:46 PDT
Stepping through the assertion crashes with evidence of use-after-free:

Program received signal SIGSEGV, Segmentation fault.
0x00000000004137c4 in js::HeapPtr<js::BaseShape, unsigned long>::operator js::BaseShape* (this=0xdadadadadadadada) at ../../gc/Barrier.h:214
214         operator T*() const { return value; }
(gdb) bt 8
#0  0x00000000004137c4 in js::HeapPtr<js::BaseShape, unsigned long>::operator js::BaseShape* (this=0xdadadadadadadada) at ../../gc/Barrier.h:214
#1  0x0000000000405b9c in js::Shape::base (this=0xdadadadadadadada) at ../../jsscope.h:704
#2  0x0000000000405aa6 in js::Shape::getObjectClass (this=0xdadadadadadadada) at ../../jsscope.h:603
#3  0x000000000040708c in js::ObjectImpl::getClass (this=0x7ffff0914a00) at ../../vm/ObjectImpl-inl.h:245
#4  0x00000000004070aa in js::ObjectImpl::hasClass (this=0x7ffff0914a00, c=0xcc14c0) at ../../vm/ObjectImpl-inl.h:257
#5  0x0000000000425503 in js::ObjectImpl::isDenseArray (this=0x7ffff0914a00) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/vm/ObjectImpl-inl.h:52
#6  0x000000000088981b in js::ion::MacroAssembler::getNewObject (this=0x7fffffff9e70, cx=0xd14d40, result=..., templateObject=0x7ffff0914a00, fail=0xd442c0)
    at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/IonMacroAssembler.cpp:416
#7  0x000000000086db46 in js::ion::CodeGenerator::visitCreateThis (this=0x7fffffff9e30, lir=0xd36078) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/CodeGenerator.cpp:1072
(More stack frames follow...)
(gdb) x /i $pc
=> 0x4137c4 <js::HeapPtr<js::BaseShape, unsigned long>::operator js::BaseShape*() const+12>:    mov    (%rax),%rax
(gdb) info reg rax
rax            0xdadadadadadadada       -2676586395008836902
Comment 2 Christian Holler (:decoder) 2012-04-19 15:28:38 PDT
JSBugMon: The testcase found in this bug no longer reproduces (tried revision de015aff650d).
Comment 3 Christian Holler (:decoder) 2012-04-23 08:43:03 PDT
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision bc1833f2111e).
Comment 4 Sean Stangl [:sstangl] 2012-05-07 17:09:21 PDT
The issue in the testcase is that MCall remembers the target JSObject * returned by TI, but by the time the JSObject is used in code generation, the Object has been GC'd. A number of other GCThings are also stored in places in the compiler -- JSFunctions, for example -- without explicit rooting.

To solve this, we'll maintain a global linked list of HeapPtrs to GCThings returned from TI during the current compilation, traced at the start of GC. The linked list itself will be managed using IonMonkey's TempAlloc. Upon return from compilation, the global reference to the linked list will be removed. Apparently HeapPtrs can't get extra dependencies added during IGC, so this should be safe.

Actually using one of these values requires a RootedVar on the stack, in case of moving GC.
Comment 5 Christian Holler (:decoder) 2012-05-08 07:53:39 PDT
It would be very helpful to keep the original subject because it helps us to spot duplicates and also later will allow people to verify bugs more easily (not on IonMonkey but on central later).
Comment 6 Sean Stangl [:sstangl] 2012-05-08 15:34:59 PDT
Can't seem to reproduce this bug anymore as of this afternoon, even with changesets that previously failed.
Comment 7 Sean Stangl [:sstangl] 2012-05-09 14:09:41 PDT
Created attachment 622508 [details] [diff] [review]
Fix.

This should fix the issue by explicitly rooting data remembered during compilation. Leaving without review until the bug manifests again so it can actually be tested.
Comment 8 Christian Holler (:decoder) 2012-05-10 03:26:24 PDT
I'll see if JSBugmon can still repro this.
Comment 9 Christian Holler (:decoder) 2012-05-10 03:34:55 PDT
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision 6e58d603a684).
Comment 10 Sean Stangl [:sstangl] 2012-05-10 15:36:37 PDT
Created attachment 622944 [details] [diff] [review]
Define MarkGCThing.

MarkGCThingRoot() can't be called after the root marking phase is over. This adds a helper to let CompilerRootNode manually mark roots between IGC slices. To land against m-i.
Comment 11 Sean Stangl [:sstangl] 2012-05-10 15:37:26 PDT
Created attachment 622945 [details] [diff] [review]
Store and trace compiler roots.
Comment 12 Bill McCloskey (:billm) 2012-05-10 15:52:56 PDT
Comment on attachment 622944 [details] [diff] [review]
Define MarkGCThing.

This seems fine. I don't think you need it for the patch below (see comments), but feel free to check it in if you need it for something else.
Comment 13 Bill McCloskey (:billm) 2012-05-10 15:53:43 PDT
Oops, that last r+ was meant for attachment 622944 [details] [diff] [review].
Comment 14 Bill McCloskey (:billm) 2012-05-10 16:15:10 PDT
Comment on attachment 622945 [details] [diff] [review]
Store and trace compiler roots.

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

::: js/src/gc/Root.h
@@ +145,5 @@
>      template <typename S> inline Handle(const RootedVar<S> &root);
> +#ifdef JS_ION
> +    // Defined in ion/IonGC.h.
> +    template <typename S> inline Handle(const js::ion::CompilerRoot<S> &root);
> +#endif

As discussed, please take this out.

::: js/src/ion/InlineList.h
@@ +171,5 @@
> +    void clear() {
> +        this->next = NULL;
> +        tail_ = this;
> +#ifdef DEBUG
> +        modifyCount_ = 0;

Can you make this field DebugOnly<T> and take out the #ifdef?

::: js/src/ion/Ion.cpp
@@ +76,2 @@
>  IonOptions ion::js_IonOptions;
> +InlineForwardList<CompilerRootNode> ion::compilerRootList;

Because of web workers, we can't really use globals for stuff like this. You might want to store the list on the runtime. I'm not sure what makes js_IonOptions okay either. Is it immutable?

@@ +777,5 @@
>      IonBuilder builder(cx, fp->scopeChain(), temp, graph, &oracle, *info);
> +    bool ok = TestCompiler(builder, graph);
> +
> +    // Destroy list of GCThings to trace: they are either unused or rooted elsewhere.
> +    compilerRootList.clear();

It would be nice to use RAII here. It looks like IonCompile has a number of convenient objects that you might be able to piggyback on. Maybe the IonContext or the IonBuilder?

::: js/src/ion/IonGC.h
@@ +83,5 @@
> +    { }
> +
> +  public:
> +    static CompilerRoot<T> *New(JSContext *cx, T ptr) {
> +        CompilerRoot<T> *root = new CompilerRoot<T>(ptr);

As far as I know, we can't use new in SpiderMonkey. Is there any kind of compiler temp pool that you could allocate out of? Also, you probably need to handle the possibility that this allocation will fail.

@@ +90,5 @@
> +        // Explicitly mark this root if this executes between incremental GC slices:
> +        // the list may have already been traced, and this root would be otherwise missed.
> +        JSCompartment *comp = cx->compartment;
> +        if (comp->needsBarrier())
> +            gc::MarkGCThing(comp->barrierTracer(), root->address(), "ion-compiler-root");

You don't need this code. If the object was allocated during an incremental GC slice, it is automatically marked when the GC returns it. If it was allocated before the incremental GC started, then we must have obtained it somehow, and whoever was holding it when the GC started is responsible for marking it.

Also, this function doesn't need to take a |cx|. So you don't need to pass it down through the MIR. However, you will need to get to the runtime to find the compilerRootList. You can get that via ptr->compartment()->rt.

::: js/src/ion/MIR.h
@@ +1172,4 @@
>      JSFunction *getSingleTarget() const {
> +        if (!target_)
> +            return NULL;
> +        return target_->raw();

Do you need ->raw() (or below)? It seems like the coercion operator should take care of this.
Comment 15 Sean Stangl [:sstangl] 2012-05-11 15:30:19 PDT
Thank you for the quick review!

(In reply to Bill McCloskey (:billm) from comment #14)
> Because of web workers, we can't really use globals for stuff like this. You
> might want to store the list on the runtime. I'm not sure what makes
> js_IonOptions okay either. Is it immutable?

It's immutable after startup. I'd expect us to keep it immutable, so it should be safe.
 
> As far as I know, we can't use new in SpiderMonkey. Is there any kind of
> compiler temp pool that you could allocate out of? Also, you probably need
> to handle the possibility that this allocation will fail.

IonMonkey overloads new to use the LIFO bump allocator with (asked dvander) a ballast to ensure that a non-negligible amount of space is available; small allocations are therefore "infallible". The CompilerRoot inherits from this allocator, so this new is the overridden one, with the same assumption as the rest of IonMonkey.
Comment 16 Sean Stangl [:sstangl] 2012-05-11 15:41:23 PDT
Created attachment 623334 [details] [diff] [review]
Store and trace compiler roots. [v2]
Comment 17 David Anderson [:dvander] 2012-05-11 17:51:01 PDT
Comment on attachment 623334 [details] [diff] [review]
Store and trace compiler roots. [v2]

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

Looks fine but the two new files probably need better names - if it's okay to put them all in one file I'd just call it "CompilerRooting.h"
Comment 18 Sean Stangl [:sstangl] 2012-05-14 14:25:42 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/269dba32ca50
Comment 19 Christian Holler (:decoder) 2012-05-18 04:36:49 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 20 Christian Holler (:decoder) 2013-02-07 05:15:33 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397

Note You need to log in before you can comment on or make changes to this bug.