Add uses of rooter and handle classes to get the sunspider and v8bench tests in jit-tests to pass (along with a lot of other jit-tests).

Is this bug still needed?

Yes, this is necessary for generational GC. Landing it immediately isn't critical, but I'd like it to stop rotting. Bill, can you review this pretty soon? (after incremental GC lands). It is large but very straightforward and should just need skimming.

Comment on attachment 585295 [details] [diff] [review] patch (10f831bfaf08) Review of attachment 585295 [details] [diff] [review]: ----------------------------------------------------------------- Sorry again for the delay. This looks nice. I guess we have to figure out what to do for char* pointers to inlined strings. Maybe we should have a special kind of rooter for those. ::: js/src/frontend/Parser.cpp @@ +1306,5 @@ > #endif /* JS_HAS_DESTRUCTURING */ > > case TOK_NAME: > { > + RootedVar<PropertyName*> name(context, tokenStream.currentToken().name()); RootedVarPropertyName? ::: js/src/frontend/TokenStream.h @@ +832,5 @@ > void updateLineInfoForEOL(); > void updateFlagsForEOL(); > > Token tokens[ntokens];/* circular token buffer */ > + CheckRoot tokensRoot; /* prevent overwriting of token buffer */ Please explain this. Maybe we don't need to root this because it only contains atoms and because we never trace through it during GC? ::: js/src/jsapi.cpp @@ +3577,4 @@ > JSPropertyOp getter, JSStrictPropertyOp setter, uintN attrs) > { > CHECK_REQUEST(cx); > + RootObject obj(cx, &obj_); Can you use RootedVarObject here? @@ +3649,4 @@ > const Value &value, PropertyOp getter, StrictPropertyOp setter, uintN attrs, > uintN flags, intN tinyid) > { > + RootObject obj(cx, &obj_); and here? @@ +3978,3 @@ > return JS_FALSE; > if (objp) > *objp = obj; Couldn't obj have been poisoned at this point? ::: js/src/jscntxt.h @@ +1589,4 @@ > typedef Root<Value> RootValue; > > /* Mark a stack location as a root for a rooting analysis. */ > class CheckRoot I realize I should have mentioned this in the previous patch, but the name CheckRoot is confusing. Wouldn't SkipRoot be more appropriate? ::: js/src/jsfun.cpp @@ +428,5 @@ > if (!obj->isStrictArguments()) > return true; > > + RootedVar<StrictArgumentsObject*> argsobj(cx, obj->asStrictArguments()); > + RootId idRoot(cx, &id); Can you use RootedVarId? ::: js/src/jsinferinlines.h @@ +1327,5 @@ > inline bool > JSScript::ensureRanAnalysis(JSContext *cx, JSObject *scope) > { > JSScript *self = this; > + js::CheckRoot root(cx, &self); Why don't we just root self? I asked the same question last time and I can't find an answer. ::: js/src/jsinterp.cpp @@ +3198,5 @@ > JS_ASSERT(entry->kshape != entry->pshape); > JS_ASSERT(!shape->hasSlot()); > } > #endif > + */ I'm not familiar with this code, but these assertions seem kind of useful. Why do they have to go? ::: js/src/jspropertycache.cpp @@ -59,5 @@ > - */ > - if (!pobj->nativeContains(cx, *shape)) { > - PCMETER(oddfills++); > - return JS_NO_PROP_CACHE_FILL; > - } Again, I don't understand what's happening here. ::: js/src/jsscope.cpp @@ +312,5 @@ > JS_ASSERT(!inDictionary()); > > Shape *shape = JS_PROPERTY_TREE(cx).getChild(cx, this, numFixedSlots(), child); > if (shape) { > + //JS_ASSERT(shape->parent == this); Is this not worth keeping? ::: js/src/jsscope.h @@ -710,5 @@ > /* Copy constructor disabled, to avoid misuse of the above form. */ > - Shape(const Shape &other); > - > - /* Not defined: Shapes must not be stack allocated. */ > - ~Shape(); Why this change? ::: js/src/vm/StringObject-inl.h @@ +56,5 @@ > StringObject::init(JSContext *cx, JSString *str) > { > JS_ASSERT(gc::GetGCKindSlots(getAllocKind()) == 2); > > + setStringThis(str); Moving this up means changes semantics if we OOM. Can we just root str?

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c82a6b298c According to Try this dings dromaeo-v8 and dromaeo-basics by a couple %. Other dromaeo tests, including DOM and CSS, are unaffected. Don't back this patch out because of these regressions.

Sorry, I had to back this out (along with two no-bug followups by Waldo): https://hg.mozilla.org/integration/mozilla-inbound/rev/906aa73122d9 because it apparently caused crashes during test_finalizer.js in Linux64 *PGO* builds only: https://tbpl.mozilla.org/php/getParsedLog.php?id=10861493&tree=Mozilla-Inbound

(In reply to Brian Hackett (:bhackett) from comment #4) > According to Try this dings dromaeo-v8 and dromaeo-basics by a couple %. > Other dromaeo tests, including DOM and CSS, are unaffected. inbound shows also regressions on DOM, CSS, String/Array/Eval/Regexp, jslib.

(In reply to Matt Brubeck (:mbrubeck) from comment #5) > Sorry, I had to back this out (along with two no-bug followups by Waldo) > because it apparently caused crashes during test_finalizer.js in Linux64 > *PGO* builds only The backout did not fix the crashes, so I will re-land the backed-out changesets. Sorry again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cfb132c7c16 https://hg.mozilla.org/integration/mozilla-inbound/rev/95fd0e025439 https://hg.mozilla.org/integration/mozilla-inbound/rev/94eb880e0cad

https://hg.mozilla.org/mozilla-central/rev/4cfb132c7c16 https://hg.mozilla.org/mozilla-central/rev/95fd0e025439 https://hg.mozilla.org/mozilla-central/rev/94eb880e0cad If possible, please don't push any no-bug changesets that is not a whitespace or comment change.