Closed
Bug 854029
Opened 11 years ago
Closed 11 years ago
Fix RelocatableValue
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file)
4.17 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter 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 1•11 years ago
|
||
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•11 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•11 years ago
|
||
Thanks, I had forgotten that isMarkable already handles NULL. https://hg.mozilla.org/integration/mozilla-inbound/rev/36e1ab8c7de0
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/36e1ab8c7de0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•