732653 - Remove JS_FrameIterator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

This API call is incompatible with IonMonkey, which does not create JSStackFrames. Most uses of this in the browser just need basic information about the topmost frame. For more special uses (like in caps), we can preserve the API (it'll be renamed to something like JS_BrokenFrameIterator) - it'll still work as intended, since Ion stack traces won't cross globals - but the actual iteration will be eliding optimized frames.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: some kind of new api

This is a somewhat ugly but safer C API for iterating and querying stack frames. XPCDebug and jsd's uses of JS_FrameIterator can go away with this, I think.

Comment 2

[Josh Matthews [:jdm]]

I think you meant to qref before attaching.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove JS_FrameIterator use from XPCStack

This patch introduces a new API, that returns an array of stack frame descriptions. This covers exactly what XPCStack needs.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: move FormatJSStack to js/src

This patch removes a bunch of complex frame snooping code from js/xpconnect/src to js/src. The functionality should be identical, I just cleaned it up.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

Luke for JS changes

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

Bobby for XPC changes

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: move FormatJSStack to js/src

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: "remove" the API

These remaining uses are either going away from CPG or are guaranteed not to run with live IonMonkey code.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 614964 [details] [diff] [review]
move FormatJSStack to js/src

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

::: js/src/jsdbgapi.h
@@ +105,5 @@
>  
> +extern JS_PUBLIC_API(char *)
> +FormatStackDump(JSContext *cx, char *buf,
> +                    JSBool showArgs, JSBool showLocals,
> +                    JSBool showThisProps);

Can you align the parameters?  Also I think you can remove 'buf'.

Comment 10

[Bobby Holley (:bholley)]

Can we assert against IM somehow in JS_BrokenFrameIterator? The caps fixup has always been a 'when we have time', sort of thing. So if we have a more serious deadline, we need to make the dependency very explicit (and possibly get someone in the loop whose job it is to keep track of these things).

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

I can't really get my head around the CreateStack rewrite, mostly because I've never looked at this code before and the diff isn't very comprehensible.

Bouncing to Blake to see if he wants to do it. I'd also be fine with luke reviewing this part.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

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

::: js/xpconnect/src/XPCStack.cpp
@@ +134,2 @@
>  
> +    JS::StackDescription* desc = JS::DescribeStack(cx, MAX_FRAMES);

Is it worth exposing a RAII helper to auto-destroy this as well?

@@ +138,2 @@
>  
> +    for (unsigned i = 0; i < desc->nframes && self; i++) {

I'd prefer to use a more explicitly-typed integer type here.

Comment 13

[Kannan Vijayan [:djvj]]

Is there an ETA for this landing?  It seems like it's been a month or so with no movement.  I'm trying to get rid of js_GetTopStackFrame uses from the code for Ion, and JS_FrameIterator seems to be the cause of basically every orange on TBPL that I'm getting.

Comment 14

[Bobby Holley (:bholley)]

Well, yes. You might want to follow bug 757046. I also just landed bug 754202, which was part of the battle.

What's the current ETA for IM?

Comment 15

[Kannan Vijayan [:djvj]]

End of June.  I'll watch this for movement and take care of more pressing fuzz bugs in the meantime.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Rebased and rolled up into one patch:

https://hg.mozilla.org/projects/ionmonkey/rev/9df5b5265826

Luckily, it looks like a lot of uses went away on their own as post-CPG work. Cool!

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 614969 [details] [diff] [review]
"remove" the API

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

::: js/src/jsdbgapi.h
@@ +251,5 @@
>  
>  /*
> + * This function does not work when IonMonkey is active. It remains for legacy
> + * code: caps/principal clamping, which will be removed shortly after
> + * compartment-per-global, and jsd, which can only be used when IonMonkey is

Heh, how shortly?