1031152 - SpiderMonkey needs a public API for working with SavedFrame instances

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

At the moment, it's very clumsy to work with SavedFrame instances in the embedding. For example, to fetch the callee's display name:

     ThreadsafeAutoJSContext cx;
     JS::Rooted<JSObject*> stack(cx, mStack);
     JS::ExposeObjectToActiveJS(mStack);
     JSAutoCompartment ac(cx, mStack);
     JS::Rooted<JS::Value> nameVal(cx);
     if (!JS_GetProperty(cx, mStack, "functionDisplayName", &nameVal) ||
         !nameVal.isString()) {

From IRC: "<bz> All I have to say is: that sucks. ;)"

Since we would like SavedFrame to be the embeddings' preferred representation for saved stacks, we ought to give it a decent API.

Comment 1

[Boris Zbarsky [:bzbarsky]]

>     if (!JS_GetProperty(cx, mStack, "functionDisplayName", &nameVal) ||

That needs to be      

  if (!JS_GetProperty(cx, stack, "functionDisplayName", &nameVal) ||

hence the need for the Rooted, since JS_GetProperty needs a Handle and you can't get one from a Heap.  Point being, having to deal with JS_GetProperty here is a PITA.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Also, the parts of the API I care about are the property getters and the stringifier.  Invoking the latter ends up looking like this, in its entirety:

    ThreadsafeAutoJSContext cx;
    JS::ExposeObjectToActiveJS(mStack);
    JSAutoCompartment ac(cx, mStack);
    JS::Rooted<JS::Value> stack(cx, JS::ObjectValue(*mStack));
    JS::Rooted<JSString*> formattedStack(cx, JS::ToString(cx, stack));
    if (!formattedStack) {
      return NS_ERROR_UNEXPECTED;
    }
    nsDependentJSString str;
    if (!str.init(cx, formattedStack)) {
      return NS_ERROR_OUT_OF_MEMORY;
    }
    mFormattedStack = str;

For my purposes here, simply having a way to get the jschar* or some other representation I can infallibly get into a flat buffer would be nice.

Comment 3

[Boris Zbarsky [:bzbarsky]]

When this is fixed, we should adjust the code I added for bug 857648 accordingly.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Define a JS public API for working with SavedFrame instances

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8560111 [details] [diff] [review]
Define a JS public API for working with SavedFrame instances

Should probably document that CaptureCurrentStack will set stackp to null if there is no stack.

>+ * Each of these functions will find the first SavedFrame object in the chain
>+ * whose principals are subsumed by the cx's current compartment's principals,

The principals of the SavedFrame object itself are, or the principals of the underlying stack frame are?  This is an important distinction...

>+ * about priveleged frames to un-priveleged callers. However, it may be the 

"privileged".

>+                  uint32_t &linep);

I'd somewhat prefer a pointer for the outparam, fwiw.

>+                    MutableHandleValue columnp);

Why not a uint32_t* or uint32_t& for columnp?  Or more to the point, why does this not match the implementation?

>+ * Given a SavedFrame JSObject, get its functionDisplayName property. Defaults
>+ * to nullptr.

Why the difference between this and source, which defaults to empty string?

>+    RootedObject savedFrameObj(cx, CheckedUnwrap(obj));

OK, so this might work, but it's entirely non-obvious and needs way more API documentation to make sure it's not just working by accident.

As a reminder, I need some way of addressing the situation described in bug 1122238 comment 5.

What I need, and what was described in bug 1122238 comment 9, is an API that lets me pass in the principal I want to use for the security checks against the frame (not the SavedFrame object, the actual stackframe) principals, independent of what compartment the SavedFrame object is in.

As far as I see, there are three ways of doing this:

1)  Pass in the principal to use for the security checks independently of the cx and savedFrame arguments (in some form that we can discuss).  The implementation would then need to use that principal for the security checks on the stack frames.

2)  Pass in the principal as the principal of the current compartment of cx  and pass in the SavedFrame in whatever compartment it's in (which may not match the compartment of cx).  In that case, the implementation would need to save the principal of the current compartment of cx if it wants to enter the SavedFrame compartment, then do stuff using the saved-off principal.

