Closed Bug 1232022 Opened 5 years ago Closed 5 years ago

Make content method calls explicit in self-hosted code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: efaust, Assigned: efaust)

Details

Attachments

(4 files)

We already outlawed method calls, now police method calls to content functions by enforcing a callContentFunction variant of callFunction.
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?
Attachment #8697654 - Flags: review?(till) → review+
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.
Attachment #8697659 - Flags: review?(till) → review+
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 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+.
Attachment #8697664 - Flags: review?(hv1989) → review+
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.
Attachment #8698759 - Flags: review?(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?
Attachment #8698759 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/a91d3c32c0ef
https://hg.mozilla.org/mozilla-central/rev/cdc3315bff36
https://hg.mozilla.org/mozilla-central/rev/b355ab6dc70a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.