If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[jsdbg2] Implement Debugger.Frame.prototype.pop

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: linclark, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox46 affected)

Details

(Reporter)

Description

2 years ago
There is a TODO in devtools/server/actors/script.js that depends on this:

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js#3208

Some questions:
- What do we need to do to make this work? Are there any remaining platform changes that need to happen?
- What client code would use this, and for what use case? Is any client code currently using it?

This TODO has been around for 4 years, so it is also possible that we don't need  this anymore. If we aren't going to implement this, can we remove the pop method that contains this TODO? When it is removed, none of the tests break.
(Reporter)

Comment 1

2 years ago
Needinfo-ing Jim because Eddy suggested that he'd be the best person to ask about this.
Flags: needinfo?(jimb)
Just a friendly heads up: things related to SpiderMonkey's Debugger API tend to go in Core: JavaScript.
Component: Developer Tools: Debugger → JavaScript Engine
Product: Firefox → Core
Summary: Implement Debugger.Frame.prototype.pop → [jsdbg2] Implement Debugger.Frame.prototype.pop
(In reply to Lin Clark from comment #0)
> - What do we need to do to make this work? Are there any remaining platform
> changes that need to happen?

The Debugger API is implemented in C++ inside of SpiderMonkey, so extending the API is itself platform work.

I'm not 100% sure if there is much other infra needed inside SM for this method, though. Possibly a good-first-bug for SM but this is really a part of SM that I'm not super familiar with.

> - What client code would use this, and for what use case? Is any client code
> currently using it?

Currently, no. The user-facing feature that would be built on top of this would be "force return 5 from this function instead of continuing to execute it and return whatever would otherwise be returned."

> This TODO has been around for 4 years, so it is also possible that we don't
> need  this anymore. If we aren't going to implement this, can we remove the
> pop method that contains this TODO? When it is removed, none of the tests
> break.

I think it would be good to remove this RDP method since it has always just returned "not yet implemented", but I do think this method is a logical and nice-to-have method for the Debugger API.
(Reporter)

Updated

2 years ago
See Also: → bug 1235901
(Reporter)

Updated

2 years ago
Flags: needinfo?(jimb)

Comment 4

2 years ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3)
> (In reply to Lin Clark from comment #0)
> > - What client code would use this, and for what use case? Is any client code
> > currently using it?
> 
> Currently, no. The user-facing feature that would be built on top of this
> would be "force return 5 from this function instead of continuing to execute
> it and return whatever would otherwise be returned."

This sounds like just returning a { return: 5 } completion value, not D.F.p.pop, though.

D.F.p.pop applied to an arbitrary frame sounds kind of impossible to implement. What happens to the C++ frames in between the youngest frame and the one being popped? Since the frame being popped is necessarily a parent of the frame calling D.F.p.pop, where is that call's continuation afterwards?

Unless I'm confused, D.F.p.pop was an impossibility the moment it was designed (by me *cough*), and completion values are the actual primitive to use to implement the same behavior.

Oh, hey. D.F.p.pop was deleted from the Debugger API spec! Yay!


> I think it would be good to remove this RDP method since it has always just
> returned "not yet implemented", but I do think this method is a logical and
> nice-to-have method for the Debugger API.

+1 to removing the RDP method.
(Reporter)

Comment 5

2 years ago
Since everyone +1-ed it, I removed the RDP method in bug 1235901.

From what Jim said, it sounds like we can close this issue out. Nick, Jim, is that right?
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
Yeah I think I was mistaken.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.