Closed Bug 732653 Opened 8 years ago Closed 7 years ago

Remove JS_FrameIterator

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached patch some kind of new api (obsolete) — Splinter Review
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.
Assignee: general → dvander
Status: NEW → ASSIGNED
I think you meant to qref before attaching.
This patch introduces a new API, that returns an array of stack frame descriptions. This covers exactly what XPCStack needs.
Attachment #603141 - Attachment is obsolete: true
Attached patch move FormatJSStack to js/src (obsolete) — Splinter Review
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 on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

Luke for JS changes
Attachment #614659 - Flags: review?(luke)
Comment on attachment 614659 [details] [diff] [review]
remove JS_FrameIterator use from XPCStack

Bobby for XPC changes
Attachment #614659 - Flags: review?(bobbyholley+bmo)
Attachment #614938 - Attachment is obsolete: true
Attachment #614964 - Flags: review?(bobbyholley+bmo)
Attachment #614964 - Attachment description: patch → move FormatJSStack to js/src
Attached patch "remove" the APISplinter Review
These remaining uses are either going away from CPG or are guaranteed not to run with live IonMonkey code.
Attachment #614969 - Flags: review?(luke)
Attachment #614659 - Flags: review?(luke) → review+
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'.
Attachment #614964 - Flags: review?(luke) → review+
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).
Attachment #614964 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #614659 - Flags: review?(bobbyholley+bmo) → review?(mrbkap)
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.
Attachment #614659 - Flags: review?(mrbkap) → review+
Attachment #614969 - Flags: review?(luke) → review+
Blocks: 759107
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.
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?
End of June.  I'll watch this for movement and take care of more pressing fuzz bugs in the meantime.
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!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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?
You need to log in before you can comment on or make changes to this bug.