729369 - Expose the same set of SpiderMonkey testing APIs to debug shell and debug browser chrome

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jesse Ruderman]

The browser is always lagging behind the shell in access to testing APIs. This makes fuzzers sad.

There should be a shared implementation (i.e. not in js/shell/) and an object for all these things to hang off of.

Examples of things that are only in the shell:
  gcslice
  verifybarriers
  terminate
  mjitChunkLimit
  schedulegc [in N allocations]
  enableStackWalkingAssertion(?)

Examples of things that are only in the browser:
  forceShrinkingGC
  schedulePreciseGC [on the event loop, when no JS is running]
  schedulePreciseShrinkingGC

In both, but with unnecessarily different APIs:
  gczeal

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: store usage/help as part of function object

This patch makes it easy to attach usage and help messages to functions defined via JS_DefineFunctions. It also changes the help() function in the shell to display those help messages (by iterating over the global to find functions with these messages).

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: support usage errors

This patch makes it a bit easier to report usage errors that take advantage of the usage strings attached to functions.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move shell functions and update help messages

This patch updates all the help messages. It also moves a number of shell functions into the JS engine, in the file builtin/ShellFunctions.cpp. I didn't really change anything about these functions besides the "wrong number of args" errors, which used some code from js.cpp. I also took out the call to sbrk in gc(), since I don't think anyone uses it and it seems pointless.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: expose the shell functions to the browser

This patch exposes the shell functions to chrome via the following:
  Components.utils.getDebugObject().shellFunc()

Comment 5

[Jesse Ruderman]

The "move shell functions" patch needs to be merged through https://hg.mozilla.org/mozilla-central/rev/f00bab9999f9

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: store usage/help as part of function object, v2

This uses a special friendapi function rather that JS_DefineFunctions.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move shell functions and update help messages, v2

This updates the old patch to use the new friendapi functions.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 600570 [details] [diff] [review]
store usage/help as part of function object, v2

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

::: js/src/jsfriendapi.cpp
@@ +189,5 @@
> +    jsval v = STRING_TO_JSVAL(atom);
> +    if (!JS_DefineProperty(cx, obj, prop, v,
> +                           JS_PropertyStub, JS_StrictPropertyStub,
> +                           JSPROP_READONLY | JSPROP_PERMANENT))
> +        return false;

Just return JS_DefineProperty(...).

@@ +195,5 @@
> +    return true;
> +}
> +
> +JS_FRIEND_API(bool)
> +JS_DefineFunctionsWithHelp(JSContext *cx, JSObject *obj, JSFunctionSpecWithHelp *fs)

const JSFSWH*

