920281 - Set breakpoints on source actors, not thread actors

Status: RESOLVED DUPLICATE of bug 905700

(DevTools :: Debugger, defect, P2)

Comment 1

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

(Woops, hit RETURN by accident)

In the remote debugging protocol, we should send 'setBreakpoint' requests to source actors instead of thread actors.

Comment 2

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

Protocol proposal:

Send

{
  to: sourceActorID,
  type: "setBreakpoint",
  location: {
    line: lineNumber,
    column: optionalColumnNumber
  }
}

receive:

{
  from: sourceActorID,
  actor: breakpointActorID,
  actualLocation: optionalActualLocation
}

Basically the same as now but without a url in location, and to a source actor.

Protocol compatibility:

* Should be fairly easy to work into our protocol compatibility shim layer:

** Move ThreadClient.prototype.setBreakpoint to SourceClient.prototype.setBreakpoint.

** If the source client has a URL, attach that to the packet's location, and send the request to the thread actor instead of the source actor.

** If the source does NOT have a URL, do nothing (or just callback with a mock error packet) because the server can't set breakpoints in these kinds of sources anyways. Perhaps we will never run into this case either way because the client won't receive newSource notifications for these sources.

Comment 3

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

I'm unassigning myself because I won't be working on this anytime soon.

Comment 4

[James Long (:jlongster)]

Attachment: 920281.patch

This is an initial pass to get this working. I'm not sure how easy it's going to be to spread these patches across bugs (here and bug 917579), so it might be worth eventually tracking all patches on bug 905700. But I'm posting this to get early feedback in small chunks, instead of posting a huge amount of code later.

This moves `onSetBreakpoint` (and all supporting functions) to the SourceActor, and refactors it to use Task.async. I've started converting some of the tests, and they pass (but not on the worker thread, need to convert Task to work there which will happen soon).

This also adds a few things to support talking in terms of sources instead of urls, such as keeping a mapping of SourceActor to Debugger.Source in ThreadSources.

I think this mostly completes this bug, and the next thing I'll focus on is bug 917579.

Comment 5

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

(In reply to James Long (:jlongster) from comment #4)
> Created attachment 8445349 [details] [diff] [review]
> 920281.patch
> 
> This is an initial pass to get this working. I'm not sure how easy it's
> going to be to spread these patches across bugs (here and bug 917579), so it
> might be worth eventually tracking all patches on bug 905700. But I'm
> posting this to get early feedback in small chunks, instead of posting a
> huge amount of code later.

Yeah I suspected we might have to do all the work in one bug, land it all at once, and then just close the others as DUPs once everything gets to m-c.

Many smaller patches are easier to review than one big patch, even if we do end up doing it all in one bug. I appreciate the work that you have to do to do that ;)

> This moves `onSetBreakpoint` (and all supporting functions) to the
> SourceActor, and refactors it to use Task.async. I've started converting
> some of the tests, and they pass (but not on the worker thread, need to
> convert Task to work there which will happen soon).

What was the resolution regarding Task + workers? Are we going to just delay landing this? Do we have bugs that this one needs to depend on?

Also, please submit a PR to https://github.com/jimblandy/DebuggerDocs with the protocol updates.

Comment 6

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

Comment on attachment 8445349 [details] [diff] [review]
920281.patch

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

Would love to see the tests use Task.jsm and the new helpers at the bottom of head_dbg.js that I added in bug 1000967. If it is too much work, then forget it.

More seriously, this doesn't handle compatibility in the client for the protocol changes introduced. As the patch is now, it would break debugging b2g and firefox for android or any situation with an older implementation of the server.

You should add a new trait to the root's hello packet[0] so we can tell if setting BPs on sources is supported, and if you don't find that trait you need to make a polyfill of sorts that will send the packet with the URL to the thread actor. It's ok to require a URL in this case because older servers don't support debugging sources without URLs.

