839376 - njn's exact rooting bug

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I'll do a bunch of small patches in this bug.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add an exact root for jstypedarray.cpp.

This one hazard showed up nine times in the analysis, due to different template
instantiations.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add some easy exact roots for jsinfer.cpp.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Exactly root PropertyAccess().

I could have instead rooted |script| at the top, but I figured this function is
called a lot.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Some low-hanging exact rooting fruit.

We're still using static methods to avoid rooting |this|, right?  We still have
at least 40 occurrences of rooted |this| in the code...

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 6

[Brian Hackett [Laid off!]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

> 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.

Comment 8

[Brian Hackett [Laid off!]]

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.

Comment 9

[Steve Fink [:sfink] [:s:]]

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.

Comment 10

[Steve Fink [:sfink] [:s:]]

Oh, and my gcFunctions.txt should be fixed now. It was mainly a problem of bad error handling.

Comment 11

[Nicholas Nethercote [inactive]]

Parts 1, 2 and 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29d3b2c3d8b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3c6d4ee0ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4562906fc61

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Fix four or five more rooting hazards.

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.)

Comment 13

[Steve Fink [:sfink] [:s:]]

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.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Make ObjectClassIs take a HandleObject.

This fixes two hazard reports (two instances of the same thing, AFAICT) in
json.cpp.

Comment 15

[Nicholas Nethercote [inactive]]

Part 4:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0846d180af

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Fix five more easy rooting hazards.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Fix seven more easy rooting hazards.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/29d3b2c3d8b3
https://hg.mozilla.org/mozilla-central/rev/1b3c6d4ee0ae
https://hg.mozilla.org/mozilla-central/rev/c4562906fc61
https://hg.mozilla.org/mozilla-central/rev/9a0846d180af

Comment 20

[Steve Fink [:sfink] [:s:]]

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

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

Nice.

Comment 21

[Steve Fink [:sfink] [:s:]]

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 22

[Steve Fink [:sfink] [:s:]]

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 23

[Steve Fink [:sfink] [:s:]]

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.

Comment 24

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/560ede42c3f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8f0f701666
https://hg.mozilla.org/integration/mozilla-inbound/rev/e650f1bab42b

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Fix another seven rooting hazards.

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/560ede42c3f6
https://hg.mozilla.org/mozilla-central/rev/ba8f0f701666
https://hg.mozilla.org/mozilla-central/rev/e650f1bab42b

Comment 27

[Steve Fink [:sfink] [:s:]]

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?

Comment 28

[Nicholas Nethercote [inactive]]

Part 8:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d771cec55373

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Fix six rooting hazards in methodjit/PolyIC.cpp.

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Fix five rooting hazards in jsproxy.cpp.

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).

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Fix two more easy rooting hazards.

Comment 32

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Fix two easy rooting hazards in shell/js.cpp.

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.

Comment 33

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Fix ten easy rooting hazards in vm/Debugger.cpp.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d771cec55373

Comment 35

[Terrence Cole [:terrence]]

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().

Comment 36

[Terrence Cole [:terrence]]

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|.

Comment 37

[Terrence Cole [:terrence]]

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.

Comment 38

[Terrence Cole [:terrence]]

(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.

Comment 39

[Nicholas Nethercote [inactive]]

Parts 9--13:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b029c64d2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2843bae075f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb11273bd848
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3697783abdf
https://hg.mozilla.org/integration/mozilla-inbound/rev/d30c69e19fa0

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7b029c64d2a
https://hg.mozilla.org/mozilla-central/rev/e2843bae075f
https://hg.mozilla.org/mozilla-central/rev/fb11273bd848
https://hg.mozilla.org/mozilla-central/rev/a3697783abdf
https://hg.mozilla.org/mozilla-central/rev/d30c69e19fa0

Comment 41

[Nicholas Nethercote [inactive]]

Attachment: (part 14) - More exact rooting in shell/js.cpp.

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.

Comment 42

[Steve Fink [:sfink] [:s:]]

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?

Comment 43

[Nicholas Nethercote [inactive]]

Part 14:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cddf5c045d5

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cddf5c045d5

Comment 45

[Terrence Cole [:terrence]]

And exact rooting is (finally) done.