892287 - GenerationalGC: do not store pointers to reallocated memory in the store buffer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

ArrayBufferObject's elements header is a normal reallocable piece of memory. In GetViewList, we happily return a HeapPtr pointing at it. This writes pointers into the store buffer that later get moved, causing us to crash. This happens in normal jsapi-tests and jit-tests with the patch from bug 889682 applied. They appear to have simply been masked by lucky GC timing, since this is clearly just wildly wrong.

I have a working patch, but it is based on 889682 and need to get pulled under it.

Comment 1

[Terrence Cole [:terrence]]

Attachment: Part 1 of 2 - Fix barriers on typedarray views; v0

Currently, we barrier every individual write in the views list. I think it is more efficient (in runtime /and/ code) to just insert a whole-object barrier on the owning buffer any time we touch the views list. The buffer's obj_trace already strongly marks the views list during minor GC, so this should insulate us from a moving elements.

The one weird issue here is that obj_trace munges the views list, which causes us to dump another item in the store buffer, which causes us to call obj_trace again, ad nauseum. Thus we have an enum on GetViewsList to prevent the store buffer insertion when we use this function from the GC routines. I also added ReentrancyGuards to the buffers to keep this from regressing, but that is in the second patch in this series.

Comment 2

[Terrence Cole [:terrence]]

Attachment: Part 2 of 2 - Allow nursery things to be stored in the whole object buffer.

We need to store nursery things in the whole object buffer now since ArrayBufferObject can be in the nursery. It is very convenient and there is no reason not to allow it, so this patch renames |tenured| to |cell_| and implements the required marking.

This patch also adds ReentracyGuard to the buffers, for the reasons explained in Part 1 of 2.

Comment 3

[Terrence Cole [:terrence]]

Attachment: Part 2 of 2 - v1 - Fix merge bustage.

The initializer for entered went missing when I re-stacked these patches.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 774091 [details] [diff] [review]
Part 1 of 2 - Fix barriers on typedarray views; v0

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

