Closed Bug 854029 Opened 9 years ago Closed 9 years ago

Fix RelocatableValue

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

Attached patch v0Splinter Review
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.
Attachment #728481 - Flags: review?(wmccloskey)
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?
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 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.
Attachment #728481 - Flags: review?(wmccloskey) → review+
Thanks, I had forgotten that isMarkable already handles NULL.

https://hg.mozilla.org/integration/mozilla-inbound/rev/36e1ab8c7de0
https://hg.mozilla.org/mozilla-central/rev/36e1ab8c7de0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.