This protocol compat stuff needs to be tested as well: a simple mock actor that implements the older protocol should work. Just verify that we can still send the old style setBreakpoint packets and that they are correctly formed.

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/root.js?from=root.js#101

::: toolkit/devtools/client/dbg-client.jsm
@@ +1726,5 @@
> +   *
> +   * @param aUrl the url of the source
> +   */
> +  getSource: function(aUrl, aCallback) {
> +    return this.getSources((aResponse) => {

Nit: I think you mean source client, not source actor.

Nit: What's the difference between the |source| method and the |getSource| method? Can we rename this method to |getSourceByURL|? Thanks.

More seriously, can we cache source clients locally and avoid the request when possible? We already cache actor ID -> source client, it shouldn't be too much work to also cache based on URL when applicable.

On top of just the round trip for the request, getting a list of sources can be slow. It's based on top of Debugger.prototype.findScripts, which literally iterates over every GC thing in the set of debuggee's GC zones (about equivalent to the heap space used by a tab) and checks to see if that GC thing is a JSScript. I've seen this take up to 200ms.

Getting a list of sources via a request to the server should be a very last resort.

@@ +1727,5 @@
> +   * @param aUrl the url of the source
> +   */
> +  getSource: function(aUrl, aCallback) {
> +    return this.getSources((aResponse) => {
> +      let form = aResponse.sources.filter(s => s.url === aUrl)[0];

What if we provide a URL that the server doesn't know about? We should handle the error case here and pass (error, sourceClient) to the callback, node style (at least until we adopt promises in the client).

@@ +2382,5 @@
> +  /**
> +   * Request to set a breakpoint in the specified location.
> +   *
> +   * @param object aLocation
> +   *        The source location object where the breakpoint will be set.

I don't see the |aLocation| parameter anymore ;)

Want to document line, column, and condition, please?

::: toolkit/devtools/server/actors/script.js
@@ +15,5 @@
>  const { SourceMapConsumer, SourceMapGenerator } = require("source-map");
>  const { defer, resolve, reject, all } = require("devtools/toolkit/deprecated-sync-thenables");
>  const { CssLogic } = require("devtools/styleinspector/css-logic");
>  
> +const { Task } = require("resource://gre/modules/Task.jsm");

We need to block this bug on making task worker-able, right?

@@ +2596,5 @@
> +    let originalLoc = aRequest.location;
> +    originalLoc.url = this.url;
> +
> +    if (loc.url) {
> +      loc = yield this.threadActor.sources.getGeneratedLocation(loc);

Not sure if we want to handle it in this patch, but getGeneratedLocation should start taking either a SourceActor or a D.Source instead of a URL. And it should return a D.Source rather than a url as well.

@@ +2601,5 @@
> +    }
> +
> +    if (loc.line == null ||
> +        loc.line < 0 ||
> +        (loc.url && this.dbg.findScripts({ url: loc.url }).length == 0)) {

Similarly, this should be checking for scripts who belong to the generated location's D.Source.

@@ +2611,5 @@
> +          + " but there is no Debugger.Script at that location"
> +      };
> +    }
> +
> +    let response = yield this._createAndStoreBreakpoint({

Wait why is this |yield|ed? If I'm reading this right, neither |_createAndStoreBreakpoint|, nor |_setBreakpoint| return promises.

(Perhaps a good excuse to add @returns notes to their jsdoc comments?)

@@ +2657,5 @@
> +   */
> +  _createAndStoreBreakpoint: function (aRequest) {
> +    // Add the breakpoint to the store for later reuse, in case it belongs to a
> +    // script that hasn't appeared yet.
> +    if(aRequest.url) {

Nit: If |url| is optional, that contradicts what the jsdoc comment says above ;)

@@ +5022,5 @@
>     * @param optional String contentType
>     *        The content type of the source, if immediately available.
>     * @returns a SourceActor representing the source at aURL or null.
>     */
> +  source: function  ({ url, script, sourceMap, generatedSource, text, contentType }) {

If the only reason we accept a script is to get its source, maybe we should just accept the source?

@@ +5052,5 @@
>      });
>      this._thread.threadLifetimePool.addActor(actor);
> +
> +    if(script) {
> +      this._sourceActors[script] = actor;

This should be

    this._sourceActors.set(script.source, actor);

no?

I don't think you can directly index a Map. Also you want to key by source, not script, right?

::: toolkit/devtools/server/tests/unit/test_breakpoint-01.js
@@ +17,5 @@
>    do_test_pending();
>  };
>  
>  function run_test_with_server(aServer, aCallback)
> +{  

Nit: trailing whitespace

::: toolkit/devtools/server/tests/unit/test_breakpoint-02.js
@@ +46,5 @@
>      do_check_eq(aPacket.type, "paused");
>      do_check_eq(aPacket.why.type, "interrupted");
>    });
>  
> +  gThreadClient.getSource(path, source => {

If you just made the getSource method for tests, you could throw it in head_dbg.js. Then my previous comments don't apply so much.

Comment 7

[James Long (:jlongster)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> > This moves `onSetBreakpoint` (and all supporting functions) to the
> > SourceActor, and refactors it to use Task.async. I've started converting
> > some of the tests, and they pass (but not on the worker thread, need to
> > convert Task to work there which will happen soon).
> 
> What was the resolution regarding Task + workers? Are we going to just delay
> landing this? Do we have bugs that this one needs to depend on?

We're going to delay landing this, but it shouldn't take too long to get promises+workers working. I'll set the blocking bug. ejpbruel said he will focus on that after worker debugging lands (in a matter of weeks). I probably won't have something ready to land until then.

> Also, please submit a PR to https://github.com/jimblandy/DebuggerDocs with
> the protocol updates.

Will do.

Comment 8

[James Long (:jlongster)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Comment on attachment 8445349 [details] [diff] [review]
> 920281.patch
> 
> Review of attachment 8445349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Would love to see the tests use Task.jsm and the new helpers at the bottom
> of head_dbg.js that I added in bug 1000967. If it is too much work, then
> forget it.

I will totally do that. Keep in mind this patch was meant for really early review to make sure I'm on the right path. Lots of stuff to do (such as protocol compatibility) that I will focus on now.

> More seriously, this doesn't handle compatibility in the client for the
> protocol changes introduced...

Yeah, was focusing on the initial changes. I'll do that next; add the trait and add tests that use a mock actor.

> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +1726,5 @@
> > +   *
> > +   * @param aUrl the url of the source
> > +   */
> > +  getSource: function(aUrl, aCallback) {
> > +    return this.getSources((aResponse) => {
> 
> Nit: What's the difference between the |source| method and the |getSource|
> method? Can we rename this method to |getSourceByURL|? Thanks.

Yeah, the main difference is it looks up by URL.

> 
> More seriously, can we cache source clients locally and avoid the request
> when possible? We already cache actor ID -> source client, it shouldn't be
> too much work to also cache based on URL when applicable.

Yep, can definitely do that. It seems useful, but I actually did just do it only for tests, so I'll just move it out for now and put it in head_dbg.js.

> 
> @@ +1727,5 @@
> > +   * @param aUrl the url of the source
> > +   */
> > +  getSource: function(aUrl, aCallback) {
> > +    return this.getSources((aResponse) => {
> > +      let form = aResponse.sources.filter(s => s.url === aUrl)[0];
> 
> What if we provide a URL that the server doesn't know about? We should
> handle the error case here and pass (error, sourceClient) to the callback,
> node style (at least until we adopt promises in the client).

If I move it out for tests, I don't think we need to worry about this as much.

> 
> @@ +2382,5 @@
> > +  /**
> > +   * Request to set a breakpoint in the specified location.
> > +   *
> > +   * @param object aLocation
> > +   *        The source location object where the breakpoint will be set.
> 
> I don't see the |aLocation| parameter anymore ;)
> 
> Want to document line, column, and condition, please?

Hey! I just copied what was there before :) But yeah, I can fix that.

> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +15,5 @@
> >  const { SourceMapConsumer, SourceMapGenerator } = require("source-map");
> >  const { defer, resolve, reject, all } = require("devtools/toolkit/deprecated-sync-thenables");
> >  const { CssLogic } = require("devtools/styleinspector/css-logic");
> >  
> > +const { Task } = require("resource://gre/modules/Task.jsm");
> 
> We need to block this bug on making task worker-able, right?

Done. (I blocked the main tracking bug)

> 
> @@ +2596,5 @@
> > +    let originalLoc = aRequest.location;
> > +    originalLoc.url = this.url;
> > +
> > +    if (loc.url) {
> > +      loc = yield this.threadActor.sources.getGeneratedLocation(loc);
> 
> Not sure if we want to handle it in this patch, but getGeneratedLocation
> should start taking either a SourceActor or a D.Source instead of a URL. And
> it should return a D.Source rather than a url as well.

Yep, I'm starting to work on all of that in bug 917579. I'm changing everything like the breakpoint store to map actors to values instead of urls to values. Let's get this patch fixed up (with compatibility and all) and then slowly convert the rest.

> @@ +2611,5 @@
> > +          + " but there is no Debugger.Script at that location"
> > +      };
> > +    }
> > +
> > +    let response = yield this._createAndStoreBreakpoint({
> 
> Wait why is this |yield|ed? If I'm reading this right, neither
> |_createAndStoreBreakpoint|, nor |_setBreakpoint| return promises.
> 
> (Perhaps a good excuse to add @returns notes to their jsdoc comments?)

It's a good question; I pretty much copied the same behavior as before, and before it treats it like a promise: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L1426 A few lines below it waits on that value.

> 
> @@ +5022,5 @@
> >     * @param optional String contentType
> >     *        The content type of the source, if immediately available.
> >     * @returns a SourceActor representing the source at aURL or null.
> >     */
> > +  source: function  ({ url, script, sourceMap, generatedSource, text, contentType }) {
> 
> If the only reason we accept a script is to get its source, maybe we should
> just accept the source?

I thought about that, and can do that. I forget the exact reason why I went this way.

> 
> @@ +5052,5 @@
> >      });
> >      this._thread.threadLifetimePool.addActor(actor);
> > +
> > +    if(script) {
> > +      this._sourceActors[script] = actor;
> 
> This should be
> 
>     this._sourceActors.set(script.source, actor);
> 
> no?
> 
> I don't think you can directly index a Map. Also you want to key by source,
> not script, right?

You're absolutely right; I added this just recently and I don't think the tests I was working with covered it. I will make a v2 of this patch soon that is a lot more robust. You're comments have been helpful and is what I was looking for.

> 
> ::: toolkit/devtools/server/tests/unit/test_breakpoint-01.js
> @@ +17,5 @@
> >    do_test_pending();
> >  };
> >  
> >  function run_test_with_server(aServer, aCallback)
> > +{  
> 
> Nit: trailing whitespace

Yesterday I finally configured Emacs to turn on showing whitespace only in certain modes. This should never happen again.

> ::: toolkit/devtools/server/tests/unit/test_breakpoint-02.js
> @@ +46,5 @@
> >      do_check_eq(aPacket.type, "paused");
> >      do_check_eq(aPacket.why.type, "interrupted");
> >    });
> >  
> > +  gThreadClient.getSource(path, source => {
> 
> If you just made the getSource method for tests, you could throw it in
> head_dbg.js. Then my previous comments don't apply so much.

Will do.

Comment 9

[James Long (:jlongster)]

This work has been merged in with all the other Debugger.Source work in bug 905700.