Hm. I've always been a little ambivalent about GetViewList returning a pointer (to a pointer), since it's sometimes used read-only and so the extra pointer layer is superfluous. With this new param, it feels even more clunky. Looking at the code, it seems like splitting out a GetViewList (returning ArrayBufferViewObject*) from SetViewList should work out ok now. (They'd both probably call an underlying GetViewListPtr or something because the accessing code is messy.) At least, in my scanning of the code in this patch, it seems like the places where you want to set are nicely separate.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 774116 [details] [diff] [review]
Part 2 of 2 - v1 - Fix merge bustage.

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

Could you explain this more? I don't understand why you would want to put things in the nursery in the store buffer. Also, I don't get the ReentrancyGuard thing either. How can adding something to the store buffer cause us to trace an object?

Comment 6

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #4)
> Hm. I've always been a little ambivalent about GetViewList returning a
> pointer (to a pointer), since it's sometimes used read-only and so the extra
> pointer layer is superfluous. With this new param, it feels even more
> clunky. Looking at the code, it seems like splitting out a GetViewList
> (returning ArrayBufferViewObject*) from SetViewList should work out ok now.
> (They'd both probably call an underlying GetViewListPtr or something because
> the accessing code is messy.) At least, in my scanning of the code in this
> patch, it seems like the places where you want to set are nicely separate.

Great idea! I'll whip that up.

(In reply to Bill McCloskey (:billm) from comment #5)
> Could you explain this more? I don't understand why you would want to put
> things in the nursery in the store buffer.

So we want to update the view list of the thing we are storing, which may or may not be in the nursery. Now that I reflect on it a bit more, it seems that if the array buffer is in the nursery, it will either (1) be dead and thus we don't need to update the view list, or (2) it will be traced by something else, which will have the same effect as this. So I think we could just test and not insert nursery things here and be fine. Does that make sense?

> Also, I don't get the
> ReentrancyGuard thing either. How can adding something to the store buffer
> cause us to trace an object?

That's backwards: it's of the store buffer tracing that is putting more things in the store buffer. What happens here is as follows:
  - The stuff we've marked already isn't in the remembered set anymore.
  - As marking puts more stuff in the buffer, eventually we trigger a |compact|.
  - Compact removes a bunch of "not remembered set" items at the front.
  - This puts |end| in front of the local |pos| in the mark loop.
  - Thus, the mark loop never exits and we try to interpret random garbage as store buffer entries.

I think the right thing to do here is just add the re-entrancy guard. It's a simple enough static property to ensure and messing up results in nasty-to-debug bugs like the above.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> Now that I reflect on it a bit more, it seems that if the array buffer is in the nursery, it
> will either (1) be dead and thus we don't need to update the view list, or (2) it will be
> traced by something else, which will have the same effect as this. So I think we could just
> test and not insert nursery things here and be fine. Does that make sense?

Yes, this is what I meant.

> I think the right thing to do here is just add the re-entrancy guard. It's a simple enough
> static property to ensure and messing up results in nasty-to-debug bugs like the above.

OK, that makes sense.

Comment 8

[Terrence Cole [:terrence]]

Attachment: Part 1 of 2 - Apply review commentary; v1

This is quite a bit cleaner. I ended up needing GetViewList, SetViewList, InitViewList, and GetViewListRef, but the last asserts that it is only used during a GC and only the two setters now need to put a store buffer entry. This should be more performant on read-only workloads. It also removes /all/ of the generic buffer entries, for views as well as buffers, and uses a common method to push new store buffer entries.

Comment 9

[Terrence Cole [:terrence]]

Attachment: Part 2 of 2 - Now just reentrancy guards; v2

As we discussed. Passes jit-tests --tbpl standalone.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 774963 [details] [diff] [review]
Part 1 of 2 - Apply review commentary; v1

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

Yeah, this seems to nicely split up the different cases, thanks.

::: js/src/vm/TypedArrayObject.cpp
@@ +280,5 @@
> +{
> +#ifdef JSGC_GENERATIONAL
> +    JS_ASSERT(obj);
> +    JSRuntime *rt = obj->runtime();
> +    if (!rt->isHeapBusy() && !IsInsideNursery(rt, obj))

Just a random side comment, but this !rt->isHeapBusy() check reads funny to me. I have to mentally translate it to !rt->isInTheMiddleOfAGCOrSomethingLikeIt(). Not for this patch, but is there any way to express the actual property that matters instead of negating something? Like, could it be rt->mutatorActive() or rt->trackingMutations()? Hm, the latter sounds suspiciously similar to needsBarrier(), which I assume is a little different.

Comment 11

[Terrence Cole [:terrence]]

Yeah, I agree. I've always hated isHeapBusy. I think isHeapCollecting would be much better now that we have isHeapMinorCollecting and isHeapMajorCollecting. Also, !isHeapCollecting actually sounds pretty reasonable.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

isHeapBusy can be true even when there's no GC going on. For example, it's true while someone is calling JS_DumpHeap. That's why the name is sort of ambiguous.

Comment 13

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/881b8c2b1058
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc9ec6aff994

Comment 14

[Steve Fink [:sfink] [:s:]]

(In reply to Bill McCloskey (:billm) from comment #12)
> isHeapBusy can be true even when there's no GC going on. For example, it's
> true while someone is calling JS_DumpHeap. That's why the name is sort of
> ambiguous.

Yes, that was my understanding, hence the !rt->isInTheMiddleOfAGCOrSomethingLikeIt(). !rt->isTracingHeap() seems slightly preferable, if it is indeed accurate, but it still seems like something similar to rt->isTrackingMutations() is more directly relevant and meaningful for local code inspection. Assuming it's the relevant characteristic. I guess I'd need to scan through the uses of rt->isHeapBusy() to see what each one really wants to know.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/881b8c2b1058
https://hg.mozilla.org/mozilla-central/rev/bc9ec6aff994