Last Comment Bug 732653 - Remove JS_FrameIterator
: Remove JS_FrameIterator
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on: 744617
Blocks: IonMonkey LandIon 759107
  Show dependency treegraph
 
Reported: 2012-03-02 19:13 PST by David Anderson [:dvander]
Modified: 2012-08-16 01:35 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
some kind of new api (71 bytes, patch)
2012-03-05 19:29 PST, David Anderson [:dvander]
no flags Details | Diff | Review
remove JS_FrameIterator use from XPCStack (7.90 KB, patch)
2012-04-12 19:25 PDT, David Anderson [:dvander]
luke: review+
mrbkap: review+
Details | Diff | Review
move FormatJSStack to js/src (19.07 KB, patch)
2012-04-13 15:47 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
move FormatJSStack to js/src (19.07 KB, patch)
2012-04-13 17:24 PDT, David Anderson [:dvander]
bobbyholley: review+
luke: review+
Details | Diff | Review
"remove" the API (12.35 KB, patch)
2012-04-13 17:28 PDT, David Anderson [:dvander]
luke: review+
Details | Diff | Review

Description David Anderson [:dvander] 2012-03-02 19:13:57 PST
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] 2012-03-05 19:29:01 PST
Created attachment 603141 [details] [diff] [review]
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] 2012-03-05 19:33:20 PST
I think you meant to qref before attaching.
Comment 3 David Anderson [:dvander] 2012-04-12 19:25:52 PDT
Created attachment 614659 [details] [diff] [review]
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] 2012-04-13 15:47:04 PDT
Created attachment 614938 [details] [diff] [review]
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] 2012-04-13 17:23:21 PDT
Comment on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

Luke for JS changes
Comment 6 David Anderson [:dvander] 2012-04-13 17:23:41 PDT
Comment on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

Bobby for XPC changes
Comment 7 David Anderson [:dvander] 2012-04-13 17:24:47 PDT
Created attachment 614964 [details] [diff] [review]
move FormatJSStack to js/src
Comment 8 David Anderson [:dvander] 2012-04-13 17:28:05 PDT
Created attachment 614969 [details] [diff] [review]
"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] 2012-04-13 17:40:29 PDT
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 (busy) 2012-04-13 18:25:59 PDT
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 (busy) 2012-04-16 11:52:33 PDT
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) (in and out until 7-14) 2012-04-18 02:37:29 PDT
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] 2012-06-07 09:30:20 PDT
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 (busy) 2012-06-07 09:38:18 PDT
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] 2012-06-07 10:15:55 PDT
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] 2012-08-15 17:30:26 PDT
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 2012-08-16 01:35:18 PDT
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?

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