1306382 - Automatically ExposeToActiveJS when reading out of a TenuredHeap<T>

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Following on from bug 1297558, I guess we also need to add a barrier on TenuredHeap<T> too.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1306382-auto-expose-tenured-heap

Patch to add an auto-exposing read barrier to TenuredHeap<T> just as we did to Heap<T>.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

Requesting additional review for the non-SM parts.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

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

r=me for the non JS engine parts.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

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

r=me for the non JS engine parts.

::: js/xpconnect/src/XPCInlines.h
@@ +470,5 @@
>  inline
>  void XPCWrappedNativeTearOff::JSObjectMoved(JSObject* obj, const JSObject* old)
>  {
>      MOZ_ASSERT(!IsMarked());
> +    MOZ_ASSERT(mJSObject.unbarrieredGetPtr() == old);

Some kind of operator== for JS::TenuredHeap<JSObject*> would avoid the need for this here and other places. I don't think we ever want to unmarkgray for that.

Comment 5

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

Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

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

I think I agree with mccr8 that making an unbarriered operator== would be a good idea. Not just for the convenience -- in fact, some of the callsites might be more clear with the current explicit unbarrieredGetPtr() == x, I'm not sure.

But it seems like the current operator== is a bit of a footgun; it feels unexpected that it would affect mark bits. With the operator==, tenuredHeap == x is different from x == tenuredHeap, I guess, but that doesn't seem too awful.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #5)
I agree, but for reasons that are still obscure to me I've been unable to provide an operator== that C++ will select in preference to converting a TenuredHeap<T> into the base type and triggering the read barrier.  I'm going to land this as is and follow up in a separate bug.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6217f779742e
Automatically ExposeToActiveJS when reading out of a TenuredHeap<T> r=sfink r=mccr8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6217f779742e