854029 - Fix RelocatableValue

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

It turns out it has been utterly broken since it landed. It didn't trigger failures with GGC because in our test suite it was either a GCThing (on which it would work), or was dead, or was masked by key marking that also marked the value incidentally. RelocatablePtr is much better tested: the new code here is basically a valuized copy of that code.

Comment 1

[Tom S [:evilpie]]

Comment on attachment 728481 [details] [diff] [review]
v0

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

::: js/src/gc/Barrier-inl.h
@@ +279,5 @@
>      return *this;
>  }
>  
> +/* static */ inline bool
> +RelocatableValue::isNonNullValue(const Value &value)

Don't really like the name. There is probably some precedence to it, but I find it confusing considering the JS |null|.
Would "isMarkableThing" make sense?

Comment 2

[Terrence Cole [:terrence]]

Agreed, I don't really like the name I came up with either. |isMarkableThing| makes sense to me, but I'll defer to whatever name Bill thinks is best.

Comment 3

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

Comment on attachment 728481 [details] [diff] [review]
v0

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

::: js/src/gc/Barrier-inl.h
@@ +222,5 @@
>  RelocatableValue::RelocatableValue(const Value &v)
>      : EncapsulatedValue(v)
>  {
>      JS_ASSERT(!IsPoisonedValue(v));
> +    if (isNonNullValue(v))

v.isMarkable() implies that v is not a NULL GC thing. So just use that and we don't even need an auxiliary function.

Comment 4

[Terrence Cole [:terrence]]

Thanks, I had forgotten that isMarkable already handles NULL.

https://hg.mozilla.org/integration/mozilla-inbound/rev/36e1ab8c7de0

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36e1ab8c7de0