We should be talking about sources, not URLs, so we should point to source actors instead of URLs in our locations. For example, frame.where should have the form { source, line, column } not { url, line, column }. We need this to fully embrace Debugger.Source.

Thinking about protocol compatibility: * For requests the client sends to the server, compatibility is pretty easy: just send the url property instead of the source form. If the source isn't associated with a URL, just abort because the server won't be able to debug that source anyways. We will need to add a little infrastructure to our protocol compatibility layer, but it shouldn't be too hard. * For receiving responses/unsolicited notifications from the server, it is slightly trickier. We can try and map the URL we receive to a source, but we won't get different sources for different evaluations of the same script. I guess that is true now anyways. * Finally, I think that we will probably have to add another trait in the hello packet for this. I suspect we will want to know whether we need to handle this compatibility right from the start.

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

I'm converting all the code dealing source maps to use sources instead of urls. It's a bit complicated, so I'm going to post questions here. Just one for now: `getGeneratedLocation` will still take a `url` right? Because an "original" location will always have a URL, since it's been sourcemapped, right?

(In reply to James Long (:jlongster) from comment #3) > I'm converting all the code dealing source maps to use sources instead of > urls. It's a bit complicated, so I'm going to post questions here. Just one > for now: `getGeneratedLocation` will still take a `url` right? Because an > "original" location will always have a URL, since it's been sourcemapped, > right? It will have /something/ as a URL, but it can definitely be malformed. The compiler can name the source whatever it likes, really.

(In reply to Nick Fitzgerald [:fitzgen] from comment #4) > (In reply to James Long (:jlongster) from comment #3) > > I'm converting all the code dealing source maps to use sources instead of > > urls. It's a bit complicated, so I'm going to post questions here. Just one > > for now: `getGeneratedLocation` will still take a `url` right? Because an > > "original" location will always have a URL, since it's been sourcemapped, > > right? > > It will have /something/ as a URL, but it can definitely be malformed. The > compiler can name the source whatever it likes, really. I'm confused, the location that I pass to `getGeneratedLocation` is the location provided by the sourcemap. The only way I get it is to call `getOriginalLocation`, and it just gives back the info provided by the source map (if one exists). So I have a URL with a lin/col. How does the compiler naming things play into this?

(In reply to James Long (:jlongster) from comment #5) > (In reply to Nick Fitzgerald [:fitzgen] from comment #4) > > (In reply to James Long (:jlongster) from comment #3) > > > I'm converting all the code dealing source maps to use sources instead of > > > urls. It's a bit complicated, so I'm going to post questions here. Just one > > > for now: `getGeneratedLocation` will still take a `url` right? Because an > > > "original" location will always have a URL, since it's been sourcemapped, > > > right? > > > > It will have /something/ as a URL, but it can definitely be malformed. The > > compiler can name the source whatever it likes, really. > > I'm confused, the location that I pass to `getGeneratedLocation` is the > location provided by the sourcemap. The only way I get it is to call > `getOriginalLocation`, and it just gives back the info provided by the > source map (if one exists). The info provided by the source map is from the compiler, and the "url" can really be an arbitrary string. > So I have a URL with a lin/col. How does the > compiler naming things play into this? You should be giving that arbitrary data from the source map & compiler back, right here. So yes, it shouldn't matter if its malformed, but don't make any assumptions and try and actually use it as a URL without verifying first. Not a big deal, but just something we shouldn't forget.

This is very much not working yet, but I wanted to put something up here before the long weekend. If you're interested, you can see what I'm working towards here. I'm trying hard to normalize all the APIs and make it consistent. (There's a chance my wife might go into labor this weekend too, another reason why I want to go ahead and put this up...) I know I need to update lots of comments and such. I've been focusing on wrapping my head around the semantics of all these parts, mainly sourcemaps and eval scripts. I'm still churning some on the `getOriginalLocation` API. It takes a Debugger.Source object, but there are places that call it with stuff like `packet.frame.where` which now has a source form instead of URL. So in specific places I need to convert that source form -> SourceActor -> Debugger.Source just to call it. Doesn't feel quite right yet, but I think I can figure out a consistent API soon.

Getting closer; some of the basic tests are passing, but there's still some rough edges that I need to fix. Most of them are just changing API calls to pass the right kind of value. I think it'll come together pretty quickly with some more work.

Still lots to do here: add comments, update other small parts of the code, etc. But here is the work that changes all the sourcemaps and frame locations to use source form instead of URLs. I updated all 18 of the test_breakpoint-*.js tests and got them all to pass. Note that all the comparisons in the tests use `frame.where.source.actor` instead of `frame.where.url` now. test_breakpoint-08.js is weird. It pauses in one source, but wants to set a breakpoint in the other. With the new code, I need the source for the latter, can't just use a URL. I thought it'd be neat if the RDP had a "sourceFor" that took a function name and gave back a source, but I realize that I really shouldn't add to the RDP, especially since I found `getDefinitionSite` which is pretty much the same thing. So I'll change that soon.

It's worth noting too that I've ended up only using Task.async in one function, so I can easily remove that dependency (`onSetBreakpoint`). It's a complicated function and it helps a lot, but it wouldn't be too bad to use raw promises. I might go ahead and remove Task for now because I have to patch each test to not run under the worker because that's broken with Task. Since it hasn't transformed my work here, we can wait to refactor parts of script.js whenever it's ready to be run in a worker.

I also haven't done the persistent stuff yet; the breakpoint store and other stores still key off of URL directly. I will change that to key off of actor and somehow map persistent sources (across reloads) to the right thing. fitzgen has an initial diagram for this in bug 905700

It's really coming along now. All of the sourcemap-*.js tests pass in addition to all breakpoint-*.js, which is some of the more complicated stuff. We need to add tests to make sure that `eval` scripts are now treated correctly in addition to URL-based scripts, but right now let's focus on getting our current tests passing. Not quite ready for review; need to finish updating comments and other things like that first. This patch is meant to be on top of the one in bug 920281. Eventually we should move these patches to the main bug (as discussed before). I can keep them as separate patches though for review purposes. Script persistence is the next thing I'll focus on (tracking breakpoints/etc across scripts we deem to be the same)

I went ahead and removed the Task.jsm dependency for now. The `onSetBreakpoint` function is uglier, but I can probably break it up some anyhow. Now all the tests that were passing are also passing running in a worker.

This patch is ALL of the Debugger.Source work, including bug 920281. I accidentally merged those patches, but I think that's fine because I fixed several mistakes in the old patch anyway. All xpcshell tests are passing with this. This moves `setBreakpoint` to be on the SourceActor, augments SourceActor to track an actual Debugger.Source object, and changes most everything that references a URL to use a SourceActor (whether it's an actual SourceActor instance, a SourceActor.form(), or an actor id depends on context). This is getting to be a big patch. There is still work left though. Nick, do you want me all future work to start in a second patch, to break it up a little? Things left to do: * Protocol compatibility; need to make SourceClient send the `setBreakpoint` request to the ThreadActor if it's an old server. Most of the compat work will be in the browser-side of things, which I haven't worked on yet. * Update the browser files to use the new SourceClient API for setting breakpoints, and update things like frame locations are now `frame.where.source.url` instead of `frame.where.url`. Compat work needed here. * Track scripts across reloads. The BreakpointStore has been converted to store breakpoints by actor id, instead of URLs, which means when a page is reloaded and new SourceActor ids are generated, it won't load previous breakpoints. I haven't gone through this patch with a fine-tooth comb, so there may be nit-picky things in it, but I'm going on PTO so I'd like to post it. Will probably hack some over the the weekend; I'd really like to get the browser side up-to-date with this. (I also haven't rebased this, sorry... probably won't apply. I will do that soon too. Feel free to wait to r? until then)

(In reply to James Long (:jlongster) from comment #14) > This is getting to be a big patch. There is still work left though. Nick, do > you want me all future work to start in a second patch, to break it up a > little? All of those things sound like they need to be in the same commit, or else it will likely break bisecting. It's nice when you break things up into small patches, but we shouldn't ever break bisect, and it can add more administrative overhead to your version control habits, so I understand.

Comment on attachment 8462888 [details] [diff] [review] 917579.patch Review of attachment 8462888 [details] [diff] [review]: ----------------------------------------------------------------- Lookin' good, can't wait for this to land. ::: toolkit/devtools/client/dbg-client.jsm @@ +2370,5 @@ > + /** > + * Request to set a breakpoint in the specified location. > + * > + * @param object aLocation > + * The source location object where the breakpoint will be set. Let's update this comment to document line, column, and condition instead. @@ +2419,5 @@ > + // Can't set the breakpoint if pausing failed. > + aOnResponse(aResponse); > + return; > + } > + doSetBreakpoint(() => { this._activeThread.resume() }); Unrelated, but can you file a follow up bug to only resume if the last pause reason was "interrupt"? I feel like we have a subtle opening for race conditions where we start setting a breakpoint but before the server gets the packet it pauses for some other reason, then the interrupt request gets an "alreadyPaused" response, we set the breakpoint, and then resume. That resumption is a bug, because from the user's point of view we "didn't" pause on that debugger statement / exception thrown / existing BP / whatever. ::: toolkit/devtools/server/actors/script.js @@ +105,5 @@ > * - condition (optional) > * - actor (optional) > */ > addBreakpoint: function (aBreakpoint) { > + let { source: { actor }, line, column } = aBreakpoint; If the only reason we are taking a source is to get the actor ID, perhaps we should just take the actor ID directly? This would simplify a lot of the BreakpointStore API... @@ +255,5 @@ > } > } > }, > > + _iterUrls: function* (actor) { This function should be renamed now. @@ +857,5 @@ > return undefined; > } > packet.why = aReason; > > + this.sources.getOriginalLocation(this.getFrameLocation(aFrame)).then(aOrigPosition => { Nit: pretty sure this line is > 80 chars. @@ +930,2 @@ > > if (thread.sources.isBlackBoxed(url)) { Rather than getting the source actor's URL and checking if it is black boxed (which means we can still only black box sources with URLs), we should make the black boxed stuff work on actors instead of URLs. I'd be fine with a follow up. @@ +1084,5 @@ > * @param Debugger.Frame aFrame > * The frame we want to clear the stepping hooks from. > */ > _clearSteppingHooks: function (aFrame) { > + if(aFrame && aFrame.live) { Wait... Didn't we land this change months ago? @@ +2028,5 @@ > + _allowSource: function (aSource) { > + let url = aSource.url; > + > + // Ignore the internal Function.prototype script > + if(aSource.text === '() {\n}') { ... we really need a better way to discover this script or avoid it ... Because the ChromeDebuggerActor's allowSource is just "return true" and we shouldn't have to keep repeating this check there and in the AddonThreadActor @@ +2079,5 @@ > // script. > if (!bp.actor.scripts.length > && bp.line >= aScript.startLine > && bp.line <= endLine) { > + var actor = this.sources.source({ script: aScript }); Don't we already have the actor above in the `source` variable? @@ +2124,5 @@ > + * The frame whose location we are getting. > + * @returns Object > + * Returns an object of the form { source, line, column } > + */ > + getFrameLocation: function(aFrame) { Why make this a method now? It isn't using anything from `this`, and it made you have to do `let threadActor = this;` a bunch in the various stepping hooks. @@ +2310,5 @@ > _addonID: null, > _addonPath: null, > > get threadActor() this._threadActor, > + get dbg() this.threadActor.dbg, Let's end the spidermonkey-isms while we're here. Can you convert all these getters to standard ES5/6: get foo() { return this.bar; }, @@ +2716,5 @@ > + * Create a breakpoint at the specified location and store it in the > + * cache. Takes ownership of `aLocation`. > + * > + * @param Object aLocation > + * An object of the form { line[, column, url] } Does it still take `url`? Also s/aLocation/aRequest/ Can we document that this is a generated location? @@ +2721,5 @@ > + */ > + _createAndStoreBreakpoint: function (aRequest) { > + let bp = merge(aRequest, { source: this.form() }); > + if(!this.breakpointStore.hasBreakpoint(bp)) { > + this.breakpointStore.addBreakpoint(bp); After bug 897567, you shouldn't need to check hasBreakpoint before calling addBreakpoint. @@ +2735,5 @@ > + * take that into account. > + * > + * @param object aLocation > + * The location of the breakpoint (in the generated source, if source > + * mapping). s/aLocation/aRequest/ @@ +2899,5 @@ > + * closest offset mappings. If there is no column in `aTargetLocation`, we add > + * all offset mappings that are on the given line. > + * > + * @param Object aTargetLocation > + * An object of the form { url, line[, column] }. Doesn't seem like we are still using `url`, right? @@ +4431,5 @@ > form.this = this.threadActor.createValueGrip(this.frame.this); > form.arguments = this._args(); > if (this.frame.script) { > + let loc = threadActor.getFrameLocation(this.frame); > + let { sourceActor } = threadActor.synchronize(threadActor.sources.getOriginalLocation(loc)); Nit: line is too long. @@ +4495,5 @@ > * The parent thread actor that contains this breakpoint. > * @param object aLocation > * The location of the breakpoint as specified in the protocol. > */ > +function BreakpointActor(aThreadActor, { sourceActor, line, column, condition }) Nit: I know this already existed, but please update the doc comment above and document sourceActor/line/column/condition rather than the non-existant aLocation. @@ +4500,4 @@ > { > this.scripts = []; > this.threadActor = aThreadActor; > + this.location = { sourceActor: sourceActor, line: line, column: column }; Nit: this can now be this.location = { sourceActor, line, column }; :D @@ +4869,5 @@ > /** > * Override the eligibility check for scripts and sources to make sure every > * script and source with a URL is stored when debugging chrome. > */ > + _allowSource: function(aSource) !!aSource.url, This is going to allow the Function.prototype source through. :( Also, please don't use spidermonkey-isms anymore: _allowSource: aSource => !!aSource.url, @@ +4941,5 @@ > * Override the eligibility check for scripts and sources to make > * sure every script and source with a URL is stored when debugging > * add-ons. > */ > + _allowSource: function(aSource) { This one will let through the Function.prototype source as well :( @@ +5106,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 ({ source, originalUrl, generatedSource, sourceMap, text, contentType }) { Nit: line is too long On a more serious note: do you think we should split this function into something like |getSource| (which throws when it doesn't find a source that we expect to exist; maybe even split out to |getSourceByX| functions...) and |getOrCreateSourceActor| (which does the actual creating if it doesn't exist)? It's (still) kind of messy to use this function with its five thousand parameters that can be mixed and matched... Ok as a follow up if you prefer. @@ +5118,5 @@ > > + // let persId = this.getPersistentId(source); > + // if(persId && this._persistentSourceActors[persId]) { > + // let actor = this._persistentSourceActors[persId]; > + // } I expect you still have work to do here? @@ +5175,2 @@ > } > + return null; Still going to do displayURL and sourceMapURL as persistent keys, right? @@ +5224,5 @@ > */ > sourceMap: function (aScript) { > dbg_assert(aScript.sourceMapURL, "Script should have a sourceMapURL"); > + let sourceMapURL = aScript.sourceMapURL; > + if(aScript.url) { nit: space after if @@ +5231,2 @@ > let map = this._fetchSourceMap(sourceMapURL, aScript.url) > + .then(aSourceMap => this.saveSourceMap(aSourceMap, aScript.source)); Nit: either 2 space indent, or align the . with the . in this._fetchSourcemap @@ +5319,5 @@ > } > > // No source map > return resolve({ > + sourceActor: this.source({ source: source }), { source } :) @@ +5446,5 @@ > + let actors = Object.keys(this._sourcemappedSourceActors).map(k => { > + return this._sourcemappedSourceActors[k]; > + }); > + for(let actor of this._sourceActors.values()) { > + if(!this._sourceMapsByGeneratedSource.has(actor.source)) { Nits: spaces before open parens in for and if @@ +5483,5 @@ > > return bestOffsetMapping.columnNumber; > } > > +function merge(obj1, obj2) { This seems like a partial reimplementation of DevToolsUtils.update, can we use that instead? Could make it take an arbitrary number of objects and merge them all into the first one, that way you can make it return a new object, like this merge does: let thing = update({}, foo, bar) ::: toolkit/devtools/server/tests/unit/head_dbg.js @@ +13,5 @@ > > const Services = devtools.require("Services"); > // Always log packets when running tests. runxpcshelltests.py will throw > // the output away anyway, unless you give it the --verbose flag. > +Services.prefs.setBoolPref("devtools.debugger.log", false); Make sure you revert this change before checkin. ::: toolkit/devtools/server/tests/unit/test_sourcemaps-02.js @@ +36,5 @@ > gClient.addOneTimeListener("paused", function (aEvent, aPacket) { > gThreadClient.getSources(function (aResponse) { > do_check_true(!aResponse.error, "Should not get an error"); > > + console.log(aResponse.sources); leftover debug log ::: toolkit/devtools/server/tests/unit/test_sourcemaps-13.js @@ +87,5 @@ > function test_new_sources() { > gThreadClient.getSources(function ({ error, sources }) { > do_check_true(!error); > + sources = sources.filter(source => source.url); > + nit whitespace

Thanks for the review! I'll respond more thoroughly when I'm back at work (coming back Aug 11). (In reply to Nick Fitzgerald [:fitzgen] from comment #15) > It's nice when you break things up into small patches, but we shouldn't ever > break bisect, and it can add more administrative overhead to your version > control habits, so I understand. Ok. I was thinking I would build up a patch queue and collapse them all into one commit to actually commit to a tree, in order to help review. It does make it even harder to see how all the changes interact. I'll keep it in one patch if you're ok with that.

While rebasing, I noticed there's a new test in there: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/tests/unit/test_breakpoint-19.js. That sets a breakpoint on a non-existent source. I don't know how to handle that with the new setup. You have to have a SourceActor to set the breakpoint on. We basically need to grab a SourceActor for a source and somehow GC it, and then try to set the breakpoint, if we want to replicate the bug. What do you think?

I fixed a bunch of your nits and responded to several other comments below: (In reply to Nick Fitzgerald [:fitzgen] from comment #16) > Comment on attachment 8462888 [details] [diff] [review] > 917579.patch > > Review of attachment 8462888 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +2419,5 @@ > > + // Can't set the breakpoint if pausing failed. > > + aOnResponse(aResponse); > > + return; > > + } > > + doSetBreakpoint(() => { this._activeThread.resume() }); > > Unrelated, but can you file a follow up bug to only resume if the last pause > reason was "interrupt"? ... https://bugzilla.mozilla.org/show_bug.cgi?id=1053468 > > ::: toolkit/devtools/server/actors/script.js > @@ +105,5 @@ > > * - condition (optional) > > * - actor (optional) > > */ > > addBreakpoint: function (aBreakpoint) { > > + let { source: { actor }, line, column } = aBreakpoint; > > If the only reason we are taking a source is to get the actor ID, perhaps we > should just take the actor ID directly? This would simplify a lot of the > BreakpointStore API... We do use the url from the source in `getBreakpoint` for the error message, but we could do without that. I don't think it would actually make the API that much better. In a lot of places we already have an object with a sourceForm/line/col combination, so we can just pass it directly in (some times we need to convert a source object into its sourceForm first). Right now it receives what the thing we most commonly have. > @@ +930,2 @@ > > > > if (thread.sources.isBlackBoxed(url)) { > > Rather than getting the source actor's URL and checking if it is black boxed > (which means we can still only black box sources with URLs), we should make > the black boxed stuff work on actors instead of URLs. I'd be fine with a > follow up. Yep, planning on doing that. I think I'd like to get the front-end working first with the current patch and maybe land all this stuff, and then move on. I keep having to rebase and carefully update with bugfixes. > > @@ +1084,5 @@ > > * @param Debugger.Frame aFrame > > * The frame we want to clear the stepping hooks from. > > */ > > _clearSteppingHooks: function (aFrame) { > > + if(aFrame && aFrame.live) { > > Wait... Didn't we land this change months ago? Yep, my mistake... My branch was old. I've rebased everything onto today's master branch. > > @@ +2028,5 @@ > > + _allowSource: function (aSource) { > > + let url = aSource.url; > > + > > + // Ignore the internal Function.prototype script > > + if(aSource.text === '() {\n}') { > > ... we really need a better way to discover this script or avoid it ... > > Because the ChromeDebuggerActor's allowSource is just "return true" and we > shouldn't have to keep repeating this check there and in the AddonThreadActor Agreed, I hate this hack. I'll look into putting it somewhere else. Unfortunately the `allowSource` method is called in 2 different places so we can't hardcode this wherever that is called. > > @@ +2079,5 @@ > > // script. > > if (!bp.actor.scripts.length > > && bp.line >= aScript.startLine > > && bp.line <= endLine) { > > + var actor = this.sources.source({ script: aScript }); > > Don't we already have the actor above in the `source` variable? Good catch. > > @@ +2124,5 @@ > > + * The frame whose location we are getting. > > + * @returns Object > > + * Returns an object of the form { source, line, column } > > + */ > > + getFrameLocation: function(aFrame) { > > Why make this a method now? It isn't using anything from `this`, and it made > you have to do `let threadActor = this;` a bunch in the various stepping > hooks. I forget. I had to do it at one point, so I must have been playing around with a different implementation. I've pulled it out. > > @@ +4495,5 @@ > > * The parent thread actor that contains this breakpoint. > > * @param object aLocation > > * The location of the breakpoint as specified in the protocol. > > */ > > +function BreakpointActor(aThreadActor, { sourceActor, line, column, condition }) How should we document destructured arguments? Technically a single object is passed in, should we still name it in the documentation? > @@ +4869,5 @@ > > /** > > * Override the eligibility check for scripts and sources to make sure every > > * script and source with a URL is stored when debugging chrome. > > */ > > + _allowSource: function(aSource) !!aSource.url, > > This is going to allow the Function.prototype source through. :( Will fix that in the next rev of my patch. > @@ +5106,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 ({ source, originalUrl, generatedSource, sourceMap, text, contentType }) { > > Nit: line is too long > > On a more serious note: do you think we should split this function into > something like |getSource| (which throws when it doesn't find a source that > we expect to exist; maybe even split out to |getSourceByX| functions...) and > |getOrCreateSourceActor| (which does the actual creating if it doesn't > exist)? > > It's (still) kind of messy to use this function with its five thousand > parameters that can be mixed and matched... Ok as a follow up if you prefer. Hm, that might be worth it. It would make it more clear what the desire is; if we know the source should exist and just want to get it, an error might be helpful if it doesn't already exist. I'll think about it. The `text` argument can be removed, which makes the line 79 chars long, so short enough. :) Also one less argument. Still might be nice to split it up. Will look tomorrow. > > @@ +5118,5 @@ > > > > + // let persId = this.getPersistentId(source); > > + // if(persId && this._persistentSourceActors[persId]) { > > + // let actor = this._persistentSourceActors[persId]; > > + // } > > I expect you still have work to do here? > > @@ +5175,2 @@ > > } > > + return null; > > Still going to do displayURL and sourceMapURL as persistent keys, right? Yep, still work to do, I will expand it to match on all the URLs that might be associated with a source. I'm still confused how to actually implement this though, and here's why: when a page gets reloaded, entirely new Debugger.Source objects are created for each source. So I can't literally just reuse a SourceActor, as the source object that it references points to the old source in memory. I need to somehow reuse a SourceActor but tell it to reference the new source. That make sense? > @@ +5319,5 @@ > > } > > > > // No source map > > return resolve({ > > + sourceActor: this.source({ source: source }), > > { source } > > :) \o/ > > @@ +5483,5 @@ > > > > return bestOffsetMapping.columnNumber; > > } > > > > +function merge(obj1, obj2) { > > This seems like a partial reimplementation of DevToolsUtils.update, can we > use that instead? Could make it take an arbitrary number of objects and > merge them all into the first one, that way you can make it return a new > object, like this merge does: > > let thing = update({}, foo, bar) I can do that.

This version fixes a bunch of stuff from fitzgen's review and rebased it onto the current fx-team branch.

(In reply to James Long (:jlongster) from comment #19) > (In reply to Nick Fitzgerald [:fitzgen] from comment #16) > > ::: toolkit/devtools/server/actors/script.js > > @@ +105,5 @@ > > > * - condition (optional) > > > * - actor (optional) > > > */ > > > addBreakpoint: function (aBreakpoint) { > > > + let { source: { actor }, line, column } = aBreakpoint; > > > > If the only reason we are taking a source is to get the actor ID, perhaps we > > should just take the actor ID directly? This would simplify a lot of the > > BreakpointStore API... > > We do use the url from the source in `getBreakpoint` for the error message, > but we could do without that. I don't think it would actually make the API > that much better. In a lot of places we already have an object with a > sourceForm/line/col combination, so we can just pass it directly in (some > times we need to convert a source object into its sourceForm first). Right > now it receives what the thing we most commonly have. Ok. > > @@ +2028,5 @@ > > > + _allowSource: function (aSource) { > > > + let url = aSource.url; > > > + > > > + // Ignore the internal Function.prototype script > > > + if(aSource.text === '() {\n}') { > > > > ... we really need a better way to discover this script or avoid it ... > > > > Because the ChromeDebuggerActor's allowSource is just "return true" and we > > shouldn't have to keep repeating this check there and in the AddonThreadActor > > Agreed, I hate this hack. I'll look into putting it somewhere else. > Unfortunately the `allowSource` method is called in 2 different places so we > can't hardcode this wherever that is called. Should be easier/less-repetitious now that we consolidated the debuggee management a bit more in bug 1039952. > > @@ +4495,5 @@ > > > * The parent thread actor that contains this breakpoint. > > > * @param object aLocation > > > * The location of the breakpoint as specified in the protocol. > > > */ > > > +function BreakpointActor(aThreadActor, { sourceActor, line, column, condition }) > > How should we document destructured arguments? Technically a single object > is passed in, should we still name it in the documentation? While it is technically a single object, we have been documenting each destructured property. I think it follows the intention of having the documentation help you understand how to use the function. > > @@ +5118,5 @@ > > > > > > + // let persId = this.getPersistentId(source); > > > + // if(persId && this._persistentSourceActors[persId]) { > > > + // let actor = this._persistentSourceActors[persId]; > > > + // } > > > > I expect you still have work to do here? > > > > @@ +5175,2 @@ > > > } > > > + return null; > > > > Still going to do displayURL and sourceMapURL as persistent keys, right? > > Yep, still work to do, I will expand it to match on all the URLs that might > be associated with a source. I'm still confused how to actually implement > this though, and here's why: when a page gets reloaded, entirely new > Debugger.Source objects are created for each source. So I can't literally > just reuse a SourceActor, as the source object that it references points to > the old source in memory. I need to somehow reuse a SourceActor but tell it > to reference the new source. That make sense? Yeah, makes sense. Interested in seeing what you come up with. Feel free to bounce ideas off me :)

Just a quick update here: I'm well on my way updating the frontend to use support the new protocol. The frontend code doesn't need to be significantly refactored, but updated every single little piece that depends on url is a bit of work. I have eval scripts showing up in the source pane, ability to set breakpoints, and most of the widgets working.

Updated version with a few more tweaks to get the frontend working. This has a few `console.log` calls because I'm still trying to fix some mochitests.

This is the frontend patch. Things work pretty well when I try to debug a script manually, but there's still a lot of tests to fix. Right now I'm trying to fix https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/test/browser_dbg_addon-modules-unpacked.js. It's erroring saying that sources[0].url is null (the first one in the file). It uses this function to get the sources: https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/test/head.js#L628 That function gets out the list of sources from the source panel. The problem is that the query `g.querySelectorAll(".dbg-source-item")` seems to return a full URL, instead of just the filename, and later on the grouping doesn't work because the labels don't match up. It's confusing and I need to figure out who to talk to about that code.

:fitzgen, if you want to review the toolkit patch, I think we could resolve this bug and move the 2 patches up to the meta bug. The backend RDP patch is pretty much done. (well, except for backwards compatibility actually, still need to do that...)

Cleaned up a bit for final review.

Comment on attachment 8476910 [details] [diff] [review] 917579-toolkit.patch Going to add in backwards compat and it might affect this patch, so wait for review. Should be done this week.

Right now we are using actorID as the "persistent" thing. I keep a mapping from uniqueURL->actorID and make sure all future source actors that match the uniqueURL (either `url`, `displayURL`, or `sourceMapURL`) have the same ID. I'm not exactly sure how this will play out. I do have to kind of "garbage collect" them. I need to blow away my url->actorID mapping on `disconnect` because otherwise the next connection starts assigning IDs from 1 again, and I'll conflict. however we always blow away the persistent stuff on disconnect anyway, so that should work. If I run into any more problems, I might try to just use "uniqueURL" and maybe call it something else (uniqueKey?). If I did that I wouldn't need my mapping object at all and I could derive it straight from the source. I'm not sure if this will limit certain functionality with evals though.

I have 11 mochitests left to get passing, 8 of which are sourcemaps so they are definitely discovering real bugs of mine. Fixing up the sourcemaps might be a little tricky. I think they are close but fixing all the little bugs might take a few days, but hopefully not. Light at the end of the tunnel though. I should have a patch ready for review this week.

I have a big patch ready for review, but since it covers a few bugs I'm going to just move it all into bug 905700

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