@@ +204,5 @@
> +
> +    CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, obj);
> +    for (; fs->name; fs++) {
> +        uintN flags = fs->flags;

Move this down closer to first use.

::: js/src/shell/js.cpp
@@ +4314,5 @@
> +    JSString *str = JSVAL_TO_STRING(v);
> +    JS::Anchor<JSString *> a_str(str);
> +    const jschar *chars = JS_GetStringCharsZ(cx, str);
> +    if (!chars)
> +        return JS_FALSE;

false

@@ +4317,5 @@
> +    if (!chars)
> +        return JS_FALSE;
> +
> +    for (const jschar *p = chars; *p; p++)
> +        fprintf(gOutFile, "%c", *p);

|char(*p)|

@@ +4321,5 @@
> +        fprintf(gOutFile, "%c", *p);
> +
> +    fprintf(gOutFile, "\n");
> +
> +    return JS_TRUE;

true

@@ +4329,5 @@
> +PrintHelp(JSContext *cx, JSObject *obj)
> +{
> +    jsval usage, help;
> +    if (!JS_LookupProperty(cx, obj, "usage", &usage))
> +        return JS_FALSE;

false

@@ +4331,5 @@
> +    jsval usage, help;
> +    if (!JS_LookupProperty(cx, obj, "usage", &usage))
> +        return JS_FALSE;
> +    if (!JS_LookupProperty(cx, obj, "help", &help))
> +        return JS_FALSE;

false

@@ +4334,5 @@
> +    if (!JS_LookupProperty(cx, obj, "help", &help))
> +        return JS_FALSE;
> +
> +    if (JSVAL_IS_VOID(usage) || JSVAL_IS_VOID(help))
> +        return JS_TRUE;

true

@@ +4337,5 @@
> +    if (JSVAL_IS_VOID(usage) || JSVAL_IS_VOID(help))
> +        return JS_TRUE;
> +
> +    if (!PrintHelpString(cx, usage) || !PrintHelpString(cx, help))
> +        return JS_FALSE;

Just return PrintHelpString(...) && PrintHelpString(...)

@@ +4351,5 @@
>      if (argc == 0) {
> +        JSObject *global = JS_GetGlobalObject(cx);
> +        AutoIdArray ida(cx, JS_Enumerate(cx, global));
> +        if (!ida)
> +            return JS_FALSE;

false

@@ +4356,5 @@
> +
> +        for (size_t i = 0; i < ida.length(); i++) {
> +            jsval v;
> +            if (!JS_LookupPropertyById(cx, global, ida[i], &v))
> +                return JS_FALSE;

false

@@ +4358,5 @@
> +            jsval v;
> +            if (!JS_LookupPropertyById(cx, global, ida[i], &v))
> +                return JS_FALSE;
> +            if (JSVAL_IS_OBJECT(v) && !PrintHelp(cx, JSVAL_TO_OBJECT(v)))
> +                return JS_FALSE;

false

@@ +4365,4 @@
>          jsval *argv = JS_ARGV(cx, vp);
> +        for (uintN i = 0; i < argc; i++) {
> +            if (JSVAL_IS_OBJECT(argv[i]) && !PrintHelp(cx, JSVAL_TO_OBJECT(argv[i])))
> +                return JS_FALSE;

false

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 599801 [details] [diff] [review]
support usage errors

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

I don't think this is an API we want, at least not one that's available to arbitrary code.  The problem is that it doesn't have consistent behavior of always throwing a particular kind of error.  Specifically, if the object passed in has properties that are getters, those getters can throw any arbitrary value they want -- shades of bug 633623.

This behavior looks like it would be safe inlined into a particular caller with extra knowledge of its calling context, with a comment specifically calling out its use as depending on intrinsics of the object passed in.  I think you can move it into the shell functions patch, into the particular file that would use this, and be safe.  Please do that.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 599804 [details] [diff] [review]
expose the shell functions to the browser

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

::: js/src/jsfriendapi.h
@@ +804,5 @@
>      operator JSObject *() const { return value; }
>  };
>  
> +extern JS_FRIEND_API(JSObject *)
> +GetDebugObject(JSContext *cx);

I think the jsdbg2 people will have something to say about this name.  Come up with a different one, and probably propagate it into the Components.utils method name.  Feel free to moot it in #jsapi, I got nothin'.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: support usage errors, v2

This version adds an assertion to prevent misuse.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move shell functions and update help messages, v3

I somehow cleared the review flag on this patch. Anyway, here's a new version that uses the new TestingFunctions name.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 601137 [details] [diff] [review]
support usage errors, v2

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

::: js/src/jscntxt.cpp
@@ +523,5 @@
>      return warning;
>  }
>  
> +/* |callee| requires a usage string provided by JS_DefineFunctionsWithHelp. */
> +JSBool

Any reason this can't return a bool?  And can't it be unprefixed in namespace js, like most new extern'd functions are?

@@ +528,5 @@
> +js_ReportUsageError(JSContext *cx, JSObject *callee, const char *msg)
> +{
> +    const char *usageStr = "usage";
> +    JSAtom *usageAtom = js_Atomize(cx, usageStr, strlen(usageStr));
> +    JS_ASSERT(!callee->nativeLookup(cx, ATOM_TO_JSID(usageAtom))->configurable());

Also assert that it's not writable, too, and that it hasDefaultGetter(), i.e. that it's a plain old data property and neither accessor-ish in the function sense or accessor-ish in the JSPropertyOp sense.

@@ +532,5 @@
> +    JS_ASSERT(!callee->nativeLookup(cx, ATOM_TO_JSID(usageAtom))->configurable());
> +
> +    jsval usage;
> +    if (!JS_LookupProperty(cx, callee, "usage", &usage))
> +        return JS_FALSE;

false

@@ +545,5 @@
> +            return JS_FALSE;
> +        JS_ReportError(cx, "%s. Usage: %hs", msg, chars);
> +    }
> +
> +    return JS_TRUE;

Erm, false?  You reported an error.  :-)

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 601139 [details] [diff] [review]
move shell functions and update help messages, v3

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

rs=me assuming this was basically copy-paste.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa346e78b0d

Comment 16

[Marco Bonardo [:mak]]

I assume this was a coalesced patch
https://hg.mozilla.org/mozilla-central/rev/cfa346e78b0d