Add more options to shell evaluate() function

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 624437 [details] [diff] [review]
v1

This makes bug 749697 and bug 738468 testable in the shell.
Attachment #624437 - Flags: review?(jimb)
(Assignee)

Comment 1

5 years ago
Before this patch, evaluate takes one string argument:
  evaluate("2+2");
With the patch, there is an optional second argument:
  evaluate("2+2", {fileName: "computeTwoPlusTwo.js",
                   lineNumber: 103,
                   newContext: true,
                   compileAndGo: false,
                   global: this});

This is new attack surface for the fuzzers. Not too juicy, one hopes, but you never know.
(Assignee)

Updated

5 years ago
Attachment #624437 - Attachment filename: bug-749697-shell-v1.patch → bug-755808-evaluate-options-v1.patch

Comment 2

5 years ago
Added to jsfunfuzz (3f10d6317203)
(Assignee)

Updated

5 years ago
Blocks: 749697

Comment 3

5 years ago
Comment on attachment 624437 [details] [diff] [review]
v1

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

Looks good to me, assuming the 'JS::Anchor' question is addressed.

::: js/src/shell/js.cpp
@@ +781,5 @@
> +
> +  public:
> +    AutoNewContext() : oldcx(NULL), newcx(NULL) {}
> +
> +    bool enter(JSContext *cx) {

Should we assert that there's no pending exception on cx?

@@ +818,5 @@
> +    jsval *argv = JS_ARGV(cx, vp);
> +
> +    if (argc < 1 || argc > 2 ||
> +        !JSVAL_IS_STRING(argv[0]) ||
> +        (argc == 2 && JSVAL_IS_PRIMITIVE(argv[1])))

nit: It might be nicer to just separate the arity and type checks; you're not commoning up too much by combining them.

@@ +891,5 @@
> +                if (!global)
> +                    return false;
> +            }
> +            if (!global ||
> +                (JS_GetClass(global)->flags & JSCLASS_IS_GLOBAL) != JSCLASS_IS_GLOBAL)

nit: I think it's more legible to just say !(x&y) here.

@@ +905,3 @@
>  
>      size_t codeLength;
>      const jschar *codeChars = JS_GetStringCharsAndLength(cx, code, &codeLength);

Do we need a JS::Anchor for 'code' before we do this?
Attachment #624437 - Flags: review?(jimb) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Jim Blandy :jimb from comment #3)
> Should we assert that there's no pending exception on cx?

Sure, done.

> nit: It might be nicer to just separate the arity and type checks; you're
> not commoning up too much by combining them.

Done.

> nit: I think it's more legible to just say !(x&y) here.

Indubitably. That was pre-existing code. Cleaned up.

> Do we need a JS::Anchor for 'code' before we do this?

I've changed it to a RootedVarString. But I don't think either one is really necessary; the string is rooted in argv.
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/648093316d93
GCC says the following, and I think it's right:

/home/njn/moz/mi1/js/src/shell/js.cpp: In function ‘JSBool Evaluate(JSContext*, unsigned int, jsval*)’:
/home/njn/moz/mi1/js/src/shell/js.cpp:912:5: warning: ‘newContext’ may be used uninitialised in this function [-Wuninitialized]
https://hg.mozilla.org/mozilla-central/rev/648093316d93
Assignee: general → jorendorff
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Updated

5 years ago
Duplicate of this bug: 710688
(Assignee)

Comment 9

5 years ago
Regarding comment 6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/732355e3de2a
(In reply to Jason Orendorff [:jorendorff] from comment #9)
> Regarding comment 6:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/732355e3de2a

This will probably also fix bug 758841 as well?
> This will probably also fix bug 758841 as well?

(It pretty much will, so I have also resolved that bug.)
(In reply to Jason Orendorff [:jorendorff] from comment #9)
> Regarding comment 6:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/732355e3de2a

https://hg.mozilla.org/mozilla-central/rev/732355e3de2a
Depends on: 768313
Depends on: 788356
Depends on: 797692
No longer depends on: 797692
You need to log in before you can comment on or make changes to this bug.