1536851 - JS_NewObjectForConstructor doesn't cooperate with JS subclassing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P5)

Description

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36

Steps to reproduce:

I have write a native class based in the examples contained in test:

struct mybase { int val; };

static JSClassOps jsmybase_ops = {
	nullptr
};

static JSClass jsmybase_class = {
	"mybase", JSCLASS_HAS_PRIVATE, &jsmybase_ops
};

// jbase constructor.
static bool jsmybase_ctor(JSContext* ctx, unsigned argc, JS::Value* vp)
{
      JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
      {
           JS::RootedObject jsmybaseObj(ctx, JS_NewObjectForConstructor(ctx, &jsmybase_class, args));
	   {
	          JS_SetPrivate(jsmybaseObj, new mybase);
	   }
	   args.rval().setObject(*jswxAppObj);
       }
       return true;
}

This is called in main function after SM initialization:

          JSObject* proto = JS_InitClass(ctx, globalObj, nullptr, &jsmybase_class, jsbase_ctor, 0, nullptr, nullptr, nullptr, nullptr);

I have executed this script in a file:

class myclass extends mybase {
    OnInit() {}
}

let a = new myclass();

a.OnInit();

I tested in 3 version of SpiderMonkey 64, 65, 66. With same result.

Actual results:

class myclass extends mybase {
    OnInit() {}
}

let a = new myclass();

a.OnInit();  // <-- OnInit is undefined

let a_proto = a.__proto__; // a_proto is mybase.prototype not myclass.prototype
let a_proto_proto = a.__proto__.__proto__; // a_proto_proto is Object.prototype not mybase.prototype

I tried to set this in jsmybase_ctor like this:

        args.rval().setObject(*jswxAppObj);
        args.setThis(JS::ObjectValue(*jswxAppObj));

But don't work.

Expected results:

class myclass extends mybase {
    OnInit() {}
}

let a = new myclass();

a.OnInit();  // <-- OnInit is undefined

let a_proto = a.__proto__; // a_proto should be myclass.prototype
let a_proto_proto = a.__proto__.__proto__; // a_proto_proto should be mybase.prototype

Comment 1

[Neha Kochar [:neha]]

Bobby, do you have ideas on this?

Comment 2

[Jason Orendorff [:jorendorff]]

Yeah, our own builtin constructors all do this in order to support subclassing, but JS_NewObjectForConstructor does this.

I think we could just change JS_NewObjectForConstructor.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1536851 - Part 1: Change JS_NewObjectForConstructor to respect `new.target`, like non-derived JS classes. r?jwalden

Classes defined in JS using the class keyword, without using extends,
implicitly construct a new this object before running the body of the
constructor, and they use new.target.prototype as the new object's
[[Prototype]]. This is one of the ways that JS classes support subclassing.
When the subclass calls the base class constructor, the subclass's .prototype
is used.

Until now, we did not do this for classes defined in C++ using
JS_NewObjectForConstructor. But there's no reason it shouldn't work the same
way. Changing this is a nicety for embedders -- there's nothing using it in
Firefox or internally right now, except for testing.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1536851 - Part 2: JS_NewObjectForConstructor should fail (not crash) if not constructing. r?jwalden

Depends on D31490

Comment 5

[Jason Orendorff [:jorendorff]]

I just realized the change I made in part 1 introduces an unintended gotcha for embedders: if there's no new.target (which is the case if the base class is called directly, without new) then it asserts and crashes.

The most idiomatic thing is for Base() without new to be a TypeError, so that's what part 2 does. Alternatively, it could fall back on args.callee() in that case, for maximum backward compatibility, but that seems too weird. I copied the approach from the Promise constructor.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/459c574bd2d0
Part 1: Change JS_NewObjectForConstructor to respect `new.target`, like non-derived JS classes. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/5f5575bb2b93
Part 2: JS_NewObjectForConstructor should fail (not crash) if not constructing. r=jwalden

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/459c574bd2d0
https://hg.mozilla.org/mozilla-central/rev/5f5575bb2b93