Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Description • 11 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 1 • 11 years ago

Alternatively, we could just extend the existing clientEvaluate packet with a breakpoints property. I think this is probably best.

	Jim Blandy :jimb
	Comment 2 • 11 years ago

(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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 3 • 11 years ago

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).

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 4 • 11 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 5 • 11 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 6 • 11 years ago

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.

	Jim Blandy :jimb
	Comment 7 • 11 years ago

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?

	Jim Blandy :jimb
	Comment 8 • 11 years ago

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.

	Jim Blandy :jimb
	Comment 9 • 11 years ago

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...

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 10 • 11 years ago

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

	Eddy Bruel [:ejpbruel]
	Comment 11 • 10 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 12 • 10 years ago

(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.

	Arthur Stolyar [:nekr]
	Comment 13 • 10 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 14 • 10 years ago

(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[0] 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.

   [0] 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!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 15 • 10 years ago

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!

	Arthur Stolyar [:nekr]
	Comment 16 • 10 years ago

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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 17 • 10 years ago

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.

	Arthur Stolyar [:nekr]
	Comment 18 • 10 years ago

Oh, missed this option. Yeah, separate patches sounds good :)

	Eddy Bruel [:ejpbruel]
	Comment 19 • 10 years ago

I found the summary of this bug somewhat misleading, so I tried to make it more clear. I hope you don't mind.

	James Long (:jlongster)
	Comment 20 • 10 years ago

I don't think this is a strict dependent of bug 905700. Let's keep it as a follow-up instead.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 21 • 9 years ago

:nekr, are you still actively working on this?

	Arthur Stolyar [:nekr]
	Comment 22 • 9 years ago

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.

	:Harald Kirschner :digitarald
	Comment 23 • 6 years ago

Part of new Event panel milestone 3 or 4.

	Jason Laster [:jlast]
	Comment 24 • 5 years ago

Harald why is this part of Event Panel planning?

	Pulsebot
	Comment 34 • 4 years ago

Pushed by dwalsh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9aa1f63be136
Implement break on first script statement. r=davidwalsh