Add getSelfHostedValue function to JS shell

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: till, Assigned: till)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 684722 [details] [diff] [review]
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.
Attachment #684722 - Flags: review?(luke)
(Assignee)

Comment 1

6 years ago
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.
Attachment #684722 - Flags: review?(luke) → review?(shu)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Created attachment 684814 [details] [diff] [review]
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.
Assignee: general → tschneidereit
Attachment #684722 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #684722 - Flags: review?(shu)
Attachment #684814 - Flags: review?(shu)

Comment 4

6 years ago
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()
(Assignee)

Comment 5

6 years ago
Created attachment 684848 [details] [diff] [review]
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.
Attachment #684814 - Attachment is obsolete: true
Attachment #684814 - Flags: review?(shu)
Attachment #684848 - Flags: review?(shu)

Updated

6 years ago
Attachment #684848 - Flags: review?(shu) → review+

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1aa4aea36603
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.