3)  Pass in the principal as the principal of the current compartment of cx, and pass in the SavedFrame wrapped into the same compartment as cx.  Then the implementation needs to save the principal of the current compartment of cx, do a _UncheckedUnwrap_ (because the security wrapper that the caller had to create to stick the SavedFrame in the compartment of cx might be opaque), then enter the compartment of the resulting object if it wants, etc.

This patch in effect implements #2, right?  In partuclar, the API methods don't assert anything about same-compartmentness, and GetFirstSubsumedFrame works fine if cx and "frame" are not same-compartment.  If this is in fact on purpose and the intended API, then the API documentation should clearly spell this out, since passing a cx and an object that are not same-compartment is generally a no-no with jsapi functions, and GetFirstSubsumedFrame could also probably use comments about the arguments not necessarily being same-compartment.

Also, the upshot of the implementation here is that the return values of GetSavedFrameSource and GetSavedFrameFunctionDisplayName need not be in the zone of cx, and the return value of GetSavedFrameParent need not be in the compartment of cx.  That needs to be documented in the API.  Note that just wrapping into the compartment of cx is in fact not desired for GetSavedFrameParent because the result would not longer be usable with API strategy #2 above.  It'd be OK to wrap before returning if we were doing API strategy #3.

On the other hand, the return value of StringifySavedFrameStack is put in the compartment of cx.  This should also be documented.

>+GetSavedFrameSource(JSContext *cx, HandleObject savedFrame, MutableHandleString sourcep)

The API documentation says on SavedFrameAccessDenied sourcep is set to empty string, but the implementation doesn't do that.  In fact, it leaves sourcep untouched in that case.

>+GetSavedFrameFunctionDisplayName(JSContext *cx, HandleObject savedFrame, MutableHandleString namep)

Do you still need the "name" temporary?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8560111 [details] [diff] [review]
Define a JS public API for working with SavedFrame instances

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

Looks good, but I'd like to see this with the comments below (and from bz) addressed.

::: js/src/jsapi.h
@@ +5459,5 @@
> + */
> +
> +enum SavedFrameResult {
> +    SavedFrameSuccess,
> +    SavedFrameAccessDenied

Please make this an enum class:

enum class SavedFrameResult {
    Success,
    AccessDenied
};

And use SavedFrameResult::Success instead of SavedFrameSuccess. It's a bit longer, but enum classes have better type safety and all our compilers support them now.

@@ +5475,5 @@
> + */
> +extern JS_PUBLIC_API(SavedFrameResult)
> +GetSavedFrameLine(JSContext *cx,
> +                  HandleObject savedFrame,
> +                  uint32_t &linep);

We use pointers for outparams, so uint32_t *linep. It makes it clearer at the callsite that the value will be modified.

Also, while you're here I think these arguments fit on one line (same below). If not, the usual style is to add as many arguments as possible on a single line, as long as it fits within 99 chars.

@@ +5483,5 @@
> + */
> +extern JS_PUBLIC_API(SavedFrameResult)
> +GetSavedFrameColumn(JSContext *cx,
> +                    HandleObject savedFrame,
> +                    MutableHandleValue columnp);

This returns MutableHandleValue but the definition in SavedStacks.cpp has uint32_t &. Please change them both to uint32_t *.

::: js/src/vm/SavedStacks.cpp
@@ +385,5 @@
>  {
> +    if (!obj)
> +        return nullptr;
> +    RootedObject savedFrameObj(cx, CheckedUnwrap(obj));
> +    MOZ_ASSERT(savedFrameObj);

I'm not familiar with CheckedUnwrap. Please request review (in addition to feedback) from bz or bholley for this part specifically.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Nick, do you have an approximate ETA for when this might happen?  Would love to remove the bug 1122238 hackery...

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Nick, do you have an approximate ETA for when this might happen?  Would love
> to remove the bug 1122238 hackery...

I can bump it up my priority list. Next week or two?

Comment 9

[Boris Zbarsky [:bzbarsky]]

That sounds just fine.  There's no huge rush; just want to make sure it doesn't totally fall through the cracks.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> >+ * Given a SavedFrame JSObject, get its functionDisplayName property. Defaults
> >+ * to nullptr.
> 
> Why the difference between this and source, which defaults to empty string?

The source of a frame is always guaranteed to exist, while the display name is not. Put another way, you can have access to a SavedFrame, and still get a null function display name because SpiderMonkey wasn't able to infer a name for whatever reason.

> 2)  Pass in the principal as the principal of the current compartment of cx 
> and pass in the SavedFrame in whatever compartment it's in (which may not
> match the compartment of cx).  In that case, the implementation would need
> to save the principal of the current compartment of cx if it wants to enter
> the SavedFrame compartment, then do stuff using the saved-off principal.

