1164664 - Make js-ctypes create a proper AutoEntryScript when it invokes closures

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Bobby Holley (:bholley)]

The current setup is wrong, and it's biting me in bug 1162187. I was sort of hoping this problem would be solved by bug 971673, but I'm just going to do something simpler and solve my immediate problem.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=862180c153ea

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Swap out the 'default JSContext callback' for something capable of setting up an AutoEntryScript. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Switch to new-style error-reporting for PrepareScriptEnvironment. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Defang PushJSContextNoScriptContext. v1

You can more or less rubber stamp this.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #1)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=862180c153ea

Looks green on linux64.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8605589 [details] [diff] [review]
Part 1 - Swap out the 'default JSContext callback' for something capable of setting up an AutoEntryScript. v1

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

Thanks for fixing this!

::: js/src/jsfriendapi.h
@@ +2579,3 @@
>  
>  JS_FRIEND_API(void)
> +SetPrepareScriptEnvironmentCallback(JSRuntime* rt, PrepareScriptEnvironmentCallback cb);

SpiderMonkey has been moving away from this style of API for some time now. We can do all these things with better type checking and better notation.

Please use the interface below instead. I think it will make the ArgClosure code a lot nicer.

/*
 * If the embedder has registered a ScriptEnvironmentPreparer,
 * PrepareScriptEnvironmentAndInvoke will call the preparer's 'invoke' method
 * with the given |closure|, with the assumption that the preparer will set up
 * any state necessary to run script in |scope|, invoke |closure| with a valid
 * JSContext*, and return.
 *
 * If no preparer is registered, PrepareScriptEnvironmentAndInvoke will assert
 * that |rt| has exactly one JSContext associated with it, enter the compartment
 * of |scope| on that context, and invoke |closure|.
 */

class ScriptEnvironmentPreparer {
    class Closure {
        virtual bool operator()(JSContext *cx) = 0;
    };

    virtual bool invoke(JS::HandleObject scope, Closure& closure) = 0;
};

extern JS_FRIEND_API(bool)
PrepareScriptEnvironmentAndInvoke(JSRuntime *rt, JS::HandleObject scope,
                                  ScriptEnvironmentPreparer::Closure& closure);

JS_FRIEND_API(void)
SetScriptEnvironmentPreparer(JSRuntime* rt, ScriptEnvironmentPreparer* preparer);

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Swap out the 'default JSContext callback' for something capable of setting up an AutoEntryScript. v2

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31621ac1ec45

Comment 9

[Jim Blandy :jimb]

Comment on attachment 8610202 [details] [diff] [review]
Part 1 - Swap out the 'default JSContext callback' for something capable of setting up an AutoEntryScript. v2

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

Looks great, thanks very much.

Just some stray "T* v" versus "T *v" issues (some perhaps from the code I provided).

::: js/src/jsfriendapi.cpp
@@ +1152,5 @@
>      return id == INTERNED_STRING_TO_JSID(nullptr, atom);
>  }
>  
> +JS_FRIEND_API(bool)
> +js::PrepareScriptEnvironmentAndInvoke(JSRuntime *rt, HandleObject scope, ScriptEnvironmentPreparer::Closure& closure)

JSRuntime*

@@ +1161,2 @@
>      MOZ_ASSERT(rt->contextList.getFirst() == rt->contextList.getLast());
> +    JSContext *cx = rt->contextList.getFirst();

"JSContext*"

@@ +1165,4 @@
>  }
>  
>  JS_FRIEND_API(void)
> +js::SetScriptEnvironmentPreparer(JSRuntime* rt, ScriptEnvironmentPreparer *preparer)

ScriptEnvironmentPreparer*

::: js/src/jsfriendapi.h
@@ +2574,4 @@
>  
> +struct ScriptEnvironmentPreparer {
> +    struct Closure {
> +        virtual bool operator()(JSContext *cx) = 0;

JSContext*

@@ +2580,5 @@
> +    virtual bool invoke(JS::HandleObject scope, Closure& closure) = 0;
> +};
> +
> +extern JS_FRIEND_API(bool)
> +PrepareScriptEnvironmentAndInvoke(JSRuntime *rt, JS::HandleObject scope,

"JSRuntime*"

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc42938aac6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/97cadd887176
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be9c1e42d9c

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc42938aac6d
https://hg.mozilla.org/mozilla-central/rev/97cadd887176
https://hg.mozilla.org/mozilla-central/rev/7be9c1e42d9c