537199 - document jsdIDebuggerService.pauseDepth changes by the platform

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[John J. Barton]

The current API documentation gives the strong impression that jsd.pauseDepth is controlled by jsd.pause() and jsd.unpause(). No documentation on jsd that I have read in my 2 years on Firebug gave me any idea that the platform may also change this value. See:
http://mxr.mozilla.org/mozilla/source/js/jsd/idl/jsdIDebuggerService.idl#246
 
Boris told me and I confirmed experimentally that the platform changes the pauseDepth. It seems to add 2 for a call to enterNestedEventLoop().

Comment 1

[Boris Zbarsky [:bzbarsky]]

"The platform" just calls pause() and and unpause(), so your impression is correct.  It's just your assumption that "you" are the only caller of pause() and unpause() that's incorrect.

So I'm not quite sure what there is to document...  You just shouldn't be assuming that you're the only caller of particular XPCOM API methods.  I don't see why such an assumption would ever be reasonable...

Comment 2

[Boris Zbarsky [:bzbarsky]]

In all honesty, I have no idea why the pauseDepth API exists at all.  It's not particularly useful; in particular I can't think of any way someone could use it at all without making exactly the assumption that they're the only debugger service consumer and the only caller of any debugger service methods...

Comment 3

[John J. Barton]

Which ever assumption I made that is incorrect, I made based on the documentation (or lack of).

Comment 4

[Boris Zbarsky [:bzbarsky]]

I really don't see how the documentation can keep you from making this assumption.  Should every single method on every single interface be documented as "just because you call this method doesn't mean that no one else does"?

Seriously, I'm happy to improve the documentation; I just don't see how.  Any suggestions?

Comment 5

[John J. Barton]

I suggest that the documentation for enterNestedEventLoop: 
http://mxr.mozilla.org/mozilla/source/js/jsd/idl/jsdIDebuggerService.idl#375
change from:
  Push a new network queue, and enter a new UI event loop.
to
  Push a new network queue, enter a new UI event loop, and call pause() to disable hooks.

and 
http://mxr.mozilla.org/mozilla/source/js/jsd/idl/jsdIDebuggerService.idl#384

   Exit the current nested event loop after the current iteration completes,
   and pop the network event queue.
to
   Exit the current nested event loop after the current iteration completes,
   pop the network event queue, and call unpause() to match the calls in
   enterNestedEventLoop().

and for pause()
http://mxr.mozilla.org/mozilla/source/js/jsd/idl/jsdIDebuggerService.idl#253

  Temporarily disable the debugger.  Hooks will not be called while the
  debugger is paused.  Multiple calls to pause will increase the "pause
  depth", and equal number of unPause calles must be made to resume
  normal debugging.
to
  Temporarily disable the debugger.  Hooks will not be called while the
  debugger is paused.  Multiple calls to pause will increase the "pause
  depth", and equal number of unPause calls must be made to resume
  normal debugging. enterNestedEventLoop() increases the pause depth; 
  exitNestedEventLoop() decreases it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Push a new network queue, enter a new UI event loop, and call pause() to
> disable hooks.

That's not what happens, though.  EnterNestedEventLoop doesn't pause anything by itself; it only pauses around the call to the callback.  Is that what you're observing?  The debugger pauses itself before calls into ANY debugger callback, and unpauses after.  It sort of has to, if you think about it: you don't want it debugging the callback code, after all.

Comment 7

[John J. Barton]

Ok now we know even more, that's great. 

pause()
http://mxr.mozilla.org/mozilla/source/js/jsd/idl/jsdIDebuggerService.idl#253
from:
  Temporarily disable the debugger.  Hooks will not be called while the
  debugger is paused.  Multiple calls to pause will increase the "pause
  depth", and equal number of unPause calles must be made to resume
  normal debugging.
to
  Temporarily disable the debugger.  Hooks will not be called while the
  debugger is paused.  Multiple calls to pause will increase the "pause
  depth", and equal number of unPause calls must be made to resume
  normal debugging. The debugger service itself calls pause() before 
  calling any hook or other callback and calls unpause() upon return.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment 7 sounds fine to me.  Do you want to just write up a patch as long as you're here?  Or want me to do it?