Yes, this is what is implemented here. Will expand documentation / fix up the problems you described.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Define a JS public API for working with SavedFrame instances

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8566847 [details] [diff] [review]
Define a JS public API for working with SavedFrame instances

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

Looks good, r=me with comments below addressed.

::: js/src/vm/SavedStacks.cpp
@@ +395,5 @@
>  {
> +    if (!obj)
> +        return nullptr;
> +    RootedObject savedFrameObj(cx, CheckedUnwrap(obj));
> +    MOZ_ASSERT(savedFrameObj);

Leaving this part to bz.

@@ +419,5 @@
>  {
> +    MOZ_ASSERT(linep);
> +    RootedSavedFrame frame(cx, UnwrapSavedFrame(cx, savedFrame));
> +    if (!frame) {
> +        linep = 0;

*linep = 0;

@@ +432,5 @@
>  {
> +    MOZ_ASSERT(columnp);
> +    RootedSavedFrame frame(cx, UnwrapSavedFrame(cx, savedFrame));
> +    if (!frame) {
> +        columnp = 0;

*columnp = 0;

I wonder if we should add a jsapi-test...

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> I wonder if we should add a jsapi-test...

The main path is well tested by the saved-stacks jit-tests, but there is no coverage for the JSAPI level default values. I'll add one.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Define a JS public API for working with SavedFrame instances

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Define a JSAPI test for the SavedFrame public API

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8568002 [details] [diff] [review]
Part 2: Define a JSAPI test for the SavedFrame public API

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

Looks good, thanks for adding the test.

::: js/src/jsapi-tests/testSavedStacks.cpp
@@ +33,5 @@
> +    CHECK(result == JS::SavedFrameResult::AccessDenied);
> +    CHECK(str.get() == cx->runtime()->emptyString);
> +
> +    // Line
> +    uint32_t line;

Uber nit: maybe |uint32_t line = 123;| or something, to test we really set line to 0 and it doesn't accidentally have an (uninitialized) value of 0.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Define a JSAPI test for the SavedFrame public API

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8568000 [details] [diff] [review]
Define a JS public API for working with SavedFrame instances

>+ * argument is non-null, its JSClass is the SavedFrame class (or it is a wrapper

"cross-compartment or Xray wrapper"?

>+ * around an object with the SavedFrame class) and is not the

"and the object is not the ..."

r=me.  Thank you for doing this!

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Define a JS public API for working with SavedFrame instances

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Define a JSAPI test for the SavedFrame public API

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2781f14d9583

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Define a JS public API for working with SavedFrame instances

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Fixed the namespacing errors.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edeb45d36eb5

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Seems like some conflicts with X11 on linux:

14:02:19 INFO - ../dist/include/jsapi.h:5134:5: error: expected identifier
14:02:19 INFO - Success,
14:02:19 INFO - ^
14:02:19 INFO - /usr/include/X11/X.h:350:21: note: expanded from macro 'Success'
14:02:19 INFO - #define Success 0 /* everything's okay */ 

Rename to "Ok" instead?

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Define a JS public API for working with SavedFrame instances

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

ni? me for when try re-opens.

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ca22201b173

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Define a JS public API for working with SavedFrame instances

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

That should fix the SM(arm) build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eef29c61b5b7

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2d062821f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9faafa3cd45

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea2d062821f1
https://hg.mozilla.org/mozilla-central/rev/f9faafa3cd45