Open Bug 972087 Opened 6 years ago Updated 6 years ago

CloneObject called on non-self-hosted function.

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

ASSIGNED

People

(Reporter: djvj, Assigned: till, NeedInfo)

Details

Attachments

(1 file)

Ran into this while working on bug 952891.  I tried to add getters for the self-hosted function name and change call sites to use the getter/setter, and it triggered some safety asserts in the accessor methods.

Reproducible with the following simplified patch and test case (inlining since they're small)

PATCH:
diff --git a/js/src/vm/SelfHosting.cpp b/js/src/vm/SelfHosting.cpp
--- a/js/src/vm/SelfHosting.cpp
+++ b/js/src/vm/SelfHosting.cpp
@@ -955,18 +955,21 @@ CloneObject(JSContext *cx, HandleObject
             RootedFunction fun(cx, &srcObj->as<JSFunction>());
             bool hasName = fun->atom() != nullptr;
             js::gc::AllocKind kind = hasName
                                      ? JSFunction::ExtendedFinalizeKind
                                      : fun->getAllocKind();
             clone = CloneFunctionObject(cx, fun, cx->global(), kind, TenuredObject);
             // To be able to re-lazify the cloned function, its name in the
             // self-hosting compartment has to be stored on the clone.
-            if (clone && hasName)
+            if (clone && hasName) {
+                JS_ASSERT(fun->isSelfHostedBuiltin());
+                JS_ASSERT(clone->as<JSFunction>().isSelfHostedBuiltin());
                 clone->as<JSFunction>().setExtendedSlot(0, StringValue(fun->atom()));
+            }
         }
     } else if (srcObj->is<RegExpObject>()) {
         RegExpObject &reobj = srcObj->as<RegExpObject>();
         RootedAtom source(cx, reobj.getSource());
         clone = RegExpObject::createNoStatics(cx, source, reobj.getFlags(), nullptr);
     } else if (srcObj->is<DateObject>()) {
         clone = JS_NewDateObjectMsec(cx, srcObj->as<DateObject>().UTCTime().toNumber());
     } else if (srcObj->is<BooleanObject>()) {


TEST CASE

(function () {})(...[0,1,2]);
Till, how bad is this?
Flags: needinfo?(till)
> Till, how bad is this?

Luckily, it's not bad at all. What's going on is that we unconditionally store the name, even for intrinsic, i.e. C++-implemented, functions. Since we're guaranteed to create ExtendedFinalizeKind functions, this is ok.

However, it's not needed, so with this patch we check if the function is interpreted and only create an ExtendedFinalizeKind function and set the name in that case.

Jason, please land this or set checkin-needed if you r+ it - I'll be on vacation starting this evening. Given that it's not urgent, you can also just let it rest until I'm back, I suppose.
Attachment #8383453 - Flags: review?(jorendorff)
Assignee: nobody → till
Status: NEW → ASSIGNED
Flags: needinfo?(till)
Comment on attachment 8383453 [details] [diff] [review]
Don't store self-hosting name on clones of intrinsic functions.

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

Oh, of course. Thanks for taking a look. Will set checkin-needed...
Attachment #8383453 - Flags: review?(jorendorff) → review+
You need to log in before you can comment on or make changes to this bug.