The default bug view has changed. See this FAQ.

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: general → dvander
Status: NEW → ASSIGNED

Comment 2

5 years ago
I think you meant to qref before attaching.
(Assignee)

Updated

5 years ago
Depends on: 744617
(Assignee)

Comment 3

5 years ago
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.
Attachment #603141 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

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

Luke for JS changes
Attachment #614659 - Flags: review?(luke)
(Assignee)

Comment 6

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

Bobby for XPC changes
Attachment #614659 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 7

5 years ago
Created attachment 614964 [details] [diff] [review]
move FormatJSStack to js/src
Attachment #614938 - Attachment is obsolete: true
Attachment #614964 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
Attachment #614964 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #614964 - Attachment description: patch → move FormatJSStack to js/src
(Assignee)

Comment 8

5 years ago
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.
Attachment #614969 - Flags: review?(luke)

Updated

5 years ago
Attachment #614659 - Flags: review?(luke) → review+

Comment 9

5 years ago
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+

Updated

5 years ago
Attachment #614969 - Flags: review?(luke) → review+

Updated

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 745386
(Assignee)

Comment 16

5 years ago
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
Last Resolved: 5 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.