Open Bug 757185 Opened 12 years ago Updated 2 years ago

[jsdbg2] Debugger needs a way to inspect WeakMap, Map, and Set instances

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

People

(Reporter: jimb, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:t]) 

At the moment, there's no convenient way in the debugger to see the bindings in a WeakMap or Map, or the elements of a Set. The debugger should be able to see into these objects reliably, even if the methods on their prototypes have been modified.

WeakMap, by design, provides no way whatsoever to enumerate the bindings. However, the debugger should be able to do this anyway.
Whiteboard: [js:t]
Blocks: 862583
Assignee: general → nobody
Pagination of these collections items is going to be interesting. Can we rely on insertion order iteration for Map/Set?
(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> Pagination of these collections items is going to be interesting. Can we
> rely on insertion order iteration for Map/Set?

Ah while this would handle additions, deletions will still be funky.

Maybe we should just abandon the idea of pagination and just let the client specify a maximum number of items to receive.
Yes, Map and Set retain insertion order, per spec
In devtools/server/actors/object.js, there are [Weak]Map/Set previewers that are able to enumerate the entries. For Maps, it calls Map.prototype.entries on the raw object. For WeakMaps, it calls ThreadSafeChromeUtils.nondeterministicGetWeakMapKeys.

In patch for bug 1172920, I refactored this code to use it for inspecting the Map/Set contents in VariablesView.

Is this way to inspect the Map/Set the right one? Or does it have any shortcomings that should be addressed? Can this bug be closed as Fixed?
Flags: needinfo?(nfitzgerald)
Did that bug also address inspecting a WeakMap? If so, then this bug can close. If not, then we should leave this bug for WeakMap.
Flags: needinfo?(nfitzgerald) → needinfo?(jsnajdr)
All of these previewers work by calling unsafeDereference to exchange the Debugger.Object for an ordinary wrapper for the map/set, and then using direct-access functions like Map.prototype.entries or nondeterministicGetWeakMapKeys to do the enumeration.

None of these approaches work for Workers, because the "ordinary wrapper" in a worker is completely opaque: debugger compartments are not allowed to touch debuggee compartments at all, other than through the Debugger API.

Eddy Bruel has started working on extending the Debugger API to do these inspections directly. I'll find the bug number for that work...
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #5)
> Did that bug also address inspecting a WeakMap?

Yes, it uses ThreadSafeChromeUtils.nondeterministicGetWeakMapKeys for that.
Flags: needinfo?(jsnajdr)
The "[jsdbg2]" tag in the description of this bug means that it's about the Debugger API, not about the JS debugger user interface as part of devtools. Since Debugger per se does not yet have any methods for inspecting these other classes, and such methods are still valuable in some cases (like worker debugging), this bug should stay open.

If we decide that Debugger does not need to be able to inspect such types itself, then we could close this, but I haven't seen that discussion.
Ah I missed that this was a jsdbg2 bug in Core: JS Engine.

Yes, we should leave this open for implementing the Proper Way that skips raw debuggee objects and xrays.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #9)
> Yes, we should leave this open for implementing the Proper Way that skips
> raw debuggee objects and xrays.

Ok, then the remaining question is: should this be a blocker for bug 1172920? Is the raw+xrays way good-enough, or should we wait and implement this properly before landing? It won't work in workers, but the current console previewers don't work in them either...
Flags: needinfo?(nfitzgerald)
No, this should not block. We are already using raw objects and unwrapping xrays, and we should fix that, but we don't need to halt other forward progress in the meantime.
No longer blocks: 862583
Flags: needinfo?(nfitzgerald)
Debugger API desiderata include:
    an intuitive API (so users don't have to look every little thing up)
    a small implementation (maximum power for time investment)
    "X-ray-like" behavior everywhere (so debuggees can't lie to the debugger)
    an API so clean that another engine could implement it

Every way of exposing internal object state to users has its tradeoffs. The best way is to let users easily get hold of X-ray wrappers. This takes care of Map and Set completely. It also takes care of accessing private data for many other types, like Date. It achieves 1-3 at the cost of 4.

We would need a few extra Debugger.Object methods for accessing private state that isn't exposed to ordinary code, like iterating over WeakMap and WeakSet data.
I would add to that list:

  * Methods for introspection _without_ running debuggee code or mutating debuggee state.

Shelling out to xrays still has two drawbacks, as I see it:

1. We still have to implement the xray wrappers for various classes, which relatively few people know how to do (eg, we don't have xrays for map and set yet: bug 1155700). I think at this point there are more people familiar with the Debugger than the xray/xpconnect machinery.

2. Debugger wishes to avoid mutating any debuggee state or running any debuggee code, and warns (for now, but will throw) Debugger.DebuggeeWouldRun when this happens without explicitly invoking a Debugger method intended to let the user run debuggee code. Xrays' guarantee is a bit softer: that you won't unintentionally run "unknown" debuggee code, but "known, safe" methods are fine. And I think they provide no protections against arbitrary mutation, right? I'm not sure how big an impact this actually has in practice (are the "safe" methods always unobservable?), but it certainly seems much easier to accidentally modify debuggee state to me.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #11)
> No, this should not block. We are already using raw objects and unwrapping
> xrays, and we should fix that, but we don't need to halt other forward
> progress in the meantime.

My thoughts exactly.
Note that the xray approach isn't available on workers, where all debugger compartment -> debuggee compartment wrappers are completely opaque.

Perhaps we should revisit the decision to make them completely opaque? But then I wonder whether it's better to implement xrays, or just extend Debugger.
I vote revisit. If we can solve the problem by simplifying and unifying, that seems better than solving it by adding more features and more code.
Well, "unifying" in this case means "bring the XPCOM wrapper logic into workers." That is a path whose consequences I can't anticipate.
Bobby, can we have X-ray wrappers on worker threads? How much work would it be?
Flags: needinfo?(bobbyholley)
(In reply to Jason Orendorff [:jorendorff] from comment #18)
> Bobby, can we have X-ray wrappers on worker threads? How much work would it
> be?

It would be a fair amount of work, not insurmountable - probably somewhat akin to the work of adding cycle collection to workers. The bigger problem is that there are few people qualified to do it, and none of us are likely to sign up for this project.

We just finished a bunch of back-and-forth in bug 1246091 comment 19 (and nearby) about this. It seems to me like there's a lot of value in keeping the debugger API orthogonal to Xrays, since they have very different consumers.
Flags: needinfo?(bobbyholley)
If it were possible to just carry over a wrapper implementation from the main thread, then that would be one thing. But I think the choice actually before is is between fresh engineering in the cross-compartment wrappers, or fresh engineering in Debugger. I think the latter is easier to write and easier to test.
Not a blocker, but if we implemented bug 1257665, then that might make this implementation a little less painful.
Thanks, Bobby.

OK, I'm convinced comment 20 is right.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.