Comment 9

[John J. Barton]

I can do it next week when I am on a machine with m-c source loaded.

Comment 10

[timeless]

I'm not quite sure I like this comment change. (Actually, I'm fairly certain I dislike it).

A more accurate set of statements follow:
 "You are required to call unpause() once for each call to pause() you make." 
 "This method like all other public methods might be called by others, as such you should not assume that merely because pauseLevel was 1 and you called unpause that the pauseLevel will reach 0 and that the debugger will thus be unpaused."

 As a comment to pauseLevel():
 The pauseLevel() during a callback will be one greater than the natural pauseLevel because the debugger is paused while it makes calls to callbacks. You can *not* assert that it will in fact be one, it might be some other number.

Comment 11

[timeless]

Perhaps a more useful tidbit:

It is *NOT* the job of an IDL file to document how an IMPLEMENTATION should call one of its own public functions to effect the result of another function.

It is *ALMOST* ok for an IDL file to document the state of a given function during another call. But only if we actually believe that it will be true for ALL implementations of an interface.

Please do remember that IDL files define interfaces, they are not equivalent to contracts against a specific implementation.

While it is often the case that there is only one implementation of a given interface, that doesn't mean it is always true or that the implementor wishes to be bound by things not currently written into the interface.

Comment 12

[John J. Barton]

While in general I agree with your concerns, the practical facts here are different.  There is only one implementation of this interface. The only "others" here is the implementation itself. (Efforts to use this interface in two extensions fail). The goal here is just a bit more documentation on a puzzling and arcane system, not a contract.

Comment 13

[Bryan McQuade]

Many clients of the jsdIDebuggerService do want/need exclusive access to it.

For instance, the Page Speed Activity profiler expects to get OnCall() callbacks for each function entry/exit.

When multiple clients of jsdIDebuggerService are active at the same time, this expectation may no longer hold. Another client might pause() the jsdIDebuggerService causing callbacks to stop getting called. Likewise another client might add unexpected jsdIFilters.

Because there's no easy way to communicate between jsdIDebuggerService clients, when Page Speed Activity starts up we unwind the pause depth to zero and remove all filters. I'd prefer to not do this but I don't have a choice. Page Speed Activity needs to be the exclusive controller of the jsdIDebuggerService so we just reset the jsdIDebuggerService state during set up, and try to restore that state on tear-down. This is of course very fragile but it's the best we can do.

It would be nice if jsdIDebuggerService exposed an API to acquire the jsdIDebuggerService. This acquisition could be informational only to help prevent multiple clients from stepping on each others' toes. Here's how I see it working:

var jsd = ...;  // get a handle to the jsd
var myName = 'Page Speed Activity';
var ownerName = jsd.tryAcquire(myName);
if (ownerName != myName) {
  alert('Client ' + ownerName + ' is currently using the JSD. Please disable that client in order to continue.');
  return;
}
try {
... we hold the "lock" ...
} finally {
  // we've released the "lock"
  jsd.release(myName);
}

tryAcquire() should also reset the jsdIDebuggerService state (clear all filters, reset pause depth, etc). release() could also reset the state.

None of the existing jsdIDebuggerService methods would assert that the caller is the one holding the lock. It's just the expectation of jsdIDebuggerService clients that you only muck with the jsdIDebuggerService state if you are the one holding the lock. It's up to the clients to be well behaved about this. Clients that were written before the new tryAcquire/release API was added would continue to work, they just wouldn't be well behaved since they wouldn't use this API.

If a client fails to call release(), this could be potentially problematic (jsdIDebuggerService is held forever), but that'd be a bug in the jsdIDebuggerService client which would need to be fixed.

John, would this kind of API be useful to you in Firebug?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Given that nowadays the function hook perturbs the performance of the JS engine to the point where the results have little bearing on real performance, it might be more worthwhile to figure out how the new profiling APIs being worked on (bug 507012) should interact with the use cases you have than trying to polish the already-broken jsd behavior.

Comment 15

[John J. Barton]

Bryan, as a practical matter we will have to deal with this ourselves.