1232022 - Make content method calls explicit in self-hosted code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

We already outlawed method calls, now police method calls to content functions by enforcing a callContentFunction variant of callFunction.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Mark intrinsics as self-hosted for the purposes or later checks.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Implement callContentFunction() in Interpreter and update existing selfhosted code

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 2b: Implement JSOP_DEBUGCHECKSELFHOSTED in jits.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8697654 [details] [diff] [review]
Part 1: Mark intrinsics as self-hosted for the purposes or later checks.

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

r=me, either with the change suggested below or, if that is for some reason more annoying than I think, as-is.

::: js/src/jsapi.h
@@ +3453,5 @@
>  
> +/**
> + * This enum is used to select whether the defined functions should be marked as
> + * builtin native instrinsics for self-hosted code.
> + * Normal JSAPI consumers must pass NotIntrinsic here.

"Normal JSAPI consumers" sounds a bit weird: adding self-hosting intrinsics will be a normal use case, after all.

More importantly, I'm not convinced we should expose this via JSAPI. Adding intrinsics should happen via the specialized function for it, never by using JS_DefineFunctions directly. Can we perhaps make the public JS_DefineFunctions a wrapper around an internal js::DefineFunctions that takes the enum, with the wrapper hard-coding NotIntrinsic?

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8697659 [details] [diff] [review]
Part 2: Implement callContentFunction() in Interpreter and update existing selfhosted code

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

Nice, thank you. r=me with feedback addressed. Perhaps just ignore my comments on debug vs release builds.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7284,5 @@
>          if (emitterMode == BytecodeEmitter::SelfHosting && !spread) {
>              // We shouldn't see foo(bar) = x in self-hosted code.
>              MOZ_ASSERT(!(pn->pn_xflags & PNX_SETCALL));
>  
>              // Calls to "forceInterpreter", "callFunction" or "resumeGenerator"

Add "callContentFunction" to the comment, too.

::: js/src/js.msg
@@ +106,5 @@
>  MSG_DEF(JSMSG_PROTO_NOT_OBJORNULL,     1, JSEXN_TYPEERR, "{0}.prototype is not an object or null")
>  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
>  MSG_DEF(JSMSG_UNINITIALIZED_THIS,      1, JSEXN_REFERENCEERR, "|this| used uninitialized in {0} class constructor")
>  MSG_DEF(JSMSG_BAD_DERIVED_RETURN,      1, JSEXN_TYPEERR, "derived class constructor returned invalid value {0}")
> +MSG_DEF(JSMSG_NOT_SELFHOSTED,          1, JSEXN_TYPEERR, "callFunction() used on non self-hosted builtin {0}")

"used on content-provided function", perhaps?

::: js/src/vm/Interpreter.cpp
@@ +178,5 @@
> +bool
> +js::Debug_CheckSelfHosted(JSContext* cx, HandleValue fun)
> +{
> +#ifndef DEBUG
> +    MOZ_CRASH("Self hosted checks should only be done in Debug builds");

nit: "self-hosted", but see comment in Interpreter.h

::: js/src/vm/Interpreter.h
@@ +484,5 @@
>  bool
>  DefaultDerivedClassConstructor(JSContext* cx, unsigned argc, Value* vp);
>  
> +bool
> +Debug_CheckSelfHosted(JSContext* cx, HandleValue v);

Do we even need to have this at all in release builds? If the callsite is wrapped in `#ifdef DEBUG`, we shouldn't, right? Or is having the opcode in debug but not in release mode a problem?

The current setup might also be a problem: I don't think we have any checks against loading code in a release build that got compiled and then XDR'd in a debug build. In that case, we'd get the crash here. If we just didn't have the opcode, I don't even know what'd happen. However, I also think that we just shouldn't care: if you're gonna load debug code in a release build, you're gonna have a hard time, as you well should, so deal with it.

Comment 6

[Jeff Walden [:Waldo]]

It seems likely some developers will build and test both opt/debug variants of a single revision.  I don't think we want to have a single XDR version correspond to different sets of JSOp values.  I'd say just enclose all the stuff done/checked in DEBUG in an #ifdef and do nothing at all in release builds.  Crashing isn't a good idea, IMO.

Having JIT support for this debug-only thing is also a bit sadmaking, but I guess that's better than having no verification of stuff when in JITs, I guess.  I guess.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8697664 [details] [diff] [review]
Part 2b: Implement JSOP_DEBUGCHECKSELFHOSTED in jits.

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

Do we need to create a new (debug only) opcode for this?
Can we create an intrinsic (cpp) that does this debug check and emit a call to this during parsing?
Seems like a little bit cleaner, since we wouldn't need a new opcode for this, no jit code and we can make everything debug-only.

If not. The patch seems fine. r+.

Comment 8

[Eric Faust [:efaust]]

Attachment: Part 1.5: Remove DefineAsIntrinsic from external API.

Enough violence here that I want another set of eyes, and some opinion about that unsightly extern function thing. The PropertySpec stuff clearly belongs in with JSAPI, as that's where property spec lives. I just want to call it.

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8698759 [details] [diff] [review]
Part 1.5: Remove DefineAsIntrinsic from external API.

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

Thanks, that's much nicer.

Also, wouldn't it be nice to live in a better world? A world where cats and dogs happily live together and diff tools tell you that a chunk hasn't actually changed, just moved around? I'd like to live in that world.

::: js/src/jsobj.cpp
@@ +2942,5 @@
> +        if (!fun)
> +            return false;
> +
> +        // As jsapi.h notes, fs must point to storage that lives as long
> +        // as fun->object lives.

Might as well fix this comment while copying it: fun->object hasn't been a thing for quite some time, so just remove it?

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a91d3c32c0ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc3315bff36

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b355ab6dc70a

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a91d3c32c0ef
https://hg.mozilla.org/mozilla-central/rev/cdc3315bff36
https://hg.mozilla.org/mozilla-central/rev/b355ab6dc70a