Our current poisoning scheme does make Values invalid, however, we frequently only access this is Value::isMarkable, particularly when marking. Unfortunately, Value::isMarking happens to be false, at least with our Nursery poisoning pattern, so we will silently skip over poisoned Slots in this case. Instead, we should poison at the value level by ensuring Value::isObject is true, but with an invalid pointer, so that subsequent code will crash.

Comment on attachment 8591002 [details] [diff] [review] make_poison_crash_when_used_as_a_value-v0.diff I can confirm that this crashes immediately while tracing the uninitialized slots that were previously seen as doubles. Thanks so much for doing this!

Comment on attachment 8591002 [details] [diff] [review] make_poison_crash_when_used_as_a_value-v0.diff Review of attachment 8591002 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsutil.h @@ +304,5 @@ > char* env = getenv("JSGC_DISABLE_POISONING"); > if (env) > poison = false; > inited = true; > } I wonder if this whole thing could be replaced with static bool poison = getenv("JSGC_DISABLE_POISONING"); Ah, who cares. @@ +309,5 @@ > > + if (poison) { > + // Without a valid Value tag, a poisoned Value may look like a valid > + // floating point number. To ensure that we crash more readily when > + // observing a poised Value, we make the poison an invalid ObjectValue. *poisoned @@ +315,5 @@ > + memset(&obj, value, sizeof(obj)); > +#if defined(JS_PUNBOX64) > + obj >>= JSVAL_TAG_SHIFT; > +#endif > + jsval_layout layout = OBJECT_TO_JSVAL_IMPL((JSObject*)obj); reinterpret_cast<> ObjectOrNullValue(reinterpret_cast<JSObject*>(obj)) doesn't work?

(In reply to Steve Fink [:sfink, :s:] from comment #2) > Comment on attachment 8591002 [details] [diff] [review] > make_poison_crash_when_used_as_a_value-v0.diff > > Review of attachment 8591002 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsutil.h > @@ +304,5 @@ > > char* env = getenv("JSGC_DISABLE_POISONING"); > > if (env) > > poison = false; > > inited = true; > > } > > I wonder if this whole thing could be replaced with > > static bool poison = getenv("JSGC_DISABLE_POISONING"); > > Ah, who cares. Yeah, I'll clean this up while I'm in the area. > @@ +309,5 @@ > > > > + if (poison) { > > + // Without a valid Value tag, a poisoned Value may look like a valid > > + // floating point number. To ensure that we crash more readily when > > + // observing a poised Value, we make the poison an invalid ObjectValue. > > *poisoned That may explain why I didn't see any crashes. :-( > @@ +315,5 @@ > > + memset(&obj, value, sizeof(obj)); > > +#if defined(JS_PUNBOX64) > > + obj >>= JSVAL_TAG_SHIFT; > > +#endif > > + jsval_layout layout = OBJECT_TO_JSVAL_IMPL((JSObject*)obj); > > reinterpret_cast<> > > ObjectOrNullValue(reinterpret_cast<JSObject*>(obj)) doesn't work? No, this just dispatches to ObjectValue |if (obj)|, so does *obj.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e6c7fde463 https://hg.mozilla.org/integration/mozilla-inbound/rev/42ed856b37d9

dmajor pointed out in IRC that it looks like your environment checking logic is reversed, eg it only enables poisoning when DISABLE_POISONING is set.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ef1b55f785 to see if it possibly fixes a suddenly-near-permafailing mochitest-e10s-bc1 orange that popped up around the time this landed. https://treeherder.mozilla.org/logviewer.html#?job_id=8910719&repo=mozilla-inbound [16:44] terrence KWierso: I need to push a fixup anyway, so feel free to backout if you want to check directly

(In reply to Andrew McCreight [:mccr8] from comment #5) > dmajor pointed out in IRC that it looks like your environment checking logic > is reversed, eg it only enables poisoning when DISABLE_POISONING is set. Yup, missed the negation. "Luckily", disabling poisoning results in a reliable bc1 timeout, so there is that.

I ran ~10 bc1 with and without poisoning and did not see a significant difference in their (startlingly large) timeout rate. So, relanding, I guess.

This appears to have regressed builds with js-diagnostics enabled. I think I'd like to let this sit a week and see if this makes any crashes reliable, then mask the new code as #ifdef DEBUG so that diagnostics builds get the prior behavior and only debug builds get the better assertions.

This got done. I'm not sure why the bug isn't closed.