1047655 - Debugger.prototype.enabled should be deleted

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Each Debugger instance has an 'enabled' accessor; setting it to false is supposed to deactivate the Debugger, ensuring that it has no effects on anything's performance, etc.

In practice, we don't really use this much. What we *do* do, and which seems to have all the same effects, is add and remove debuggees. It's not clear why we need to have the complexity of |.enabled = false| and enabled tests everywhere, when we can just do |.removeAllDebuggees()|.

Comment 1

[Tom Tromey :tromey]

I wrote a patch for this and ran into one issue that I think needs your feedback.

Debugger.md has this to say about 'enabled':

-    This property gives debugger code a single point of control for
-    disentangling itself from the debuggee, regardless of what sort of
-    events or handlers or "points" we add to the interface.

Now consider the test case Debugger-onNewGlobalObject-07.js.  In an onNewGlobalObject
callback, one debugger will disable some other debuggers.  The straightforward approach
of changing "dbg1.enabled = false" to "dbg1.removeAllDebuggees()" does not work, because
onNewGlobalObject is not affected by removeAllDebuggees -- nor should it be, as that wouldn't
make sense.

This also affects Frame-onPop-multiple-03.js, which is the same issue but involving onEnterFrame --
leading me to think that perhaps the original rationale for 'enabled' was justified.

So, one option would be to just leave things as they are and close this bug.  Another option
would be to say that if one debugger wants to disable another one, it must know all the
things to tweak; in this case I think I can just delete the affected tests and continue on with
the patch.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

^

Comment 3

[Jim Blandy :jimb]

Well, rats. I guess onNewGlobalObject really is something that removing debuggees doesn't neuter. I hadn't thought of that.

It doesn't change the fact that D.p.enable is *almost* useless, yet must be checked all over, nor that debuggees get de-optimized even when it's cleared. (Although bug 1032869 will fix that last problem.)

Perhaps what would be just as good, in light of bug 1032869, would be to instead have a 'disable()' method, that clears all hooks on the Debugger and Debugger.Blah instances, as well as any hook-like things like breakpoint callbacks. As before, there'd be no more flag to check everywhere; but it would clearly be able to kill the Debugger. Since landing the fix in bug 1032829 will mean that a debugger that doesn't have any hooks set won't deoptimize anything, there'll be no need to clear the debuggee set.

Comment 4

[Tom Tromey :tromey]

I've been digging in toolkit/devtools as there's some code
there that uses 'enabled'.  For the most part I think it's straightforward to update.
However, server/actor/utils/make-debugger.js sets the "onNewGlobalObject" hook:

  dbg.onNewGlobalObject = global => {
    if (shouldAddNewGlobalAsDebuggee(global)) {
      safeAddDebuggee(dbg, global);
    }
  };

... and it seems to me that at least server/script.js can bring up the Debugger in the
enabled=false state and then later transition it to enabled=true.  I think other code
(tracer.js perhaps) can also both enable and disable the debugger this way.

This is a problem because the change needed for this bug requires clearing the onNewGlobalObject
hook when disabling the debugger, whereas '.enabled = false' leaves the hook's value in place.

My current plan is to add a new 'enable' method to the object returned by makeDebugger.
I think this should work fine, though it seems like it is just moving "enabled" from one
layer to another -- though perhaps the resulting implementation will be clearer.
However, note that removing "enabled" requires this change:

@@ -555,7 +551,7 @@ Debugger::slowPathOnLeaveFrame(JSContext *cx, AbstractFramePtr frame, bool frame
         RootedObject frameobj(cx, *p);
         Debugger *dbg = Debugger::fromChildJSObject(frameobj);
 
-        if (dbg->enabled &&
+        if (dbg->debuggees.has(global) &&
             !frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER).isUndefined()) {
             RootedValue handler(cx, frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER));
 
This is needed to make Frame-onPop-multiple-03.js work after ".enabled = false" is changed
to ".disable()".

I think this undermines the rationale for the change a little bit, so I thought I would check
in to see what you think.  It's worth noting that similar checks are already
in place along other code paths, so this is the only such addition; at least assuming that
my current approach of "clear onNewGlobalObject but leave the other hooks intact" actually works.

I think what I'm looking for is "go ahead, that sounds good" or "let's drop it" or "you forgot
cases X Y Z".

Comment 5

[Jim Blandy :jimb]

I think we should drop it for now. The only reason for doing the change would be to simplify js/src/vm/Debugger.cpp, so if we're just piling more stuff on that, there's no win. 

The natural progression here would be:

- Implement Debugger.prototype.disable;
- Make devtools use that, and remove all uses of Debugger.prototype.enable;
- Delete Debugger.prototype.enable.

Comment 6

[Tom Tromey :tromey]

Ok, thanks.  I will attach the current version of my patch to remove "enable"
and work on something else.

Comment 7

[Tom Tromey :tromey]

Attachment: remove Debugger.enabled and add Debugger.disable()

Comment 8

[Logan Smyth [:loganfsmyth]]

This was done last year.