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

RESOLVED FIXED in Firefox 41

Status

()

Core
js-ctypes
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

(Blocks: 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=862180c153ea
Created attachment 8605589 [details] [diff] [review]
Part 1 - Swap out the 'default JSContext callback' for something capable of setting up an AutoEntryScript. v1
Attachment #8605589 - Flags: review?(jimb)
Created attachment 8605590 [details] [diff] [review]
Part 2 - Switch to new-style error-reporting for PrepareScriptEnvironment. v1
Attachment #8605590 - Flags: review?(jimb)
Created attachment 8605591 [details] [diff] [review]
Part 3 - Defang PushJSContextNoScriptContext. v1

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 6

3 years ago
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);

Updated

3 years ago
Attachment #8605589 - Flags: review?(jimb)

Updated

3 years ago
Attachment #8605590 - Flags: review?(jimb) → review+

Updated

3 years ago
Attachment #8605591 - Flags: review?(jimb) → review+

Updated

3 years ago
Attachment #8605589 - Flags: feedback+
Created attachment 8610202 [details] [diff] [review]
Part 1 - Swap out the 'default JSContext callback' for something capable of setting up an AutoEntryScript. v2
Attachment #8605589 - Attachment is obsolete: true
Attachment #8610202 - Flags: review?(jimb)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31621ac1ec45

Comment 9

3 years ago
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+

Comment 10

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/dc42938aac6d
https://hg.mozilla.org/mozilla-central/rev/97cadd887176
https://hg.mozilla.org/mozilla-central/rev/7be9c1e42d9c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.