Closed Bug 917963 Opened 7 years ago Closed 3 months ago
Break on first statement in new script
47 bytes, text/x-phabricator-request
|Details | Review|
As we integrate the scratchpad into the debugger, we will be in a situation where we have a source we are editing but has never been eval'd, we will need to be able to set breakpoints in it (from a UI POV), and then eval it for the first time. Therefore we need to be able to send an eval request with breakpoint locations we would like to set in the source before the eval executes, since a BP might be set on some code that runs immediately. In fact, we will get a new Debugger.Source for every eval, so it doesn't matter if the source has been eval'd or not, we will need this API every single time.
Alternatively, we could just extend the existing clientEvaluate packet with a breakpoints property. I think this is probably best.
(In reply to Nick Fitzgerald [:fitzgen] from comment #1) > Alternatively, we could just extend the existing clientEvaluate packet with > a breakpoints property. I think this is probably best. I like that idea, too.
I think we actually need to pause whenever we get a new source, like you suggested in Vidyo, Jim. Think of the refresh case: we will get new sources on refresh and we need to stop and let the client re-set breakpoints (since breakpoints will be per-source).
Summary: add evalWithBreakpoints remote debugging protocol request → debugger server should pause on new source, before executing its code
This begs the question as to how we should identify sources that we believe to be the new version of the same source we saw before the last reload or whatever, as well as what line to set the breakpoint on. Options that I see right now (not necessarily mutually exclusive): 1) Keep track of URL + line (and maybe column) on the client. Basically just move BreakpointStore from script.js to dbg-client.js. ** This is going to be sad when you have multiple HTML embedded scripts (and dynamically appended script tags). *** Perhaps we can fix this by bridging the disconnect between lines in the file, lines in the D.Source#text (multiple of which can exist for one HTML file), and the D.Script/D.Source startLines. Possible that the info we need from D.Source is not yet implemented. This is kind of a mess. ** This won't even work at all with eval'd scripts (unless they have //# sourceURL, but this still fails for most eval'd scripts). ** If the user has edited the file in between refreshes, and the line where we set the breakpoint moved up or down, we will end up setting a breakpoint on the wrong line. 2) Remember the text of the line that the breakpoint was set on. ** Will work correctly if the user edits the file in between refreshes and the line where we set the breakpoint shifted up or down. ** Could possibly do some kind of text matching (edit distance or something simpler) in case the user edits the line we set the breakpoint on. ** Works with eval'd scripts that don't have a URL. ** If we wanted to do this approach, we would probably want to have the server send source contents in the newSource unsolicited pause/notification so we wouldn't need to round trip once more for the source text. ** Possibly only use this technique when the source does not have a URL. 3) Same as option 2, but instead of remembering the text of the line we set the breakpoint on, remember the branch of the AST we set it on. ** Intuitively, I suspect this might be too heavy, but I also suspect it would do the best job of matching breakpoints back to the correct location.
Musings on protocol compatibility: TLDR: it should be pretty easy. * We don't need to reset breakpoints in the client when we get newSource notifications; this is already handled by the server right now via BreakpointStore. Also, we won't pause on newSource when attached to an older server, so this shouldn't even need to have a conditional or a check or anything since unsolicitedPausePacket.why.type will never be === "newSource". * Just need to make sure we don't accidentally destroy breakpoints that we suspect belong to an old Debugger.Source instance, since this we won't be re-setting the breakpoints on every newSource pause.
Jim, I didn't find any mention of a dbg.onNewSource hook (what I'm imagining would be basically the same as dbg.onNewScript, except for D.Source instead of D.Script) in the Debugger.Source branch of the docs, is this an intentional omission? It should be fairly easy to polyfill with onNewScript, but it would be nice if it had its own hook. It should fire *before* any of its code is actually executed (same as onNewScript does) so that we have a chance to set breakpoints before the code is run.
Let's try to describe the user-visible behavior these mechanisms are meant to implement, without talking about the mechanisms at all: 1) When we turn on the debugger, we want to be able to set breakpoints in any source code that is already present in the debuggee. 2) If a breakpoint is set in a source that has some kind of persistent identity (like a URL or a scratchpad), we want it to persist across edits, reloads, and navigation. (It should go without saying that we must never skip a breakpoint.) 3) We want content to be able to participate in defining a source's persistent identity: if a page or app uses its own module loader, we should still be able to set "persistent" (in the sense used above) breakpoints in that. Naturally, this requires cooperation from the content. 4) We want to set breakpoints in truly anonymous code, like the stuff passed to random calls to eval, when we have no real handle on where it's coming from or what name the developer might think it has. Right now we have UI only for 1). We can't even type in an unseen URL; we can only choose from amongst the sources the browser has seen before. By treating scratchpads as just another kind of persistent identity, 2) covers the cases Nick is addressing in this bug. Ideally, once we got content loaders participating, the UI for 3) would look just like 1). The UI for 4) would be interesting; perhaps a "stop on eval" mode that pops up the source when control has just begun execution? Perhaps we could select particular calls to eval?
Debugger.Source represents a compilation unit; and I think that's the right thing for a SpiderMonkey API. Recognizing two D.S's as variants of the same thing is a job for higher-level code. So these persistent identities are going to be a server-and-client-level fiction, not a SpiderMonkey feature.
To actually answer your question, though: there's no Debugger.Source announcement hook. It seems like everything that one might use it for actually wants Debugger.Scripts anyway...
Assignee: nobody → nfitzgerald
Priority: -- → P2
I'm unassigning myself because I won't be working on this anytime soon.
Assignee: nfitzgerald → nobody
Could you explain to me why this is a dependency for Debugger.Source? I've glanced over the comments but I'm not sure I understand.
(In reply to Eddy Bruel [:ejpbruel] from comment #11) > Could you explain to me why this is a dependency for Debugger.Source? I've > glanced over the comments but I'm not sure I understand. I guess it isn't strictly necessary, but you can't set breakpoints on an eval'd source's top level unless you are given a chance before it runs.
Summary: debugger server should pause on new source, before executing its code → Pause on new source
Summary: Pause on new source → Break on new source
Hi, I can take this bug, but I have time for it only on weekends. Can I? If so, can we talk in details in IRC about this issue, e.g which place is correct to start, which thins must be done first etc.
(In reply to Arthur Stolyar [:nekr] from comment #13) > Hi, > > I can take this bug, but I have time for it only on weekends. Can I? If so, > can we talk in details in IRC about this issue, e.g which place is correct > to start, which thins must be done first etc. There are two pieces to fixing this bug: 1. Extending the Remote Debugging Protocol to let the client express to the server that it wishes to pause on new source introduction. We should do this the same way we handle pausing on exceptions, for example.  https://wiki.mozilla.org/Remote_Debugging_Protocol Protocol changes required: * When the client attaches to the ThreadActor, the "attach" packet has various options; we should add a "pauseOnNewSource" option. I don't think this actually requires any code changes, because we just update the server's options based on whatever properties we get from the packet, but you can double check (and familiarize yourself with the code flow). Relevant code for the client: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#1285 http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#550 Relevant code for the server: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#671 I guess you will need to set a default (false) for the pauseOnNewSource option, which you can do here: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#485 http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#1490 * The "reconfigure" packet allows us to change the ThreadActor's options at arbitrary times; it should have a "pauseOnNewSource" option. Again, I don't think you actually need to do anything here, but double check and familiarize yourself with the code flow. Relevant code for the client: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#1557 Relevant code for the server: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#732 * "resume" packets also let us specify the options with which we wish to continue JS execution; again, there should be support here for the "pauseOnNewSource" option. Relevant code for the client: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#1517 Also do something similar to this for pauseOnNewSource: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#1631 Relevant code for the server: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#1012 * "paused" notifications from the server should support the "newSource" reason. Basically, `packet.why.type` should be "newSource" instead of "breakpoint" or "debuggerStatement" when we pause because of a new source introduction. This shouldn't need any client changes, just server changes to actually do the pause and notify the client. We have a function that is called every time a new source is introduced. Right now all it does is send a notification to the client so the frontend can keep its source list synchronized and up to date. *After* this notification, you would want to test if the pauseOnNewSource option is set, and if it is then pause the JS execution. The reason we do it after the notification to the client of the new source is so that the frontend can populate its source list with this new source and add breakpoints and all of that while we are paused and before that new source's top level script has been executed. http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2262 http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#756 http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#553 This will require "xpcshell" (unit-test-y, no DOM just JS + some parts of the platform) tests. They should go in `toolkit/devtools/server/test/unit/*`. You can crib from other tests in that directory. Note that new test files need to be added to the xpcshell.ini file in that directory or else the build system will miss them. https://wiki.mozilla.org/DevTools/Hacking#xpcshell_Tests https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests This is a pretty good test to use as an example / skeleton / crib from when writing your tests: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_blackboxing-07.js Things we should test: * We can pause on "normal" new sources. Use Cu.evalInSandbox to introduce new sources: https://developer.mozilla.org/en-US/docs/Components.utils.evalInSandbox * If we introduce multiple new sources, we pause once (and only once) for each new source. * We can pause on new source mapped sources. You can see how to make a source mapped JS file that introduces multiple sources here: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_sourcemaps-01.js 2. Once the protocol, client, and server support pausing on new sources, it doesn't take much to expose that option in the frontend UI. Add a "devtools.debugger.pause-on-new-source" pref, so the option is persistent: http://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1363 Hook it up here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#2326 Add a menu option (this is the cog/sprocket drop down in the top right of the debugger pane): http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul#91 http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul#214 http://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/debugger.dtd#98 And then from there it's mostly just a little glue for passing the option to the appropriate client method at the appropriate time: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#401 http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#341 http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#347 http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#294 If you have any questions, feel free to drop a comment here and I can reply when I see it or you could ping me on IRC and if I happen to be at the keyboard I can talk with you directly. Happy hacking!
Assignee: nobody → nekr.fabula
Status: NEW → ASSIGNED
That looks like a lot, but not really because it is a lot of work, but mostly because I was very thorough :) Again, don't hesitate to ask questions!
Thank you Nick, for detailed answer! At right now I have only one question: can we have separate bugs for [protocol, server, client] and for UI? That way looks for me more structural (of course I 'll take and second bug). Or it's overkill?
That's fine, or you can do it as two different patches in this bug that we can land separately. Whichever is easier for you.
Oh, missed this option. Yeah, separate patches sounds good :)
I found the summary of this bug somewhat misleading, so I tried to make it more clear. I hope you don't mind.
Summary: Break on new source → Setting breakpoints on sources before they are evaluated
I don't think this is a strict dependent of bug 905700. Let's keep it as a follow-up instead.
No longer blocks: dbg-source
Summary: Setting breakpoints on sources before they are evaluated → Break on introduction of new source
:nekr, are you still actively working on this?
Unfortunately I do not have time for it anymore :( I did not started working on this bug actually since I have one other to do, but then got problems with build and got myself out of free time. So you probably should reassign it to someone other, sorry.
Assignee: nekr.fabula → nobody
Status: ASSIGNED → NEW
Part of new Event panel milestone 3 or 4.
Severity: normal → enhancement
Harald why is this part of Event Panel planning?
Assignee: dhyey35 → nobody
OS: macOS → All
Priority: P5 → P2
Hardware: x86 → All
Summary: Break on introduction of new source → Break on first statement in new script
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/9aa1f63be136 Implement break on first script statement. r=davidwalsh
You need to log in before you can comment on or make changes to this bug.