814729 - Add getSelfHostedValue function to JS shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Attachment: v1

Using getSelfHostedValue, all (clonable) values can be retrieved from the self-hosting compartment.

This obviates the need to install self-hosted functions into the global or into some builtin class for testing purposes.

In combination with `make self-hosting` and the MOZ_SELFHOSTEDJS env var, it enables full testing of self-hosted code without recompilation.

Comment 1

[Till Schneidereit [:till]]

Comment on attachment 684722 [details] [diff] [review]
v1

I guess Luke's going to have his hands full when he gets back anyway, so I hope you don't mind doing this quick review.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 684722 [details] [diff] [review]
v1

This function should return undefined if the function is not found. Currently it asserts.

>+    RootedAtom srcAtom(cx, AtomizeString(cx, args[0].toString()));
>+    if (!srcAtom)
>+        return false;
>+    RootedPropertyName srcName(cx, srcAtom->toPropertyName(cx));
>+    RootedValue val(cx);
>+    if (!cx->global()->getIntrinsicValue(cx, srcName, &val))
>+        return false;
>+    args.rval().set(val.get());
>+    return true;
>+}
>+

No need for these intermediates and no need for |toPropertyName(cx)|; we also have a helper function |ToAtom| already. I suggest:

    RootedAtom srcAtom(cx, ToAtom(cx, args[0]));
    if (!srcAtom)
        return false;
    return cx->global()->getIntrinsicValue(cx, srcAtom->asPropertyName(), args.rval());

>+JS_FN_HELP("relaxRootChecks", RelaxRootChecks, 0, 0,

Nitpick: Why change formatting of this line?

>+JS_FN_HELP("getSelfHostedValue", GetSelfHostedValue, 1, 0,
>+"getSelfHostedValue()",
>+"  Get a self-hosted value by its name. Note that these values don't get \n"
>+"  cached, so repeatedly getting the same value creates multipe distinct clones."),
>+
>     JS_FS_HELP_END
> };

This help message does not describe what the function above is doing. |getIntrinsicValue| is going to clone and cache into the holder. So either the comment is wrong or the above code is wrong. :)

Typo: multipe -> multiple.
Nitpick: See formatting above.

Comment 3

[Till Schneidereit [:till]]

Attachment: v2

Ugh, sorry about the mess.

I moved storing the clone in the holder from Runtime::CloneSelfHostedValue to GlobalObject::getSelfHostedValue because I think that not automatically caching the clones in the shell is the better behavior for testing. Also, it makes more sense, I think.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 684814 [details] [diff] [review]
v2

>@@ -460,18 +459,16 @@ JSRuntime::cloneSelfHostedValue(JSContex
>         vp.set(funVal);
>     } else if (funVal.toObject().isFunction()){
>         RootedFunction fun(cx, funVal.toObject().toFunction());
>         RootedObject clone(cx, CloneFunctionObject(cx, fun, cx->global(), fun->getAllocKind()));
>         if (!clone)
>             return false;
>         vp.set(ObjectValue(*clone));
>     }
>-    DebugOnly<bool> ok = JS_DefinePropertyById(cx, holder, NameToId(name), vp, NULL, NULL, 0);
>-    JS_ASSERT(ok);
>     return true;
> }

This function asserts on intrinsics which are not found, as you do |funVal.toObject().isFunction()| while |funVal| could be undefined. So when called from the shell function, the shell crashes when getting nonexisting intrinsics.

>@@ -3576,16 +3576,33 @@ RelaxRootChecks(JSContext *cx, unsigned 
> ...
>+    RootedPropertyName srcName(cx, srcAtom->toPropertyName(cx));

asPropertyName()

Comment 5

[Till Schneidereit [:till]]

Attachment: v3

> This function asserts on intrinsics which are not found, as you do
> |funVal.toObject().isFunction()| while |funVal| could be undefined. So when
> called from the shell function, the shell crashes when getting nonexisting
> intrinsics.

I'm in the process of overhauling Runtime::cloneSelfHostedValue, which will fix that issue automatically. You're right, though: no need to let it in even in the short term.

Comment 6

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa4aea36603

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1aa4aea36603