Closed Bug 839376 Opened 7 years ago Closed 7 years ago

njn's exact rooting bug

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t][leave open])

Attachments

(14 files, 1 obsolete file)

922 bytes, patch
sfink
: review+
Details | Diff | Splinter Review
4.44 KB, patch
sfink
: review+
Details | Diff | Splinter Review
8.73 KB, patch
sfink
: review+
Details | Diff | Splinter Review
8.35 KB, patch
sfink
: review+
Details | Diff | Splinter Review
9.60 KB, patch
sfink
: review+
Details | Diff | Splinter Review
4.26 KB, patch
sfink
: review+
Details | Diff | Splinter Review
7.54 KB, patch
sfink
: review+
Details | Diff | Splinter Review
15.35 KB, patch
sfink
: review+
Details | Diff | Splinter Review
5.57 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.25 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.90 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.05 KB, patch
terrence
: review+
Details | Diff | Splinter Review
8.14 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.62 KB, patch
sfink
: review+
Details | Diff | Splinter Review
I'll do a bunch of small patches in this bug.
Blocks: ExactRooting
This one hazard showed up nine times in the analysis, due to different template
instantiations.
Attachment #711678 - Flags: review?(sphink)
I could have instead rooted |script| at the top, but I figured this function is
called a lot.
Attachment #711686 - Flags: review?(bhackett1024)
We're still using static methods to avoid rooting |this|, right?  We still have
at least 40 occurrences of rooted |this| in the code...
Attachment #711692 - Flags: review?(sphink)
Comment on attachment 711692 [details] [diff] [review]
(part 3) - Some low-hanging exact rooting fruit.

Review of attachment 711692 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/ParallelArray.cpp
@@ +1423,4 @@
>      if (args.length() >= 2)
>          defaultValue = args[1];
>      else
>          defaultValue.setUndefined();

This could be 

RootedValue defaultValue(cx, args.get(1));
Comment on attachment 711686 [details] [diff] [review]
(part 3) - Exactly root PropertyAccess().

Review of attachment 711686 [details] [diff] [review]:
-----------------------------------------------------------------

Is the static analysis reporting hazards somewhere around here?  PropertyAccess (and almost everything in jsinfer.cpp) is called with GC suppressed and does not need to be rooted.
Attachment #711686 - Flags: review?(bhackett1024)
> Is the static analysis reporting hazards somewhere around here? 
> PropertyAccess (and almost everything in jsinfer.cpp) is called with GC
> suppressed and does not need to be rooted.

It was this one:

> Function 'jsinfer.cpp:void PropertyAccess(JSContext*, JSScript*, uint8*, js::types::TypeObject*, js::types::StackTypeSet*, jsid) [with PropertyAccessKind access = (PropertyAccessKind)2u; JSContext = JSContext; jsbytecode = unsigned char; js::RawId = jsid]' has unrooted 'script' live across GC call 'void js::types::TypeSet::addType(JSContext*, js::types::Type)' at js/src/jsinfer.cpp:1169

It occurs within PropertyAccess, which is listed as a CanGC function.
Hmm, looking at http://people.mozilla.org/~sfink/data/gcFunctions.txt I see 3173 functions that can GC and 476 suppressed functions.  On my machine, with a slightly outdated tree, I get 4242 functions that can GC and 7745 suppressed functions.  The latter numbers are much closer to being correct.

I'll do a clean run off inbound tip to try to narrow down what's going on.  For now I would just ignore anything in jsinfer, Ion and methodjit, as these are where most of those suppressed functions are going to be.
Attachment #711678 - Flags: review?(sphink) → review+
Attachment #711681 - Flags: review?(sphink) → review+
Comment on attachment 711692 [details] [diff] [review]
(part 3) - Some low-hanging exact rooting fruit.

Review of attachment 711692 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

