Implement a C++ interface for Debugger.Frame instances.

RESOLVED INACTIVE

Status

()

defect
P3
normal
RESOLVED INACTIVE
3 years ago
6 months ago

People

(Reporter: ejpbruel, Assigned: jimb)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(24 attachments, 18 obsolete attachments)

15.00 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
7.52 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
5.51 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
7.42 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
4.31 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
5.17 KB, patch
jimb
: review+
Details | Diff | Splinter Review
8.63 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
5.08 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
11.81 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
5.66 KB, patch
jimb
: review+
Details | Diff | Splinter Review
7.80 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
70.05 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
7.38 KB, patch
jimb
: review+
Details | Diff | Splinter Review
9.08 KB, patch
jimb
: review+
Details | Diff | Splinter Review
11.52 KB, patch
jimb
: review+
Details | Diff | Splinter Review
13.96 KB, patch
jimb
: review+
Details | Diff | Splinter Review
9.47 KB, text/plain
Details
8.49 KB, patch
jimb
: review+
Details | Diff | Splinter Review
6.64 KB, patch
jimb
: review+
Details | Diff | Splinter Review
19.09 KB, patch
jimb
: review+
Details | Diff | Splinter Review
14.77 KB, patch
jimb
: review+
Details | Diff | Splinter Review
8.90 KB, patch
jimb
: review+
Details | Diff | Splinter Review
9.17 KB, patch
jimb
: review+
Details | Diff | Splinter Review
16.42 KB, patch
jimb
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
This bug is part of the effort to implement a C++ interface for the debugger API. See the parent bug 1271641 for more context.
Blocks: 1263289
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee

Updated

3 years ago
No longer blocks: 1263289
Blocks: 1263289
Assignee: nobody → ejpbruel
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Priority: P2 → P1
Iteration: 50.1 → 50.2
Reporter

Comment 1

3 years ago
Attachment #8766401 - Flags: review?(nfitzgerald)
Comment on attachment 8766401 [details] [diff] [review]
Implement a DebuggerFrame class.

Review of attachment 8766401 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good but the variable renaming shouldn't happen.

