Fix RelocatableValue

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 728481 [details] [diff] [review]
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.
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?
(Assignee)

Comment 2

5 years ago
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+
(Assignee)

Comment 4

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.