Closed Bug 729369 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jruderman, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sg:want])

Attachments

(4 files, 4 obsolete files)

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
Whiteboard: [sg:want]
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).
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #599800 - Flags: review?(jwalden+bmo)
Attached patch support usage errors (obsolete) — Splinter Review
This patch makes it a bit easier to report usage errors that take advantage of the usage strings attached to functions.
Attachment #599801 - Flags: review?(jwalden+bmo)
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.
Attachment #599803 - Flags: review?(jwalden+bmo)
This patch exposes the shell functions to chrome via the following:
  Components.utils.getDebugObject().shellFunc()
Attachment #599804 - Flags: review?(jwalden+bmo)
The "move shell functions" patch needs to be merged through https://hg.mozilla.org/mozilla-central/rev/f00bab9999f9
This uses a special friendapi function rather that JS_DefineFunctions.
Attachment #599800 - Attachment is obsolete: true
Attachment #600570 - Flags: review?(jwalden+bmo)
Attachment #599800 - Flags: review?(jwalden+bmo)
This updates the old patch to use the new friendapi functions.
Attachment #599803 - Attachment is obsolete: true
Attachment #599803 - Flags: review?(jwalden+bmo)
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
Attachment #600570 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #599801 - Flags: review?(jwalden+bmo) → review-
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'.
Attachment #599804 - Flags: review?(jwalden+bmo) → review+
This version adds an assertion to prevent misuse.
Attachment #599801 - Attachment is obsolete: true
Attachment #601137 - Flags: review?(jwalden+bmo)
I somehow cleared the review flag on this patch. Anyway, here's a new version that uses the new TestingFunctions name.
Attachment #600571 - Attachment is obsolete: true
Attachment #601139 - Flags: review?(jwalden+bmo)
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.  :-)
Attachment #601137 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #601139 - Flags: review?(jwalden+bmo) → review+
I assume this was a coalesced patch
https://hg.mozilla.org/mozilla-central/rev/cfa346e78b0d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 736795
You need to log in before you can comment on or make changes to this bug.