::: js/src/vm/Debugger.cpp
@@ +719,4 @@
>                                   const ScriptFrameIter* maybeIter, MutableHandleValue vp)
>  {
> +    MOZ_ASSERT_IF(maybeIter, maybeIter->abstractFramePtr() == referent);
> +    MOZ_ASSERT(!referent.script()->selfHosted());

Why rename frame -> referent?

@@ +732,3 @@
>  
> +        RootedNativeObject frame(cx, DebuggerFrame::create(cx, proto, referent, maybeIter,
> +                                                           debugger));

So that you can use `frame` as a name for this one? I think `frameobj` was just fine, and this renaming is making this patch hard to follow.
Attachment #8766401 - Flags: review?(nfitzgerald)
Comment on attachment 8766402 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.getCallee.

Review of attachment 8766402 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +7036,5 @@
> +{
> +    AbstractFramePtr referent = frame->referent();
> +    Debugger* dbg = frame->owner();
> +
> +    if (!referent.isFunctionFrame()) {

Nit: move this check right after defining `referent`, and get the owner Debugger after we know we aren't taking the early abort path. I think it reads better this way and is more similar to the usual spidermonkey early abort style.

  AbstractFramePtr referent = frame->referent();
  if (!referent.isFunctionFrame()) {
      result.set(nullptr);
      return true;
  }

  Debugger* dbg = frame->owner();
  ...
Attachment #8766402 - Flags: review?(nfitzgerald) → review+
Reporter

Comment 8

3 years ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #6)
> Comment on attachment 8766401 [details] [diff] [review]
> Implement a DebuggerFrame class.
> 
> Review of attachment 8766401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks mostly good but the variable renaming shouldn't happen.
> 
> ::: js/src/vm/Debugger.cpp
> @@ +719,4 @@
> >                                   const ScriptFrameIter* maybeIter, MutableHandleValue vp)
> >  {
> > +    MOZ_ASSERT_IF(maybeIter, maybeIter->abstractFramePtr() == referent);
> > +    MOZ_ASSERT(!referent.script()->selfHosted());
> 
> Why rename frame -> referent?
> 
> @@ +732,3 @@
> >  
> > +        RootedNativeObject frame(cx, DebuggerFrame::create(cx, proto, referent, maybeIter,
> > +                                                           debugger));
> 
> So that you can use `frame` as a name for this one? I think `frameobj` was
> just fine, and this renaming is making this patch hard to follow.

I've been using the same naming scheme in the Debugger.Object patches:
- object for the Debugger.Object instance
- referent for the JSObject it refers to

And the Debugger.Environment patches:
- environment for the Debugger.Environment instance
- referent for the Env it refers to

The reason I did it like this is that I felt that 'dbgobj', which we used for Debugger.Object, is misleading, since Debugger instances also have an associated object. The above naming scheme felt more consistent, and didn't lead to misleading name. To my knowledge, Jim never commented on it, so I assumed he was ok with the change.

If it's ok with you, I'd like to continue on the same foot for the Debugger.Frame patches.
Comment on attachment 8766403 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.getIsConstructing.

Review of attachment 8766403 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +7063,5 @@
> +                                  Maybe<ScriptFrameIter>& result)
> +{
> +    AbstractFramePtr referent = frame->referent();
> +    if (referent.isScriptFrameIterData()) {
> +        result.emplace(*(ScriptFrameIter::Data*)(referent.raw()));

reinterpret_cast<ScriptFrameIter::Data*>(referent.raw());

@@ +7072,5 @@
> +            ++iter;
> +        AbstractFramePtr data = iter.copyDataAsAbstractFramePtr();
> +        if (!data)
> +            return false;
> +        frame->setPrivate(data.raw());

Since you've put the guts of THIS_FRAME_ITER into this method, make THIS_FRAME_ITER call this method so we don't have two copies floating around.
Attachment #8766403 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8766404 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.getEnvironment.

Review of attachment 8766404 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +7113,5 @@
> +        env = GetDebugScopeForFrame(cx, iter.abstractFramePtr(), iter.pc());
> +        if (!env)
> +            return false;
> +    }
> + 

Nit: trailing whitespace on this line.
Attachment #8766404 - Flags: review?(nfitzgerald) → review+
Attachment #8766407 - Flags: review?(nfitzgerald) → review+

Comment 11

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95006e936e44
Implement a DebuggerFrame class;r=fitzgen
Reporter

Comment 13

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #12)
> backedout for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=31055868&repo=mozilla-
> inbound

Why did this not show up in my local build? I compile with --enable-optimize and --warnings-as-errors for good measure.
Flags: needinfo?(ejpbruel)

Comment 14

3 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59c06b26e78
Backed out changeset 95006e936e44 for bustage
Reporter

Comment 15

3 years ago
Attachment #8767136 - Flags: review?(nfitzgerald)
Reporter

Comment 16

3 years ago
Attachment #8767137 - Flags: review?(nfitzgerald)
Reporter

Comment 17

3 years ago
Attachment #8767138 - Flags: review?(nfitzgerald)
Reporter

Comment 18

3 years ago
Attachment #8767139 - Flags: review?(nfitzgerald)
Reporter

Comment 19

3 years ago
Attachment #8767140 - Flags: review?(nfitzgerald)
Reporter

Comment 20

3 years ago
Try push for the patch to implement DebuggerFrame class to the patch to implement DebuggerFrame.isGenerator:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=649b84496ae9
Comment on attachment 8767136 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.isLive.

Review of attachment 8767136 [details] [diff] [review]:
-----------------------------------------------------------------

I remember asking you to define {Rooted,Handle,MutableHandle}Debugger{Frame,Object, etc} etc in js/src/gc/Rooting.h and use those typedefs to be consistent with the usual sm style, but I notice that still hasn't happened. Please do it.
Attachment #8767136 - Flags: review?(nfitzgerald) → review+
Attachment #8767137 - Flags: review?(nfitzgerald) → review+
Attachment #8767138 - Flags: review?(nfitzgerald) → review+
Attachment #8767139 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8767139 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.type.

Review of attachment 8767139 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +7414,5 @@
> +        str = cx->names().call;
> +        break;
> +      case DebuggerFrameType::Module:
> +        str = cx->names().module;
> +        break;

Does the compiler make a warning that we treat as an error and fail a build if you drop one of these cases? If not, let's add a `default: MOZ_CRASH("...")` branch or initialize str to nullptr and then assert that it is not nullptr after the switch.
Comment on attachment 8767140 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.implementation.

Review of attachment 8767140 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +7246,5 @@
> +    if (referent().isBaselineFrame())
> +        return DebuggerFrameImplementation::Baseline;
> +    else if (referent().isRematerializedFrame())
> +        return DebuggerFrameImplementation::Ion;
> +    return DebuggerFrameImplementation::Interpreter;

Nit: Add

  MOZ_ASSERT(referent().isInterpreterFrame());

right before the return.

@@ +7447,2 @@
>          s = "interpreter";
> +        break;

Ditto regarding assertions about every case if removing a case doesn't cause the build to fail.
Attachment #8767140 - Flags: review?(nfitzgerald) → review+
Reporter

Comment 24

3 years ago
@fitzgen @jimb: I've spent today working on a proof of concept of a C++ interface for DebuggerFrame.onStep. This accessor is more complicated than the others, because it involves a callback object.

The general idea is that for the C++ interface, the callback is a pointer to an interface with a strongly typed handle method. This allows consumers of the C++ interface to implement callbacks without being tied down to using a JS function, while still giving us the flexibility to implement JS function callbacks on top of it.

I've attached the patch (including some comments/questions) to the bug for you to give feedback on. Before moving forward, I feel like it would be a good idea if we go over the patch together, and make sure we all agree on the approach to take. I will request a meeting to that end shortly after this.
Attachment #8767700 - Flags: feedback?(nfitzgerald)
Attachment #8767700 - Flags: feedback?(jimb)
Reporter

Comment 25

3 years ago
Attached a slightly updated patch.
Attachment #8767700 - Attachment is obsolete: true
Attachment #8767700 - Flags: feedback?(nfitzgerald)
Attachment #8767700 - Flags: feedback?(jimb)
Attachment #8767701 - Flags: feedback?(nfitzgerald)
Attachment #8767701 - Flags: feedback?(jimb)
Reporter

Comment 26

3 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #20)
> Try push for the patch to implement DebuggerFrame class to the patch to
> implement DebuggerFrame.isGenerator:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=649b84496ae9

Previous try push has test failures due to using the wrong prototype. Here is a new try push with that issue addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=064fa0f1a9fb

Comment 27

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f1b9d6f522
Implement a DebuggerFrame class;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/14212ea063c6
Implement a C++ interface for DebuggerFrame.getCallee;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b93deff3205a
Implement a C++ interface for DebuggerFrame.getIsConstructing;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/8698c60d265d
Implement a C++ interface for DebuggerFrame.getEnvironment;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/359a15f3afea
Implement a C++ interface for DebuggerFrame.isGenerator;r=fitzgen
Comment on attachment 8767701 [details] [diff] [review]
Proof of concept for a C++ interface for DebuggerFrame.onStep.

Review of attachment 8767701 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +1947,5 @@
> +        // XXX: Could we makes DebuggerFrames a GCVector of DebuggerFrame
> +        // instances? (as opposed to NativeObjects). When I tried this out I ran
> +        // into obscure template error messages. I suspect we have a template
> +        // specialization for GCVector<NativeObject*> somwhere, but have not
> +        // been able to find it so far. 

Yeah that would definitely be nicer, although I don't know much about the specialization failures without seeing them. Hopefully they point at missing trait methods for GCPolicy<T> or BarrierMethods<T> that should be straightforward to add.

@@ +7253,5 @@
> +OnStepHandler*
> +DebuggerFrame::onStepHandler() const
> +{
> +    Value value = getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER);
> +    return !value.isUndefined() ? static_cast<OnStepHandler*>(value.toPrivate()) : nullptr;

Super nitty nitty nit nit: personally, I think this would read a little more naturally if you removed the negation in the condition and swapped the consequent and alternative.

@@ +7317,5 @@
> +static void
> +DebuggerFrame_trace(JSTracer* tracer, JSObject* obj)
> +{
> +    OnStepHandler* handler = obj->as<DebuggerFrame>().onStepHandler();
> +    if (handler) 

trailing whitespace

@@ +7745,5 @@
> +class OnStepHandlerFunction final : public OnStepHandler {
> +  public:
> +    OnStepHandlerFunction(JSObject* object) : object_(object)
> +    {
> +    }

Other code below relies on object being non-null (eg the trace method) so I think we should assert that we always construct valid instances:

  MOZ_ASSERT(object);

@@ +7847,5 @@
> +    // implements the OnStepHandler interface.
> +    //
> +    // Since the Debugger.Frame instance frees the onStep handler if necessary,
> +    // it must be allocated with cx->new_. Is this a reasonable restriction if
> +    // we want the allocated object to interface with Rust code?

We could make it call some generic `destroy` method instead of actually using `js_delete` and destructors.

In general, yes this looks how I imagined.

I think we should also provide a way for Rust/embedders to unset the hook and take back ownership.

::: js/src/vm/Debugger.h
@@ +393,5 @@
> +     *         return handleUncaughtException(ac, vp, callHook).
> +     */
> +    JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, bool OK, const Value& rv,
> +                                      AbstractFramePtr frame, jsbytecode* pc, MutableHandleValue vp,
> +                                      bool callHook = true);

Did this change at all or is this just code motion?

@@ +1152,5 @@
>  
> +class Handler {
> +  public:
> +    virtual ~Handler() {}
> +    virtual void trace(JSTracer* trc) = 0;

Need lots of docs for this stuff; it's going to be exposed to embedders and they should know how to use it without knowing spidermonkey internals. GC integration in particular is something that should be documented for folks who don't know the GC implementation.

@@ +1157,5 @@
> +};
> +
> +class OnStepHandler : public Handler {
> +  public:
> +    virtual JSTrapStatus handle(JSContext* cx, const ScriptFrameIter& iter,

I think we should name this `handleStep` or `onStep` or something on the off chance someone makes a single handler handle multiple things so we won't get any method name collisions.

@@ +1192,5 @@
>      static MOZ_MUST_USE bool getThis(JSContext* cx, Handle<DebuggerFrame*> frame,
>                                       MutableHandleValue result);
>  
> +    Debugger* owner() const;
> +    AbstractFramePtr referent() const;

This is just code motion?
Attachment #8767701 - Flags: feedback?(nfitzgerald) → feedback+
Assignee

Comment 30

3 years ago
Comment on attachment 8767701 [details] [diff] [review]
Proof of concept for a C++ interface for DebuggerFrame.onStep.

Review of attachment 8767701 [details] [diff] [review]:
-----------------------------------------------------------------

To summarize our discussion this morning:

Apparently, the simpler Rust apps embedding SpiderMonkey will usually be using a different malloc for Rust objects that SpiderMonkey uses for its own, so we need to support the case where the handler pointer is an inter-heap pointer. This means that the Handler base class must provide a trace method and a drop method, where the drop method does everything necessary for the Debugger to stop using the pointer, including freeing the Handler's storage if appropriate. That way, the Handler implementation can decide which malloc to use.

You have the destructor as a virtual method of Handler; perhaps the destructor itself need not be part of the abstract base class, because Debugger will only ever call the drop method.

It will be interesting to see how to turn a class with virtual methods into Rust types. Apparently bindgen doesn't do this now, but for an implementation Foo of Handler, I'd expect something like:

struct Foo {
    vtable: &'static Foo_Vtable;
    ... data members of Foo ...
}

// This must match the ABI-specified vtable layout for Handler. I'm omitting some fields.
struct Foo_Vtable {
    delete: fn(this: *mut Foo),
    trace: trace(this: *mut Foo, tracer: *mut JSTracer)
}

trait Handler {
    fn delete(&mut self);
    fn trace(&mut self, tracer: *mut JSTracer);
}

impl Handler for Foo {
    fn delete(&mut self) {
        self.vtable.delete(self);
    }
    fn trace(&mut self, tracer: *mut JSTracer) {
        self.vtable.trace(self, tracer);
    }
}

::: js/src/vm/Debugger.cpp
@@ +1944,5 @@
>  
>      // Call onStep for frames that have the handler set.
>      for (size_t i = 0; i < frames.length(); i++) {
> +        // XXX: Could we makes DebuggerFrames a GCVector of DebuggerFrame
> +        // instances? (as opposed to NativeObjects). When I tried this out I ran

Sure, that seems like the right thing.

::: js/src/vm/Debugger.h
@@ +393,5 @@
> +     *         return handleUncaughtException(ac, vp, callHook).
> +     */
> +    JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, bool OK, const Value& rv,
> +                                      AbstractFramePtr frame, jsbytecode* pc, MutableHandleValue vp,
> +                                      bool callHook = true);

This method shouldn't be public. It would be better to have a common base class for those Handler implementations that invoke JS functions, and make that a friend of Debugger.
Attachment #8767701 - Flags: feedback?(jimb) → feedback+
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Reporter

Comment 31

3 years ago
Yet another try push, with the memory leak that caused the previous backout hopefully resolved:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a52a6a7f6af2

Comment 32

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d59741e736
Implement a DebuggerFrame class;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4fb2446f1f
Implement a C++ interface for DebuggerFrame.getCallee;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/276a5e166738
Implement a C++ interface for DebuggerFrame.getIsConstructing;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/763679990ba8
Implement a C++ interface for DebuggerFrame.getEnvironment;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c1b2a66590
Implement a C++ interface for DebuggerFrame.isGenerator;r=fitzgen
Reporter

Comment 33

3 years ago
Attachment #8768729 - Flags: review?(nfitzgerald)
Reporter

Comment 34

3 years ago
Attachment #8768750 - Flags: review?(nfitzgerald)
Reporter

Comment 35

3 years ago
Try push from the patch to implement a C++ interface for DebuggerFrame.isLive to the patch to implement a C++ interface for DebuggerFrame.implementation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79340c5f8067
Comment on attachment 8768729 [details] [diff] [review]
DebuggerFrameVector should be a GCVector<DebuggerFrame*>.

Review of attachment 8768729 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ slightly more type safe!
Attachment #8768729 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8768750 [details] [diff] [review]
Add GC typedefs for commonly used debugger types.

Review of attachment 8768750 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/src/vm/Debugger.h
@@ +258,5 @@
> +typedef JS::Handle<DebuggerObject*> HandleDebuggerObject;
> +
> +typedef JS::MutableHandle<DebuggerEnvironment*> MutableHandleDebuggerEnvironment;
> +typedef JS::MutableHandle<DebuggerFrame*> MutableHandleDebuggerFrame;
> +typedef JS::MutableHandle<DebuggerObject*> MutableHandleDebuggerObject;

These belong in js/src/gc/Rooting.h, with forward decls as necessary.
Attachment #8768750 - Flags: review?(nfitzgerald) → review+
Reporter

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Comment 39

3 years ago
I've rewritten the patch to implement a C++ interface for DebuggerFrame.onStep based on your and Jim's feedback. The patch is ready for review, but we probably need one or two iterations before we are both happy with it.

Things I'd like to get feedback on in particular:
1. Did I miss anything in the interface? You mentioned in your feedback that we
   need a way for Rust to take back ownership of the handler. If we treat the
   handlers as a reference to a trait object, rather than the trait object
   itself, the delete_ method should allow us to do this, no?
2. Are you happy with the documentation? Is there anything you would like to add
   or phrase differently?
3. How do you feel about the cast in DebuggerFrame::onStepGetter? That cast is
   only safe if the handler was created by DebuggerFrame::onStepSetter. Any
   attempt to get a handler from JS that was created by C++ (and does not wrap
   a JSObject*) will cause a crash. That's arguably not something you would
   ever do, but you never know.
Attachment #8767701 - Attachment is obsolete: true
Attachment #8769657 - Flags: review?(nfitzgerald)
Reporter

Comment 40

3 years ago
On the off chance that you're happy with the previous patch as is, I'm putting up the next patch for r? as well. Feel free to cancel this r? if you've r-'d the previous patch.
Attachment #8769661 - Flags: review?(nfitzgerald)
Reporter

Comment 41

3 years ago
Try push for the patch to turn DebuggerFrameVector into a GCVector<DebuggerFrame> and the patch to add GC typedefs for commonly used Debugger types:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb36acd94a1
Reporter

Updated

3 years ago
Keywords: leave-open

Comment 42

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bda8deb45e63
Implement a C++ interface for DebuggerFrame.isLive;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa204468133
Implement a C++ interface for DebuggerFrame.getOffset;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/2879d03b9ce4
Implement a C++ interface for DebuggerFrame.getOlder;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f55b62daef
Implement a C++ interface for DebuggerFrame.type;r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c374964ed8db
Implement a C++ interface for DebuggerFrame.implementation;r=fitzgen
backed out for hazard failures like : https://treeherder.mozilla.org/logviewer.html#?job_id=31607995&repo=mozilla-inbound

sorry eddy!
Flags: needinfo?(ejpbruel)
Reporter

Comment 45

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #43)
> backed out for hazard failures like :
> https://treeherder.mozilla.org/logviewer.html#?job_id=31607995&repo=mozilla-
> inbound
> 
> sorry eddy!

Rooting hazards eh? That's a new one. Can I somehow run this analysis locally?
Flags: needinfo?(ejpbruel)
Reporter

Comment 46

3 years ago
SpiderMonkey has the convention that a method should not be static if it cannot GC. I didn't (and in fact I still don't) feel comfortable with this rule, because it forces you to think about whether your method can GC or not, potentially leading to subtle GC hazards if you get it wrong.

Because of these worries, we ended up settling on the compromise that a method should not be static if it does not take a JSContext* as parameter. The assumption was that if a method does not take a JSContext* as parameter, then it cannot GC, so it is always safe to make it non-static.

As it now turns out, that assumption does not hold. Since it does not take a JSContext* as parameter, at first glance it may look like DebuggerFrame::referent cannot GC. However, the GC hazard analysis, which we run on inbound, thinks that it does (Shu had a good explanation for why it must, but I don't remember it). Because of this, the previous 5 patches got backed out.

To fix the issue, we have to change DebuggerFrame::referent into a static function, and then rebase the other patches on top of this change.
Attachment #8767136 - Attachment is obsolete: true
Attachment #8767137 - Attachment is obsolete: true
Attachment #8767138 - Attachment is obsolete: true
Attachment #8767139 - Attachment is obsolete: true
Attachment #8767140 - Attachment is obsolete: true
Attachment #8768729 - Attachment is obsolete: true
Attachment #8768750 - Attachment is obsolete: true
Attachment #8770144 - Flags: review?(nfitzgerald)
Reporter

Comment 47

3 years ago
Rebased patch to implement a C++ interface for DebuggerFrame.isLive. Patch is essentially unchanged, so carrying over r+.
Attachment #8770145 - Flags: review+
Reporter

Comment 48

3 years ago
Rebased patch to implement a C++ interface for DebuggerFrame.getOffset. Patch is essentially unchanged, so carrying over r+.
Attachment #8770148 - Flags: review+
Reporter

Updated

3 years ago
Attachment #8770148 - Attachment is patch: true
Attachment #8770148 - Attachment mime type: text/x-patch → text/plain
Reporter

Comment 49

3 years ago
Rebased patch to implement a C++ interface for DebuggerFrame.getOlder. Patch is essentially unchanged, so carrying over r+.
Attachment #8770150 - Flags: review+
Reporter

Comment 50

3 years ago
Rebased patch to implement a C++ interface for DebuggerFrame.getType. This patch has changed due to the changes in DebuggerFrame::referent, so re-requesting review.
Attachment #8770151 - Flags: review?(nfitzgerald)
Reporter

Comment 51

3 years ago
Rebased patch to implement a C++ interface for DebuggerFrame.getType. This patch has changed due to the changes in Debugger::referent, so re-requesting review.
Attachment #8770153 - Flags: review?(nfitzgerald)
Reporter

Updated

3 years ago
Attachment #8770153 - Attachment is patch: true
Attachment #8770153 - Attachment mime type: text/x-patch → text/plain
Reporter

Comment 52

3 years ago
Rebased patch to turn DebuggerFrameVector into GCVector<DebuggerFrame*>. Patch is essentially unchanged, so carrying over r+.
Attachment #8770157 - Flags: review+
Reporter

Comment 53

3 years ago
Rebased patch to add GC typedefs for commonly used debugger types. Had to resolve a few merge conflicts, and change some additional types introduced in the earlier rebased patches, so re-requesting review.
Attachment #8770158 - Flags: review?(nfitzgerald)
Reporter

Comment 54

3 years ago
At the moment, DebuggerGenericEval, which is used by DebuggerFrame_eval, DebuggerFrame_evalWithBindings and DebuggerObject::executeInGlobal, returns a JSValue representing a completion value.

For the C++ interface, we prefer strong types over weak types wherever possible. A completion value consists of a status and value. By making DebuggerGenericEval return these two values directly we obtain an interface that is more strongly typed.

Similarly, DebuggerObject::executeInGlobal, which belongs to the C++ interface of DebuggerObject, should also return these two values directly. DebuggerFrame_eval and DebuggerFrame_evalWithBindings don't yet have a C++ equivalent, so these will be addressed in an upcoming patch.
Attachment #8770159 - Flags: review?(nfitzgerald)
Reporter

Comment 55

3 years ago
This patch moves EvaluateInEnv and DebuggerGenericEval to near the C++ interface functions for DebuggerFrame, so I will be able to define DebuggerFrame::eval and DebuggerFrame::evalInBindings (which depend on these functions) near the other C++ interface functions in the next patch.
Attachment #8770161 - Flags: review?(nfitzgerald)
Reporter

Comment 56

3 years ago
I'm still waiting for your review of the onStep/onPop handler methods, so I went ahead and implemented the remaining C++ interface functions for Debugger.Frame. Note that these patches do not depend on the onStep/onPop handler patches, so feel free to hold off on those if you need more time to think.
Attachment #8770163 - Flags: review?(nfitzgerald)
Attachment #8770158 - Flags: review?(nfitzgerald) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #46)
> Created attachment 8770144 [details] [diff] [review]
> DebuggerFrame::referent should be a static method.
> 
> SpiderMonkey has the convention that a method should not be static if it
> cannot GC. I didn't (and in fact I still don't) feel comfortable with this
> rule, because it forces you to think about whether your method can GC or
> not, potentially leading to subtle GC hazards if you get it wrong.
> 
> Because of these worries, we ended up settling on the compromise that a
> method should not be static if it does not take a JSContext* as parameter.
> The assumption was that if a method does not take a JSContext* as parameter,
> then it cannot GC, so it is always safe to make it non-static.
> 
> As it now turns out, that assumption does not hold. Since it does not take a
> JSContext* as parameter, at first glance it may look like
> DebuggerFrame::referent cannot GC. However, the GC hazard analysis, which we
> run on inbound, thinks that it does (Shu had a good explanation for why it
> must, but I don't remember it). Because of this, the previous 5 patches got
> backed out.
> 
> To fix the issue, we have to change DebuggerFrame::referent into a static
> function, and then rebase the other patches on top of this change.

I'm left with a couple questions:

(1) Which operation(s) inside referent() can GC?
(2) Which pointers get invalidated? Is it pointers inside referent() calls or GC pointers across referent() calls?

In general though I'm a bit preoccupied with getting ready for my ECOOP presentation and feel like I'm not able to give these patches the attention they deserve and so I'm bumping review back to jimb.
Attachment #8770159 - Flags: review?(nfitzgerald) → review?(jimb)
Attachment #8770161 - Flags: review?(nfitzgerald) → review?(jimb)
Attachment #8770163 - Flags: review?(nfitzgerald) → review?(jimb)
Attachment #8769657 - Flags: review?(nfitzgerald) → review?(jimb)
Attachment #8769661 - Flags: review?(nfitzgerald) → review?(jimb)
Attachment #8770144 - Flags: review?(nfitzgerald) → review?(jimb)
Attachment #8770151 - Flags: review?(nfitzgerald) → review?(jimb)
Attachment #8770153 - Flags: review?(nfitzgerald) → review?(jimb)
Assignee

Comment 58

3 years ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #57)
> (1) Which operation(s) inside referent() can GC?
> (2) Which pointers get invalidated? Is it pointers inside referent() calls
> or GC pointers across referent() calls?

The stack iterator uses JSPrincipal subsumption to decide which frames to show. Subsumption is controlled by an embedding-provided callback, which the rooting analysis conservatively assumes could do anything at all, including cause a GC. (This is known as the "Scumbag JSPrincipal Subsumption Check" problem.)

So, we don't know of any reasons that any actual pointers would really be changed by calling 'referent'. But to err on the side of caution and keep the static analysis happy, we'll just treat it as if it could GC.
Reporter

Comment 59

3 years ago
(In reply to Jim Blandy :jimb from comment #58)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #57)
> > (1) Which operation(s) inside referent() can GC?
> > (2) Which pointers get invalidated? Is it pointers inside referent() calls
> > or GC pointers across referent() calls?
> 
> The stack iterator uses JSPrincipal subsumption to decide which frames to
> show. Subsumption is controlled by an embedding-provided callback, which the
> rooting analysis conservatively assumes could do anything at all, including
> cause a GC. (This is known as the "Scumbag JSPrincipal Subsumption Check"
> problem.)
> 
> So, we don't know of any reasons that any actual pointers would really be
> changed by calling 'referent'. But to err on the side of caution and keep
> the static analysis happy, we'll just treat it as if it could GC.

Thank you for explaining it better than I could Jim.
Assignee

Comment 60

3 years ago
Comment on attachment 8770144 [details] [diff] [review]
DebuggerFrame::referent should be a static method.

Review of attachment 8770144 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, that's too bad. Well, seems necessary.
Attachment #8770144 - Flags: review?(jimb) → review+
Assignee

Comment 61

3 years ago
Eddy, could you write a patch that changes

/* static */ bool
DebuggerFrame::getScriptFrameIter(JSContext* cx, Handle<DebuggerFrame*> frame,
                                  Maybe<ScriptFrameIter>& result)

to:

/* static */ Maybe<ScriptFrameIter>
DebuggerFrame::getScriptFrameIter(JSContext* cx, Handle<DebuggerFrame*> frame)

It's sort of ugly to have the returned bool carry exactly the same information as the discriminant of the Maybe. In other similar cases we've returned Maybe by value, so doing so here would be more consistent. And since structs returned by value are actually implemented by constructing the return value in a buffer provided by the caller, the actual machine code is going to be very similar.

(This is something Nick tells me he decided not to nit, but it bugs me quite a bit.)
Assignee

Comment 62

3 years ago
Comment on attachment 8770151 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.getType.

Review of attachment 8770151 [details] [diff] [review]:
-----------------------------------------------------------------

I think there are missing changes here:

::: js/src/vm/Debugger.cpp
@@ +7509,5 @@
>      return true;
>  }
>  
> +/* static */ bool
> +DebuggerFrame::thisGetter(JSContext* cx, unsigned argc, Value* vp)

Why doesn't DebuggerFrame::thisGetter use DebuggerFrame::getThis?
Attachment #8770151 - Flags: review?(jimb) → review-
Assignee

Updated

3 years ago
Attachment #8770153 - Flags: review?(jimb) → review+
Reporter

Comment 63

3 years ago
Looks like I accidentally merged two patches during rebase, but only partially :S

Here is the patch again, with the missing parts added.
Attachment #8770151 - Attachment is obsolete: true
Attachment #8770736 - Flags: review?(jimb)
Assignee

Comment 64

3 years ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #9)
> Since you've put the guts of THIS_FRAME_ITER into this method, make
> THIS_FRAME_ITER call this method so we don't have two copies floating around.

Eddy, I'd like to re-iterate this request. It's a distraction to have to wonder why there are two copies and carefully check them over for differences.
Assignee

Comment 65

3 years ago
Comment on attachment 8770736 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.getType.

Review of attachment 8770736 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, much better.
Attachment #8770736 - Flags: review?(jimb) → review+
Assignee

Comment 66

3 years ago
Comment on attachment 8770159 [details] [diff] [review]
DebuggerGenericEval should return both trap status and value.

Review of attachment 8770159 [details] [diff] [review]:
-----------------------------------------------------------------

If I'm understanding this correctly, in the old arrangement, receiveCompletionValue carries out three steps:

- call resultToCompletion
- destruct the AutoCompartment
- call newCompletionValue

In the new arrangement, the same steps get carried out in the same order:

- DebuggerGenericEval calls resultToCompletion directly.
- The AutoCompartment is destructed by returning from DebuggerGenericEval.
- The caller then calls newCompletion value, if needed.

It worries me that DebuggerGenericEval is now returning a debuggee value directly, without calling wrapDebuggeeValue on it. That seems wrong. So I think you're going to need to reintroduce the explicit AutoCompartment destruction in DebuggerGenericEval, and change newCompletionValue to assert that its argument is already in the debugger's compartment, rather than wrapping it itself.

::: js/src/vm/Debugger.cpp
@@ +7895,5 @@
>  static bool
>  DebuggerGenericEval(JSContext* cx, const mozilla::Range<const char16_t> chars,
> +                    HandleObject bindings, const EvalOptions& options, JSTrapStatus& status,
> +                    MutableHandleValue value, Debugger* dbg, HandleObject scope,
> +                    ScriptFrameIter* iter)

Nit: put status and value on their own line, with bindings and options above, and dbg, scope and iter below.
Attachment #8770159 - Flags: review?(jimb) → review-
Assignee

Comment 67

3 years ago
Comment on attachment 8770161 [details] [diff] [review]
Move EvaluateInEnv and DebuggerGenericEval.

Review of attachment 8770161 [details] [diff] [review]:
-----------------------------------------------------------------

This is the wrong patch.
Attachment #8770161 - Flags: review?(jimb)
Assignee

Updated

3 years ago
Attachment #8770163 - Flags: review?(jimb) → review+
Assignee

Comment 68

3 years ago
Comment on attachment 8769657 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.onStep.

Review of attachment 8769657 [details] [diff] [review]:
-----------------------------------------------------------------

Still reviewing, but I wanted to publish what I had to this point:

I think the design of Handler is pretty close to what we want. There are two remaining bits of dissonance to resolve.

First, when we have C++ set a Debugger.Frame's onStep handler to some custom OnStepHandler subclass, and then JavaScript retrieves that handler, what should it see? There's no JavaScript representation for a random OnStepHandler. I suppose we could wrap it up as a JavaScript native function, but that doesn't seem worth it. So I think we need to return undefined or something.

But the immediate point is, Debugger needs some way to know whether a Handler is JS or not. Perhaps there should be a "retrieveJSFunction" virtual method, with a default definition that returns nullptr, which OnStepHandlerFunction overrides to return the actual function?

I don't remember what our original expectations were, but now it's clear that reflection objects (Debugger, Debugger.Frame, etc.) own *pointers* to Handlers, not Handlers themselves. Since Handler is an abstract base class, the reflection objects have no idea in general what size the concrete classes will be, so they can only point to them.

If reflection objects only own pointers to Handlers, then the only finalization-related method that Handler needs to offer is a "drop" method. It doesn't need to mention a destructor, nor a delete method.

That is, I think we need something like this:

struct Handler {
  virtual void drop() = 0;
  virtual void trace(JSTracer* tracer) = 0;
  virtual JSFunction* maybeJSFunction() { return nullptr; }
  protected:
    ~Handler() { }
}

The comments for drop should definitely continue to mention that the Handler may be using a different allocator. That much is great. But it should talk about dropping pointers to Handlers, not about deleting and freeing their storage.

Making the destructor protected, in Handler and in derived abstract classes like OnStepHandler, ensures that reflection objects that know only the base class can never destruct the instances, only drop them.

We're going to need to be super-careful about dropping these things properly; self-assignment hazards, the whole nine yards. The disadvantage of storing these things in NativeObject slots is that we can't easily use a smart pointer type to gather all those rules into one place.

::: js/src/vm/Debugger.h
@@ +34,5 @@
>      JSTRAP_THROW,
>      JSTRAP_LIMIT
>  };
>  
> +class OnStepHandlerFunction;

I would expect these to be in the same namespace as DebuggerObject, etc.

Also, by placing "Function" in the name, you mean to indicate that this is a handler that calls a JS function. But there are "Functions" all over the place, in all languages. OnStepHandlerScripted might be a better name.

@@ +1158,5 @@
> + * this data (see below).
> + *
> + * Finally, because handlers are implemented as interfaces, they need to be
> + * allocated on the heap. Since we generally don't know what allocator was used
> + * to allocate handelr, each handler also needs to provide a method to delete

"handelr"

@@ +1166,5 @@
> + * each handler (i.e. trace and delete). Handlers for specific events inherit
> + * from this base class and define the method to be called when that event
> + * happens.
> + */
> +class Handler {

I think abstract base classes should just be 'struct', since their whole purpose is to provide public methods.
Assignee

Comment 69

3 years ago
Comment on attachment 8769657 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.onStep.

Review of attachment 8769657 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, this is all I have. I'd like to review the revised patch as well.

::: js/src/vm/Debugger.cpp
@@ -1950,5 @@
> -        Debugger* dbg = Debugger::fromChildJSObject(frame);
> -        EnterDebuggeeNoExecute nx(cx, *dbg);
> -
> -        Maybe<AutoCompartment> ac;
> -        ac.emplace(cx, dbg->object);

This patch moves the EnterDebuggeeNoExecute and AutoCompartment into OnStepHandlerFunction::onStep, but I don't see why different subclasses of OnStepHandler should run in different dynamic contexts this way. This code should stay where it is.

@@ +7230,5 @@
> +    }
> +
> +    if (prior && handler != prior) {
> +        prior->delete_();
> +    }

nit: let's delete / drop prior after we've replaced it with handler. It feels strange to have the slot referring to a deleted handler...

@@ +7796,5 @@
> +        // Calling the handler function may cause the frame's referent to be
> +        // changed into an ion frame. parseResumptionValue expects the referent
> +        // to be either an interpreter or baseline frame, so we need to copy it
> +        // before calling the handler function below.
> +        AbstractFramePtr referent = frame->referent();

AbstractFramePtr is incapable of referring to an Ion frame. If the referent really has turned into an Ion frame, then the `referent` value saved here is certainly invalid by the time we try to use it in parseResumptionValue.

I have forgotten the details of how Debugger.Frame instances refer to the various sorts of frames. But there's nothing in this patch that should have any effect on the behavior of the JITs, so this seems like evidence of a bug elsewhere.
Attachment #8769657 - Flags: review?(jimb) → review-
Assignee

Comment 70

3 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #40)
> On the off chance that you're happy with the previous patch

(Just to be clear, I'm very happy with the previous patch! I rejected it because we need to go around on the design once more.)
Assignee

Comment 71

3 years ago
(In reply to Jim Blandy :jimb from comment #69)
> I have forgotten the details of how Debugger.Frame instances refer to the
> various sorts of frames. But there's nothing in this patch that should have
> any effect on the behavior of the JITs, so this seems like evidence of a bug
> elsewhere.

I checked up with Shu; there's no way the frame whose onStep handler we're calling could have turned into an Ion frame. It's marked as a debuggee frame, and those are never Ion. So there's some other bug here.
Assignee

Comment 72

3 years ago
Comment on attachment 8769661 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.onPop.

Review of attachment 8769661 [details] [diff] [review]:
-----------------------------------------------------------------

Same general thoughts here as with onStep. I'd like to re-review after it's been changed.

::: js/src/vm/Debugger.cpp
@@ -893,5 @@
>          /* For each Debugger.Frame, fire its onPop handler, if any. */
>          for (size_t i = 0; i < frames.length(); i++) {
> -            HandleDebuggerFrame frameobj = frames[i];
> -            Debugger* dbg = Debugger::fromChildJSObject(frameobj);
> -            EnterDebuggeeNoExecute nx(cx, *dbg);

Ideally this would stay here, in the code that invokes the onPopHandler.

@@ -901,5 @@
> -            {
> -                RootedValue handler(cx, frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER));
> -
> -                Maybe<AutoCompartment> ac;
> -                ac.emplace(cx, dbg->object);

Similarly.

@@ +7271,5 @@
> +OnPopHandler*
> +DebuggerFrame::onPopHandler() const
> +{
> +    Value value = getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER);
> +    return value.isUndefined() ? nullptr : static_cast<OnPopHandler*>(value.toPrivate());

I believe private values can store nullptr, too. I think it would be a bit cleaner if we *always* stored a private value in the slots that refer to Handlers, and then checked for nullptr after calling toPrivate. This would become, simply:

    return static_cast<OnPopHandler*>(value.toPrivate());

@@ +7280,5 @@
> +{
> +    OnPopHandler* prior = onPopHandler();
> +    if (prior && handler != prior) {
> +        prior->delete_();
> +    }

Same slight nit as before: it would make me a little more comfortable if we deleted the prior handler *after* we install the new one.

@@ +7347,5 @@
> +        onStepHandler->delete_();
> +
> +    OnPopHandler* onPopHandler = obj->as<DebuggerFrame>().onPopHandler();
> +    if (onPopHandler)
> +        onPopHandler->delete_();

Nit: why not have:

    DebuggerFrame &frame = obj->as<DebuggerFrame>();

and then use that everywhere? We're going to be adding more and more cases here.

@@ +7355,5 @@
>  DebuggerFrame_trace(JSTracer* trc, JSObject* obj)
>  {
> +    OnStepHandler* onStepHandler = obj->as<DebuggerFrame>().onStepHandler();
> +    if (onStepHandler) 
> +        onStepHandler->trace(trc);

Similarly.

@@ +7872,5 @@
> +        js_delete(this);
> +    }
> +
> +    virtual void trace(JSTracer* tracer) override {
> +        TraceEdge(tracer, &object_, "OnPopHandlerFunction.object");

Here and in the onStep patch, I think it might make more sense to label this edge "Debugger.Frame onPop handler". Since Debugger.Frame's trace function calls the OnPopHandler's trace function directly, there's no record of that edge; so tools will see the name given here attributed to Debugger.Frame objects.

@@ +7892,5 @@
> +        // Calling the handler function may cause the frame's referent to be
> +        // changed into an ion frame. parseResumptionValue expects the referent
> +        // to be either an interpreter or baseline frame, so we need to copy it
> +        // before calling the handler function below.
> +        AbstractFramePtr referent = frame->referent();

Here, too, this suggests a bug elsewhere.

::: js/src/vm/Debugger.h
@@ +35,5 @@
>      JSTRAP_LIMIT
>  };
>  
>  class OnStepHandlerFunction;
> +class OnPopHandlerFunction;

This should be in the same namespace as DebuggerObject et al.

@@ +1216,5 @@
>  
> +class OnPopHandler : public Handler {
> +  public:
> +    virtual JSTrapStatus onPop(JSContext* cx, HandleDebuggerFrame frame, JSTrapStatus status,
> +                               HandleValue value, jsbytecode* pc, MutableHandleValue result) = 0;

Why does an OnPopHandler need a `pc` pointer?
Attachment #8769661 - Flags: review?(jimb) → review-
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1

Comment 73

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/904a6eb36f1b
DebuggerFrame::referent should be a static method. r=jimb

Comment 74

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae502e2271f2
Implement a C++ interface for DebuggerFrame.isLive. r=fitzgen

Comment 76

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/496ab46d92e3
Implement a C++ interface for DebuggerFrame.getOffset. r=fitzgen

Comment 77

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0163913570
Implement a C++ interface for DebuggerFrame.getOlder. r=fitzgen

Comment 78

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb2a15891d3
Implement a C++ interface for DebuggerFrame.getType. r=jimb

Comment 79

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52057d746d72
Implement a C++ interface for DebuggerFrame.getImplementation. r=jimb
Depends on: 1291685
Reporter

Comment 81

3 years ago
Try push for the patches to change DebuggerFrameVector into GCVector<DebuggerFrame*> and add GC typedefs for commonly used debugger types:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be5b7095d395
Reporter

Comment 82

3 years ago
Apologies for the delay in this bug. I suffered a catastrophic crash of my Linux VM last week, and lost several days of work in the process. As a result, I find myself back on OS X again, where even basic stuff like running a jit-test with lldb doesn't work. Good times.
Reporter

Comment 83

3 years ago
New patch with comments by Jim addressed.
Attachment #8770159 - Attachment is obsolete: true
Attachment #8778839 - Flags: review?(jimb)
Assignee

Updated

3 years ago
Summary: Implement a C++ interface for for Debugger.Frame instances. → Implement a C++ interface for Debugger.Frame instances.
Assignee

Comment 84

3 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #82)
> I suffered a catastrophic crash of my
> Linux VM last week, and lost several days of work in the process.

Wow, that sucks!!!
Assignee

Comment 85

3 years ago
Comment on attachment 8778839 [details] [diff] [review]
DebuggerGenericEval should return both trap status and value.

Review of attachment 8778839 [details] [diff] [review]:
-----------------------------------------------------------------

The change to newCompletionValue is right, but this doesn't make the change to DebuggerGenericEval that I asked for.

The patch as written has DebuggerGenericEval return a (JSTrapStatus, unwrapped debuggee value) pair, leaving its callers to call wrapDebuggeeValue on the value.

Instead, DebuggerGenericEval should explicitly destruct `ac` by calling `ac.reset()` to leave the debuggee compartment, and then call wrapDebuggeeValue itself, returning a (JSTrapStatus, properly wrapped debuggee value) pair ready for newCompletionValue to consume.
Attachment #8778839 - Flags: review?(jimb) → review-
Reporter

Comment 86

3 years ago
New patch with review comments by jimb addressed that were unaddressed by the previous patch with review comments by jimb addressed (I think).
Attachment #8778839 - Attachment is obsolete: true
Attachment #8779241 - Flags: review?(jimb)
Reporter

Comment 87

3 years ago
Attachment #8770161 - Attachment is obsolete: true
Attachment #8779244 - Flags: review?(jimb)
Reporter

Comment 88

3 years ago
In the patch to implement a C++ interface for DebuggerFrame.onStep, I ran into a problem where the referent of a Debugger.Frame would change after calling its onStep handler. At the time, I thought this was due to the referent being changed to an Ion frame as a result of the call. However, after reviewing the patch, Jim pointed out that this shouldn't be happening.

This made me worried that we had a serious bug somewhere, so I wanted to know what exactly caused the referent to change. After some debugging, I came up with the following stack trace, which illustrates the problem:
https://pastebin.mozilla.org/8890749

As you can see, in this particular test, the onStep handler removes the global to which the frame belongs as debuggee. Doing that will cause the Debugger.Frame to be invalidated, and *that* causes it referent to be changed.

Now that we know this, it should be enough to simply create a copy of the referent before calling the onStep handler, like I did in the r-'d patch.
Assignee

Comment 89

3 years ago
Comment on attachment 8779241 [details] [diff] [review]
DebuggerGenericEval should return both trap status and value.

Review of attachment 8779241 [details] [diff] [review]:
-----------------------------------------------------------------

This has it all. Thanks!
Attachment #8779241 - Flags: review?(jimb) → review+
Assignee

Comment 90

3 years ago
Comment on attachment 8779244 [details] [diff] [review]
Move EvaluateInEnv and DebuggerGenericEval.

Review of attachment 8779244 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is just pure code motion, right? Thanks for splitting it out.
Attachment #8779244 - Flags: review?(jimb) → review+
Assignee

Comment 91

3 years ago
Moving the stack trace from pastebin into an attachment.
Assignee

Comment 92

3 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #88)
> This made me worried that we had a serious bug somewhere, so I wanted to
> know what exactly caused the referent to change. After some debugging, I
> came up with the following stack trace, which illustrates the problem:
> https://pastebin.mozilla.org/8890749
> 
> As you can see, in this particular test, the onStep handler removes the
> global to which the frame belongs as debuggee. Doing that will cause the
> Debugger.Frame to be invalidated, and *that* causes it referent to be
> changed.

Which test case is this, again? (Sorry, I did look through the bug, but I didn't see you mentioning the test case that motivates the change.)

I think I see the bug in the patch.

The code now in Mozilla Central initializes `iter` at the top of onSingleStep, finds all the Debugger.Frames that refer to that frame, and then iterates over them, using the original `iter` value to produce the arguments parseResumptionValue needs.

With the patch, the initial steps are the same: initialize `iter`, find all the Debugger.Frames that refer to that frame, and then iterate over them. However, the patch tries to retrieve the referent from each Debugger.Frame, instead of using the `iter` captured at the top of the function.

A test case that causes Debugger.Frames to be invalidated in their onStep handlers will certainly leave the Debugger.Frame unable to provide the information parseResumptionValue needs.

However, I think the patch is still incorrect: if there are two Debugger.Frames referring to a frame, and the first D.F's onStep handler removes the frame's global from second D.F's debuggee set, then the second iteration through the loop will invoke OnStepHandlerFunction::onStep, passing nothing but an invalidated Debugger.Frame. That function will have no way to reach the actual stack frame at all.

I think there's a bigger problem here with the OnStepHandler API. Clearly, only an OnStepHandler subclass that invokes a JavaScript handler should have to call parseResumptionValue. But parseResumptionValue requires types like AbstractFramePtr that shouldn't appear in a public interface like OnStepHandler::onStep.
Assignee

Comment 93

3 years ago
It looks to me like bug 1232685 has entangled Debugger::parseResumptionValue with a bunch of stuff it shouldn't be concerned with. All the OnStepHandler really wants to do is take a JavaScript value representing a resumption value and convert it to <JSTrapStatus, Value> form, and return that. And that's what parseResumptionValue is supposed to do. But now it's taking care of a bunch of ES6 concerns with correcting forced return values for derived class constructors and I-don't-know-what, that require an AbstractFramePtr.

All of that extra garbage *must* be handled outside OnStepHandler, not within implementations of OnStepHandler::onStep. Whatever it is, it certainly looks like stuff which needs to be handled in exactly the same way no matter what sort of handler is being invoked.

As it stands, it seems like what OnStepHandlerFunction really wants to call is something simpler, like ParseResumptionValueAsObject. All the other handling in parseResumptionValue should take place in OnStepHandler::onStep's caller.

So we need to refactor parseResumptionValue before we can get this right.
Reporter

Comment 94

3 years ago
(In reply to Jim Blandy :jimb from comment #93)
> It looks to me like bug 1232685 has entangled Debugger::parseResumptionValue
> with a bunch of stuff it shouldn't be concerned with. All the OnStepHandler
> really wants to do is take a JavaScript value representing a resumption
> value and convert it to <JSTrapStatus, Value> form, and return that. And
> that's what parseResumptionValue is supposed to do. But now it's taking care
> of a bunch of ES6 concerns with correcting forced return values for derived
> class constructors and I-don't-know-what, that require an AbstractFramePtr.
> 
> All of that extra garbage *must* be handled outside OnStepHandler, not
> within implementations of OnStepHandler::onStep. Whatever it is, it
> certainly looks like stuff which needs to be handled in exactly the same way
> no matter what sort of handler is being invoked.
> 
> As it stands, it seems like what OnStepHandlerFunction really wants to call
> is something simpler, like ParseResumptionValueAsObject. All the other
> handling in parseResumptionValue should take place in
> OnStepHandler::onStep's caller.
> 
> So we need to refactor parseResumptionValue before we can get this right.

I was working on a new version of the patch to implement a C++ interface for DebuggerFrame.onStep with your review comments addressed, but given your comments above, we should probably take a step back and briefly discuss over Vidyo how we want this code to behave before I continue.
Reporter

Updated

3 years ago
Attachment #8769657 - Attachment is obsolete: true

Comment 95

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6d54ce5fa4
DebuggerFrameVector should be a GCVector<DebuggerFrame*>. r=fitzgen

Comment 96

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9554d44cbc
Add GC typedefs for commonly used debugger types. r=fitzgen
Reporter

Comment 97

3 years ago
Try push for the patch to make DebuggerGenericEval return both trap status and value, to the patch to implement a C++ interface for DebuggerFrame.eval(WithBindings):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66f4d6d6d8c9
Reporter

Updated

3 years ago
Depends on: 1294013

Comment 99

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c3b20f9afa
DebuggerGenericEval should return both trap status and value. r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5962eff1ad
Move EvaluateInEnv and DebuggerGenericEval. r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/837ca629be91
Implement a C++ interface for DebuggerFrame.eval(WithBindings). r=jimb
Reporter

Comment 100

3 years ago
In bug 1294013, you said you wanted to hold off on reviewing the patch to factor our GetThisValueForCheck, because you expect Debugger::onSingleStep to use the same functions for checking and applying resumption values that other hook invocation spots do.

That's not exactly true: processHandlerResult gets the this value for the check and then passes that to processHandlerResultHelper. We don't want to use processHandlerResult, because we want to do parse the result ourselves in the on step handler. We then do the remaining steps such as unwrapping the return value and checking the resumption value ourselves. We have separate functions for each of these steps, but we don't have a separate function for getting the this value (remember, we can't use processHandlerValue).

I've attached a WIP patch to implement a C++ interface for DebuggerFrame.onStep. It's not quite ready for review yet, but hopefully it will help to make clear why we need the patch in bug 1288084. Let me know if you agree, so I can re-request review.
Attachment #8769661 - Attachment is obsolete: true
Attachment #8781150 - Flags: feedback?(jimb)
Assignee

Comment 102

3 years ago
Comment on attachment 8781150 [details] [diff] [review]
WIP patch to implement a C++ interface for DebuggerFrame.onStep.

Review of attachment 8781150 [details] [diff] [review]:
-----------------------------------------------------------------

The goal of the refactoring is to separate parsing of resumption values into (JSTrapStatus, wrapped Debuggee value) pairs, which is what Handler subclass methods like onStep should return, from the further processing. But there should still be a single function that handles all that further processing; the return value check-and-fix should still be internal to that function.

Remember that most handlers can return resumption values, so every one of those needs the same uncaught exception handling and return value checking/fixing. If you change Debugger::onSingleStep in the way proposed here, then all those handler invocation sites will need the same GetThisValueForCheck/unwrapDebuggeeValue/CheckResumptionValue/handleUncaughtException process you're writing out here. That's a step backwards.

Instead, processHandlerResult (originally parseResumptionValue) needs to continue to be the single call that a handler invocation site must make to get all that machinery. The only difference is that it should take a parsed resumption value (the pair), instead of the unparsed rv value it takes now.

Basically, you want another overload of processHandlerResult that expects the (JSTrapStatus, debuggee value) pair:

JSTrapStatus
Debugger::parseResumptionValue(Maybe<AutoCompartment>& ac, bool ok,
                               JSTrapStatus resumptionStatus, HandleValue resumptionV,
                               AbstractFramePtr frame, jsbytecode* pc, MutableHandleValue vp,
                               bool callHook)

and then the extant overload just calls ParseResumptionValue, and then calls the new overload.
Attachment #8781150 - Flags: feedback?(jimb) → feedback-
Reporter

Comment 103

3 years ago
(In reply to Jim Blandy :jimb from comment #102)
> Comment on attachment 8781150 [details] [diff] [review]
> WIP patch to implement a C++ interface for DebuggerFrame.onStep.
> 
> Review of attachment 8781150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The goal of the refactoring is to separate parsing of resumption values into
> (JSTrapStatus, wrapped Debuggee value) pairs, which is what Handler subclass
> methods like onStep should return, from the further processing. But there
> should still be a single function that handles all that further processing;
> the return value check-and-fix should still be internal to that function.
> 
> Remember that most handlers can return resumption values, so every one of
> those needs the same uncaught exception handling and return value
> checking/fixing. If you change Debugger::onSingleStep in the way proposed
> here, then all those handler invocation sites will need the same
> GetThisValueForCheck/unwrapDebuggeeValue/CheckResumptionValue/
> handleUncaughtException process you're writing out here. That's a step
> backwards.
> 
> Instead, processHandlerResult (originally parseResumptionValue) needs to
> continue to be the single call that a handler invocation site must make to
> get all that machinery. The only difference is that it should take a parsed
> resumption value (the pair), instead of the unparsed rv value it takes now.
> 
> Basically, you want another overload of processHandlerResult that expects
> the (JSTrapStatus, debuggee value) pair:
> 
> JSTrapStatus
> Debugger::parseResumptionValue(Maybe<AutoCompartment>& ac, bool ok,
>                                JSTrapStatus resumptionStatus, HandleValue
> resumptionV,
>                                AbstractFramePtr frame, jsbytecode* pc,
> MutableHandleValue vp,
>                                bool callHook)
> 
> and then the extant overload just calls ParseResumptionValue, and then calls
> the new overload.

Ok, that seems like a reasonable approach to me. New patch coming up tomorrow.
Reporter

Comment 104

3 years ago
I now have two helper functions for processing a handler result. The first one, processHandlerResultHelper, takes an unparsed handler result (that is, a success value and a resumption value). The second one, processParsedHandlerResultHelper, takes a parsed handler result (that is, a success value, a trap status, and a value to be returned).

The first helper function is used by the existing JS style hooks. It simply parses the handler result, and then forwards to the second helper function to do the rest of the processing. The new C++ style hooks can call the second helper function directly, since they always return parsed resumption values.

Neither of these helper functions can be called directly, because they also need a frame pointer and an optional this value to check the resumption value against. There are two ways to obtain the this value, so each helper function has two non-helper overloads: one that takes a this value directly, and one that takes a program counter that can be used to obtain the this value indirectly via GetThisValueForDebuggerMaybeOptimizedOut.

Since the code to obtain the this value via GetThisValueForDebuggerMaybeOptimizedOut is still duplicated between these two overloads, it might still be nice to factor that code out into GetThisValueForCheck, like I did before. On the other hand, these are now the only two functions between where this code is duplicated (as opposed to for every C++ style hook, as would have been the case in my original approach), so we could also leave it out.

Jim, are you happy with this approach? Or are there still things that you would like to be done differently?
Attachment #8781150 - Attachment is obsolete: true
Attachment #8781974 - Flags: feedback?(jimb)
Assignee

Comment 105

3 years ago
I just noticed the comment in Debugger.h that says:

    /*
     * When we run the onEnterFrame hook, the |this| slot hasn't been fully
     * initialized, because the initialzation happens in the function's
     * prologue. To combat this, we pass the this for the primitive return
     * check directly. When bug 1249193 is fixed, this overload should be
     * removed.
     */
    JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac, bool OK, const Value& rv,
                                      const Value& thisVForCheck, AbstractFramePtr frame,
                                      MutableHandleValue vp, bool callHook = true);

Bug 1249193 is marked RESOLVED FIXED, but Debugger::fireEnterFrame is still using this overload. What's the change needed to Debugger::fireEnterFrame to let us delete this overload?
Flags: needinfo?(shu)
Assignee

Comment 106

3 years ago
Asking because I think it will halve the number of overloads Eddy needs.
Assignee

Comment 107

3 years ago
Comment on attachment 8781974 [details] [diff] [review]
WIP patch to implement a C++ interface for DebuggerFrame.onStep (v2).

Review of attachment 8781974 [details] [diff] [review]:
-----------------------------------------------------------------

This looks more like it. Note my needinfo? above --- we may be able to drop a bunch of variations here.

::: js/src/vm/Debugger.cpp
@@ -2004,5 @@
>      // Call onStep for frames that have the handler set.
>      for (size_t i = 0; i < frames.length(); i++) {
> -        HandleDebuggerFrame frame = frames[i];
> -        if (frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined())
> -            continue;

nit: I don't think changing this no-handler check into an `if` around the handler call is a clarification. (I'm guessing that's the intent?) In the old version, it's easier to see that the loop iteration has no effect if the frame has no handler. You can read the rest of the loop body under the assumption that we're going to invoke a handler.
Attachment #8781974 - Flags: feedback?(jimb) → feedback+
(In reply to Jim Blandy :jimb from comment #105)
> I just noticed the comment in Debugger.h that says:
> 
>     /*
>      * When we run the onEnterFrame hook, the |this| slot hasn't been fully
>      * initialized, because the initialzation happens in the function's
>      * prologue. To combat this, we pass the this for the primitive return
>      * check directly. When bug 1249193 is fixed, this overload should be
>      * removed.
>      */
>     JSTrapStatus parseResumptionValue(mozilla::Maybe<AutoCompartment>& ac,
> bool OK, const Value& rv,
>                                       const Value& thisVForCheck,
> AbstractFramePtr frame,
>                                       MutableHandleValue vp, bool callHook =
> true);
> 
> Bug 1249193 is marked RESOLVED FIXED, but Debugger::fireEnterFrame is still
> using this overload. What's the change needed to Debugger::fireEnterFrame to
> let us delete this overload?

It can be deleted now. We forgot about it after bug 1249193 landed. The original issue was that to accommodate classes, the 'this' binding is now initialized in bytecode and treated like a var binding, but this meant the 'this' value was uninitialized on the frame in onEnterFrame. Bug 1249193 added special logic to initialize the 'this' binding should a frame be inspected before any code has executed.

I removed that overload locally, passing in |frame.script()->code()|, i.e., the first bytecode, to parseResumptionValue, and all debug tests pass.
Flags: needinfo?(shu)
Assignee

Comment 109

3 years ago
Okay, thanks very much. So it's now possible to derived the `this` value from a (frame, pc) pair just as all the other parseResumptionValue sites do.

Eddy, if we could get this small patch landed, it would simplify your patch nicely!
Reporter

Comment 110

3 years ago
This patch removes one of the two overloads of processHandlerResultHelper, as suggested by Jim, and merged the remaining helper function back into processHandlerResult.
Attachment #8783513 - Flags: review?(jimb)
Reporter

Comment 111

3 years ago
This patch adds a new version of processHandlerResult, that can be used by those callers that already have a parsed version of the handler result, and which I've named processParsedHandlerResult.

Note that I've introduced a helper function, processParsedHandlerResultHelper, that is called by both processParsedHandlerResult and processHandlerResult. Both functions first obtain the this value to be used to to check the resumption value. In addition, the latter then parses the unparsed handler result. Finally, both functions pass the parsed handler and the this value for the check to the helper function.
Attachment #8783514 - Flags: review?(jimb)
Reporter

Comment 112

3 years ago
This seems about ready for review. Essentially the same patch on which you gave feedback on, but with comments added, and some of the work split off into the patches I r?'d you on earlier.
Attachment #8781974 - Attachment is obsolete: true
Attachment #8783535 - Flags: review?(jimb)
Reporter

Comment 113

3 years ago
Review ping.
Assignee

Comment 114

3 years ago
Thanks for the ping!
Assignee

Comment 115

3 years ago
Comment on attachment 8783513 [details] [diff] [review]
Remove processHandlerResultHelper.

Review of attachment 8783513 [details] [diff] [review]:
-----------------------------------------------------------------

The changes to processHandlerResult and processHandlerResultHelper look great.

But why are there changes to slowPathOnEnterFrame and fireEnterFrame here? Those are extraneous to the patch. Constructing a ScriptFrameIter is not terribly cheap; it may involve reading Ion frame snapshots and various other bits of hair.
Attachment #8783513 - Flags: review?(jimb)
Assignee

Comment 116

3 years ago
Comment on attachment 8783514 [details] [diff] [review]
Factor out processParsedHandlerResult(Helper).

Review of attachment 8783514 [details] [diff] [review]:
-----------------------------------------------------------------

I feel like this is more complicated than it needs to be, but it seems like it should work.
Attachment #8783514 - Flags: review?(jimb) → review+
Assignee

Comment 117

3 years ago
Comment on attachment 8783535 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.onStep.

Review of attachment 8783535 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +2030,5 @@
>  #endif
>  
>      // Call onStep for frames that have the handler set.
>      for (size_t i = 0; i < frames.length(); i++) {
> +        RootedDebuggerFrame frame(cx, frames[i]);

Is there a reason this changes the HandleDebuggerFrame to a RootedDebuggerFrame?

@@ -2031,5 @@
>      // Call onStep for frames that have the handler set.
>      for (size_t i = 0; i < frames.length(); i++) {
> -        HandleDebuggerFrame frame = frames[i];
> -        if (frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined())
> -            continue;

The nit I mentioned in comment 107 is still here; is that on purpose?
Attachment #8783535 - Flags: review?(jimb) → review+
Reporter

Comment 118

3 years ago
(In reply to Jim Blandy :jimb from comment #115)
> Comment on attachment 8783513 [details] [diff] [review]
> Remove processHandlerResultHelper.
> 
> Review of attachment 8783513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes to processHandlerResult and processHandlerResultHelper look
> great.
> 
> But why are there changes to slowPathOnEnterFrame and fireEnterFrame here?
> Those are extraneous to the patch. Constructing a ScriptFrameIter is not
> terribly cheap; it may involve reading Ion frame snapshots and various other
> bits of hair.

We removed the version of processHandlerResult that takes an AbstractFramePtr and a JSValue, and replaced it with the version that takes a AbstractFramePtr and a jsbytecode* (from which we can then obtain the JSValue).

My assumption was that the only way to obtain an AbstractFramePtr *and* a jsbytecode* is with a ScriptFrameIter. Is this not the case? If so, please let me know what the correct approach should be.

On the other hand, if this approach is correct, we no longer need to pass an AbstractFramePtr to fireEnterFrame, because we already obtain it from the ScriptFrameIter. Note that this is consistent with what the other fire* methods do.
Flags: needinfo?(jimb)
Assignee

Comment 119

3 years ago
Okay, I see: the patch just makes fireEnterFrame look just like all the other hook invocations, that create a ScriptFrameIter and use that to find the Debugger.Frame and pc to pass to processHandlerResult. Okay, now it all makes more sense - thanks for the explanation.
Flags: needinfo?(jimb)
Assignee

Updated

3 years ago
Attachment #8783513 - Flags: review+
Reporter

Comment 120

3 years ago
Try push for the following patches:
- Remove processResumptionValueHelper.
- Factor our processParsedResumptionValue(Helper).
- Implement a C++ interface for DebuggerFrame.onStep.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49cccb98001
Reporter

Comment 122

3 years ago
Previous try patch had the following issues:
- Compile errors in browser builds due to missing 'explicit' keyword.

New try push for the following patches, with above issues addressed:
- Remove processResumptionValueHelper.
- Factor our processParsedResumptionValue(Helper).
- Implement a C++ interface for DebuggerFrame.onStep.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f3e53a3f53a
Assignee

Comment 123

3 years ago
Comment on attachment 8787598 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.onPop

Review of attachment 8787598 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +905,2 @@
>              {
> +                OnPopHandler* handler = frameobj->onPopHandler();

There's something that makes me a little nervous here - and it applies to the onStep handler as well. This is a non-owning pointer to the handler; if anything stores a new handler on the frame, this onPopHandler will be destructed. This didn't happen with the old code, because we stored the handler in our very own RootedValue.

I can't see anything between here and the use of `handler` that could change it, though, so I guess it's okay. It's just a little precarious. It'd make me happier if we actually just dropped the `handler` variable entirely, and called it like this:

    bool success = frameobj->onPopHandler()->onPop(...)

@@ +7794,5 @@
> +    if (onStepHandler) 
> +        onStepHandler->trace(trc);
> +    OnPopHandler* onPopHandler = obj->as<DebuggerFrame>().onPopHandler();
> +    if (onPopHandler) 
> +        onPopHandler->trace(trc);

FWIW, the cool kids these days just say:

    if (OnPopHandler* onPopHandler = obj->as<DebuggerFrame>().onPopHandler())
        onPopHandler->trace(trc);

If you like that style, you could use it for onStepHandler, and in _finalize as well.

@@ +8251,5 @@
> +
> +    OnPopHandler* handler = frame->onPopHandler();
> +    RootedValue value(cx, handler ? ObjectValue(*handler->object()) : UndefinedValue());
> +    MOZ_ASSERT(IsValidHook(value));
> +    args.rval().set(value);

Does this mean that we get JS `null` if there's a non-Scripted handler installed, but JS `undefined` if there's no handler at all? Isn't this going to fail the IsValidHook assertion?

It seems to me we should either return some JS value that really represents the (non-Scripted) handler, so you could save it, replace it with a JS function, and then put the original back and get back the same handler as before --- which would be a lot of work for something I really doubt anyone's going to want to do --- or we should just make non-Scripted handlers look like no handler at all (i.e. undefined) to JS.

::: js/src/vm/Debugger.h
@@ +1241,5 @@
> +};
> +
> +class ScriptedOnPopHandler final : public OnPopHandler {
> +  public:
> +    ScriptedOnPopHandler(JSObject* object);

This is going to need an 'explicit' keyword, right?
Attachment #8787598 - Flags: review?(jimb) → review+

Comment 124

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0ecfb7ad9c
Remove processResumptionValueHelper. r=jimb

Comment 125

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f82d03a8de0c
Factor our processParsedResumptionValue(Helper).

Comment 126

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb39c7c98dd
Implement a C++ interface for DebuggerFrame.onStep.
(In reply to Pulsebot from comment #126)
> Pushed by ejpbruel@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb39c7c98dd
> Implement a C++ interface for DebuggerFrame.onStep.

I had to back this out for ASAN leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=35865989&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/056e578d03bb
Flags: needinfo?(ejpbruel)
Whiteboard: [devtools-html]
Assignee

Comment 128

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #127)
> I had to back this out for ASAN leaks

Thanks, Wes!

I wish I'd caught whatever the problem is when I reviewed. Eddy, I'm very curious to know what the problem was once you've analyzed; if you could post for re-review, that'd be great.
Reporter

Comment 130

3 years ago
I'm having a hard time actually analysing the ASAN leak reported in comment 127, because I cannot reproduce it locally.

I managed to create an ASAN build in my Linux VM, but running it against the failing mochitest (i.e. browser_dbg_breakpoints-reload.js) causes it to crash with the following stack trace:
https://pastebin.mozilla.org/

Note that this happens even without the failing patch applied. I don't know if this is a problem with my VM, with my ASAN build, or with something else.
Flags: needinfo?(ejpbruel)
Reporter

Comment 131

3 years ago
Here's a try push for the fix I proposed in comment 130:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e99df4fbe6&selectedJob=28067489

The previous ASAN failures are gone, but they are replaced with use-after-free errors. I don't understand how these can happen: if the prior onStep handler is dropped, then the new handler must be null. Since we always replace the onStep handler slot with the new handler after that, the prior handler should no longer be accessible after it has been freed.
Assignee

Comment 132

3 years ago
Comment on attachment 8783535 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.onStep.

Review of attachment 8783535 [details] [diff] [review]:
-----------------------------------------------------------------

Some further comments on this. Very sorry for dropping the ball in the initial review.

::: js/src/vm/Debugger.cpp
@@ +7152,5 @@
> +
> +void
> +ScriptedOnStepHandler::drop()
> +{
> +}

As your try push shows, this isn't right. This is the only possible place the memory allocated by the `cx->new_<ScriptedOnStepHandler>` call in onStepSetter could possibly be freed, but we don't do it.

@@ +7409,5 @@
>      return DebuggerFrameImplementation::Interpreter;
>  }
>  
> +/* static */ bool
> +DebuggerFrame::setOnStepHandler(JSContext* cx, HandleDebuggerFrame frame, OnStepHandler* handler)

Although we're not documenting the C++ API, I think we still need a comment here that this function promises to either:
1) succeed and take ownership of `handler`, or
2) fail and leave ownership of `handler` in the hands of the caller
but never anything else.

@@ +8129,5 @@
>  {
> +    THIS_DEBUGGER_FRAME(cx, argc, vp, "get onStep", args, frame);
> +
> +    OnStepHandler* handler = frame->onStepHandler();
> +    RootedValue value(cx, handler ? ObjectValue(*handler->object()) : UndefinedValue());

For what it's worth, this assertion is going to fire the first time we ever have a non-scripted onStepHandler. Its `object` method will return nullptr, which ObjectValue will barf on.

@@ +8155,4 @@
>      }
>  
> +    if (!DebuggerFrame::setOnStepHandler(cx, frame, handler))
> +        return false;

Demonstrating the importance of that comment on setOnStepHandler, this function leaks `handler` if this call fails.
Reporter

Comment 133

3 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #131)
> Here's a try push for the fix I proposed in comment 130:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=97e99df4fbe6&selectedJob=28067489
> 
> The previous ASAN failures are gone, but they are replaced with
> use-after-free errors. I don't understand how these can happen: if the prior
> onStep handler is dropped, then the new handler must be null. Since we
> always replace the onStep handler slot with the new handler after that, the
> prior handler should no longer be accessible after it has been freed.

As it turns out, the use-after-free error happened because even though we freed the ScriptedOnStepHandler, we never called its destructor in that patch. ScriptedOnStepHandler stores a JSObject in a HeapPtr. If we don't call the destructor for that HeapPtr, the GC can end up trying to access the HeapPtr after we freed the ScriptedOnStepHandler, thus causing the use-after-free bug.

I've resolved that problem, and also addressed Jim's additional review comments in comment 132. Here is a try push for the new patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d44498feceb
Assignee

Comment 134

3 years ago
This patch makes me want to turn in my badge.

Comment 135

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddeaf72c3abe
Implement a C++ interface for DebuggerFrame.onStep. r=jimb
Reporter

Comment 136

3 years ago
Try push for the patch to implement a C++ interface for DebuggerFrame.onPop:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4923e3a77574e38dc8fc2efed2ddf7c86ca68e2b
Reporter

Comment 140

3 years ago
Note that this patch depends on the patch in bug 1271654.
Attachment #8814935 - Flags: review?(jimb)

Comment 141

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3da8afdc2497
Implement a C++ interface for DebuggerFrame.onPop. r=jimb
Assignee

Comment 143

3 years ago
Comment on attachment 8814915 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.getArguments.

Review of attachment 8814915 [details] [diff] [review]:
-----------------------------------------------------------------

Could I see this one again, with these revisions made?

::: js/src/vm/Debugger.cpp
@@ +7571,5 @@
> +                            MutableHandleDebuggerArguments result)
> +{
> +    Value argumentsv = frame->getReservedSlot(JSSLOT_DEBUGFRAME_ARGUMENTS);
> +    if (!argumentsv.isUndefined()) {
> +        result.set(argumentsv.isObject() ? &argumentsv.toObject().as<DebuggerArguments>() : nullptr);

If I'm reading right, dropping the MOZ_ASSERT here silently turns bogus slot values into nullptr. Unless I've misread, let's retain the MOZ_ASSERT from the original.

@@ +7575,5 @@
> +        result.set(argumentsv.isObject() ? &argumentsv.toObject().as<DebuggerArguments>() : nullptr);
> +        return true;
> +    }
> +
> +    AbstractFramePtr referent = DebuggerFrame::getReferent(frame);

Before this patch, the code does the equivalent of getReferent, written out in the THIS_FRAME macro's expansion, once. With this patch applied, the code does it twice: once here, and then again in DebuggerArguments::create. Unfortunately, the getReferent operation isn't super-cheap: if you look at the definition of FrameIter::FrameIter(const Data& data) (linked), it sometimes has to construct a js::jit::InlineFrameIterator and then walk it in a few frames, which entails decoding IonMonkey snapshots, which can be expensive.

http://searchfox.org/mozilla-central/rev/0c055ccbcf96846044fc9a71421bd9b7978686f7/js/src/vm/Stack.cpp#633

There are argument both ways, but I think DebuggerArguments::create should take an argument count, in addition to the Debugger.Frame. That seems to be the only thing it actually needs from the referent. Then we can extract that here, given the AbstractFramePointer we already have, and just pass it in.

@@ +7579,5 @@
> +    AbstractFramePtr referent = DebuggerFrame::getReferent(frame);
> +
> +    RootedDebuggerArguments arguments(cx);
> +    if (referent.hasArgs()) {
> +        Rooted<GlobalObject*> global(cx, &frame->global());

Could you add the comment:

// Allocate our DebuggerArguments object in the same compartment as its Debugger.Frame object.

The "frame->global()" expression freaked me out for a bit, because variables named `frame` sometimes refer to the stack frame itself, which is, of course, in the wrong compartment for this. But here it refers to the D.F, which is correct.

@@ +8175,2 @@
>  {
> +    AbstractFramePtr referent = DebuggerFrame::getReferent(frame);

Let's add, just inside the opening brace:
assertSameCompartment(cx, frame, proto);

and then, after the declaration of referent:
MOZ_ASSERT(referent.hasArgs());
Attachment #8814915 - Flags: review?(jimb) → review-
Reporter

Comment 144

3 years ago
New patch with comments by jimb addressed.
Attachment #8814915 - Attachment is obsolete: true
Attachment #8816474 - Flags: review?(jimb)
Assignee

Comment 145

3 years ago
Comment on attachment 8816474 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.arguments (v2).

Review of attachment 8816474 [details] [diff] [review]:
-----------------------------------------------------------------

This patch seems to have been combined with "Implement a C++ interface for DebuggerArgument.getArgument", but that's fine, I'll review them both together.

The DebuggerArguments object should probably have a `length` method as well.
Assignee

Comment 146

3 years ago
Comment on attachment 8816474 [details] [diff] [review]
Implement a C++ interface for DebuggerFrame.arguments (v2).

Review of attachment 8816474 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +8186,2 @@
>  
> +    SetReservedSlot(obj, JSSLOT_DEBUGARGUMENTS_FRAME, ObjectValue(*frame));

Shouldn't this be, simply, FRAME_SLOT?
Attachment #8816474 - Flags: review?(jimb) → review+
Assignee

Comment 147

3 years ago
(In reply to Jim Blandy :jimb from comment #145)
> This patch seems to have been combined with "Implement a C++ interface for
> DebuggerArgument.getArgument", but that's fine, I'll review them both
> together.

Oh, just Debugger.h was folded in, I guess.
Assignee

Comment 148

3 years ago
Comment on attachment 8814917 [details] [diff] [review]
Implement a C++ interface for DebuggerArgument.getArgument

Review of attachment 8814917 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +8111,3 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    int32_t index = args.callee().as<JSFunction>().getExtendedSlot(0).toInt32();

If I had my druthers, I would rather get renames as separate patches. For your own sake, I think I'm faster at reviews when there are fewer lines changed due to renames and code motion.

@@ +8119,2 @@
>          return false;
> +    if (thisobj->getClass() != &DebuggerArguments::class_) {

I think you can write this:

    if (!thisobj->is<DebuggerArguments>()) {

@@ +8136,5 @@
>      RootedNativeObject obj(cx, NewNativeObjectWithGivenProto(cx, &DebuggerArguments::class_, proto));
>      if (!obj)
>          return nullptr;
>  
> +    SetReservedSlot(obj, DebuggerArguments::FRAME_SLOT, ObjectValue(*frame));

You can just write FRAME_SLOT here, no need for DebuggerArguments::.

@@ +8150,5 @@
>  
>      Rooted<jsid> id(cx);
>      for (unsigned i = 0; i < fargc; i++) {
>          RootedFunction getobj(cx);
> +        getobj = NewNativeFunction(cx, DebuggerArguments::getArgumentMethod, 0, nullptr,

Similarly: no need for DebuggerArguments::.
Attachment #8814917 - Flags: review?(jimb) → review+
Assignee

Updated

3 years ago
Attachment #8814935 - Flags: review?(jimb) → review+
Assignee

Updated

3 years ago
Depends on: 1321186

Comment 151

3 years ago
Pushed by ejpbruel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b58022dda33
Implement a C++ interface for DebuggerFrame.arguments. r=jimb
Reporter

Updated

3 years ago
Assignee: ejpbruel → nobody
Depends on: 1335533
Jim, what's next here?
Flags: needinfo?(jimb)
Assignee

Comment 154

2 years ago
I'll take over finishing up this bug.

It seems that some of the patches written here have not landed. There may also be remaining work yet to be done.

From a preliminary look, it seems that the following patches above haven't been landed:

Implement a C++ interface for DebuggerFrame.getType.
Stack trace of Debugger.Frame getting its ScriptFrameIter::Data freed.
Remove processHandlerResultHelper.
Factor out processParsedHandlerResult(Helper).
Implement a C++ interface for DebuggerArgument.getArgument
Implement a C++ interface for DebuggerFrame.script.

These may have been subsumed by other patches; I'll look into this in more detail tomorrow.
Assignee: nobody → jimb
Flags: needinfo?(jimb)
Assignee

Comment 155

2 years ago
It looks like this is almost done. All that's missing is a C++ getter for Debugger.Frame.prototype.script.

Rather than looking through the patches in the bug, I looked through the code to see which Debugger.Frame properties and methods lacked corresponding well-typed methods in the js::DebuggerFrame C++ type.
Assignee

Comment 156

2 years ago
Attachment 8814935 [details] [diff] implements a C++ analogue to the `script` accessor, and is blocked on bug 1271654, a C++ API for Debugger.Script, which has not landed.
Priority: P1 → P3
The leave-open keyword is there and there is no activity for 6 months.
:jimb, maybe it's time to close this bug?
Flags: needinfo?(jimb)
Assignee

Comment 158

6 months ago
Thanks for the nudge, Release mgmt bot!
Status: REOPENED → RESOLVED
Closed: 3 years ago6 months ago
Flags: needinfo?(jimb)
Resolution: --- → INACTIVE
Assignee

Comment 159

6 months ago
I do intend to finish this at some point, though.
You need to log in before you can comment on or make changes to this bug.