::: js/src/builtin/Eval.cpp
@@ +163,5 @@
>      JS_ASSERT_IF(evalType == INDIRECT_EVAL, scopeobj->isGlobal());
>      AssertInnerizedScopeChain(cx, *scopeobj);
>  
> +    Rooted<GlobalObject*> scopeObjGlobal(cx, &scopeobj->global());
> +    if (!GlobalObject::isRuntimeCodeGenEnabled(cx, scopeObjGlobal)) {

Hm... you could probably use fromMarkedLocation here (scopeobj is rooted, and you're just temporarily using the global), but who cares? I don't think this is very hot, and your way is simpler.
Attachment #711692 - Flags: review?(sphink) → review+
Oh, and my gcFunctions.txt should be fixed now. It was mainly a problem of bad error handling.
The one is GlobalObject.cpp is this:

> Function 'uint8 js::GlobalObject::isRuntimeCodeGenEnabled(JSContext*, class JS::Handle<js::GlobalObject*>)' has unrooted '__temp_4' live across GC call *allows at js/src/vm/GlobalObject.cpp:494

I just moved the BooleanValue() call out of line.  Not sure if that'll fix it,
but at the very least it should make the hazard message easier to understand.
(That explains the "four or five" in the patch description.)
Attachment #712744 - Flags: review?(sphink)
Comment on attachment 712744 [details] [diff] [review]
(part 4) - Fix four or five more rooting hazards.

Review of attachment 712744 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/GlobalObject.cpp
@@ +491,5 @@
>           * and that it permits runtime code generation, then cache the result.
>           */
>          JSCSPEvalChecker allows = cx->runtime->securityCallbacks->contentSecurityPolicyAllows;
> +        Value boolValue = BooleanValue(!allows || allows(cx));
> +        v.set(global, HeapSlot::Slot, RUNTIME_CODEGEN_ENABLED, boolValue);

Hm. I can't decide whether the hazard was real or not, but we definitely want your change. The issue is whether the compiler could put some sort of GC-movable thing from invoking the first half of v.set() onto the stack, then call allows(cx) which triggers a moving GC, and then write into the thing stored on the stack. I don't see how that could happen in this case, but it smells unsafe. And thinking about it makes my head hurt.

Your change adds a sequence point between allows(cx) and the v.set call, so nothing fishy can happen.
Attachment #712744 - Flags: review?(sphink) → review+
This fixes two hazard reports (two instances of the same thing, AFAICT) in
json.cpp.
Attachment #712768 - Flags: review?(sphink)
Attachment #711686 - Attachment is obsolete: true
Attachment #711692 - Attachment description: (part 4) - Some low-hanging exact rooting fruit. → (part 3) - Some low-hanging exact rooting fruit.
Attachment #712792 - Attachment description: (part 5) - Fix five more easy rooting hazards. → (part 6) - Fix five more easy rooting hazards.
Comment on attachment 712792 [details] [diff] [review]
(part 6) - Fix five more easy rooting hazards.

Review of attachment 712792 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Eval.cpp
@@ +30,2 @@
>          if (JSObjectOp op = o->getClass()->ext.innerObject) {
>              Rooted<JSObject*> obj(cx, o);

Still need this?
Comment on attachment 712768 [details] [diff] [review]
(part 5) - Make ObjectClassIs take a HandleObject.

Review of attachment 712768 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #712768 - Flags: review?(sphink) → review+
Comment on attachment 712792 [details] [diff] [review]
(part 6) - Fix five more easy rooting hazards.

Review of attachment 712792 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with Ms2ger's comment taken into account.
Comment on attachment 712792 [details] [diff] [review]
(part 6) - Fix five more easy rooting hazards.

Review of attachment 712792 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with Ms2ger's comment taken into account.
Attachment #712792 - Flags: review?(sphink) → review+
Comment on attachment 712796 [details] [diff] [review]
(part 7) - Fix seven more easy rooting hazards.

Review of attachment 712796 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome progress!

::: js/src/jsobj.cpp
@@ +2903,5 @@
>          objp.set(NULL);
>          return true;
>      }
>  
> +    RootedObject cobj(cx);

I'd prefer this to be explicitly NULL to show that its initial value is meaningful.

  RootedObject cobx(cx, NULL);

::: js/src/vm/ObjectImpl.cpp
@@ +685,5 @@
>      NEW_OBJECT_REPRESENTATION_ONLY();
>  
>      Rooted<ObjectImpl*> current(cx, obj);
>  
> +    RootedValue get(cx);

I know this is pre-existing, but this sounds funny. Could it be 'getter' instead of 'get'? I didn't read the code to see, but I notice that 'setter' is used later.
Attachment #712796 - Flags: review?(sphink) → review+
I made JSObject::getType() self-root because (a) it's a case where it's really
easy to see that we don't use |this| again, and (b) getType() is called in lots
of places.
Attachment #713264 - Flags: review?(sphink)
Comment on attachment 713264 [details] [diff] [review]
(part 8) - Fix another seven rooting hazards.

Review of attachment 713264 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I like the getType() part.

::: js/src/jsobj.h
@@ +284,5 @@
>      friend class  js::NewObjectCache;
>  
>      /* Make the type object to use for LAZY_TYPE objects. */
> +    static js::types::TypeObject *makeLazyType(JSContext *cx,
> +                                               js::HandleObject obj);

Why is this wrapped? Surely that fits in 99 columns?
Attachment #713264 - Flags: review?(sphink) → review+
The AutoSuppressGC in RenewProxyObject is ugly, but I didn't want to root |obj|
and |priv| just for DEBUG builds, especially when isOuterWindow() almost
certainly cannot GC in practice (it's marked as a GC function only because it's
virtual).
Attachment #713792 - Flags: review?(sphink)
I think the first one is a false positive due to the |RootedFunction fun|
shadowing a |JSFunction *fun| parameter.  Hopefully changing its name to |f|
will fix it.
Attachment #713796 - Flags: review?(sphink)
Attachment #713783 - Flags: review?(sphink) → review+
Attachment #713792 - Flags: review?(sphink) → review+
Comment on attachment 713795 [details] [diff] [review]
(part 11) - Fix two more easy rooting hazards.

Review of attachment 713795 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsdbgapi.cpp
@@ +511,5 @@
>  JS_GetFunctionScript(JSContext *cx, JSFunction *fun)
>  {
>      if (fun->isNative())
>          return NULL;
> +    RootedScript script(cx);

Uhg. This one is actually a performance concern. Please move the Rooted variant below isInterpretedLazy().
Attachment #713795 - Flags: review?(sphink) → review+
Comment on attachment 713796 [details] [diff] [review]
(part 12) - Fix two easy rooting hazards in shell/js.cpp.

Review of attachment 713796 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/js.cpp
@@ +1873,2 @@
>                  RootedScript script(cx);
> +                JSFunction::maybeGetOrCreateScript(cx, f, &script);

Why the name change? |f| seems worse in almost every way than |fun|.
Attachment #713796 - Flags: review?(sphink) → review+
Comment on attachment 713801 [details] [diff] [review]
(part 13) - Fix ten easy rooting hazards in vm/Debugger.cpp.

Review of attachment 713801 [details] [diff] [review]:
-----------------------------------------------------------------

Weird. I'd expect that Invoke would set those after all GCs, but I guess even one counterexample would ruin it.
Attachment #713801 - Flags: review?(sphink) → review+
(In reply to Terrence Cole [:terrence] from comment #36)
> ::: js/src/shell/js.cpp
> @@ +1873,2 @@
> >                  RootedScript script(cx);
> > +                JSFunction::maybeGetOrCreateScript(cx, f, &script);
> 
> Why the name change? |f| seems worse in almost every way than |fun|.

Nevermind, my question is already answered in comment 32.
I just made ToStringHelper::mStr a RootedString.  I think this is safe because
ToStringHelper and IdStringifier (a sub-class) objects are only used on the
stack.
Attachment #718819 - Flags: review?(sphink)
Comment on attachment 718819 [details] [diff] [review]
(part 14) - More exact rooting in shell/js.cpp.

Review of attachment 718819 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/js.cpp
@@ +1909,5 @@
>      Sprinter sprinter(cx);
>      if (!sprinter.init())
>          return false;
> +    RootedFunction nullFun(cx);
> +    bool ok = DisassembleScript(cx, script, nullFun, p.lines, p.recursive, &sprinter);

Can't you use NullPtr() instead of nullFun?
Attachment #718819 - Flags: review?(sphink) → review+
And exact rooting is (finally) done.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.