Last Comment Bug 755808 - Add more options to shell evaluate() function
: Add more options to shell evaluate() function
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on: 768313 788356
Blocks: 749697
  Show dependency treegraph
 
Reported: 2012-05-16 10:25 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-10-23 17:15 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (10.94 KB, patch)
2012-05-16 10:25 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-05-16 10:25:46 PDT
Created attachment 624437 [details] [diff] [review]
v1

This makes bug 749697 and bug 738468 testable in the shell.
Comment 1 Jason Orendorff [:jorendorff] 2012-05-16 10:43:08 PDT
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.
Comment 2 Jesse Ruderman 2012-05-16 12:26:01 PDT
Added to jsfunfuzz (3f10d6317203)
Comment 3 Jim Blandy :jimb 2012-05-17 13:54:52 PDT
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?
Comment 4 Jason Orendorff [:jorendorff] 2012-05-17 16:51:11 PDT
(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.
Comment 5 Jason Orendorff [:jorendorff] 2012-05-17 17:04:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/648093316d93
Comment 6 Nicholas Nethercote [:njn] 2012-05-17 19:30:31 PDT
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]
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-05-18 13:22:15 PDT
https://hg.mozilla.org/mozilla-central/rev/648093316d93
Comment 8 Jim Blandy :jimb 2012-05-18 15:54:53 PDT
*** Bug 710688 has been marked as a duplicate of this bug. ***
Comment 9 Jason Orendorff [:jorendorff] 2012-05-30 13:15:53 PDT
Regarding comment 6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/732355e3de2a
Comment 10 Gary Kwong [:gkw] [:nth10sd] 2012-05-30 13:19:39 PDT
(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?
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2012-05-30 13:26:17 PDT
> This will probably also fix bug 758841 as well?

(It pretty much will, so I have also resolved that bug.)
Comment 12 Ed Morley [:emorley] 2012-05-31 06:31:04 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.