Closed Bug 1164664 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: js-ctypes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
You can more or less rubber stamp this.
Attachment #8605591 - Flags: review?(jimb)
(In reply to Bobby Holley (:bholley) from comment #1)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=862180c153ea

Looks green on linux64.
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);
Attachment #8605589 - Flags: review?(jimb)
Attachment #8605590 - Flags: review?(jimb) → review+
Attachment #8605591 - Flags: review?(jimb) → review+
Attachment #8605589 - Flags: feedback+
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*"
Attachment #8610202 - Flags: review?(jimb) → review+
You need to log in before you can comment on or make changes to this bug.