Last Comment Bug 737808 - Separate breakpoints and scripts handling in the client from the server
: Separate breakpoints and scripts handling in the client from the server
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 15
Assigned To: Panos Astithas [:past]
:
: James Long (:jlongster)
Mentors:
Depends on:
Blocks: 754251
  Show dependency treegraph
 
Reported: 2012-03-21 04:42 PDT by Panos Astithas [:past]
Modified: 2012-06-03 13:47 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (5.20 KB, patch)
2012-05-03 10:53 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v2 (6.28 KB, patch)
2012-05-04 10:13 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v3 (17.08 KB, patch)
2012-05-15 07:55 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v4 (21.25 KB, patch)
2012-06-01 07:59 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Splinter Review
Patch v5 (21.27 KB, patch)
2012-06-02 10:45 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-03-21 04:42:39 PDT
Currently setting a breakpoint in the debugger UI requires having the complete list of scripts in the page available, but that precludes setting a breakpoint in scripts that execute right after the page loads, like inline scripts. What we should do instead, is to store breakpoints in the debugger UI and use the onNewScripts handler to set any available breakpoints to scripts as they appear.
Comment 1 Panos Astithas [:past] 2012-05-02 10:06:24 PDT
In bug 740551, Jim lays out a plan to have an onNewGlobals hook for reacting to the appearance of new globals, so that stored breakpoints can be set right away. We could implement the content debugging part in this bug, using the existing DOMWindowCreated mechanism, along with onNewScripts. Interesting test cases include setting breakpoints inside large eval scripts, inside dynamically-created iframes, and even scripts injected via script tags. Also, requirejs and similar script loaders should be tested as well.
Comment 2 Panos Astithas [:past] 2012-05-03 07:21:42 PDT
Consider this use case:

var a = eval("var b = 1;" +   // line: 1
             "var c = b++;" + // line: 2
             "var d = 6;");   // line: 3
console.log(a);               // line: 4

Setting a breakpoint in line 2, will cause it to slide to line 4, because there are no bytecodes in line 2 in the outer script. What we want to do, however, is store both the initial location and the actual location and when onNewScript is triggered, try to set the breakpoint again in the original location (which will now succeed) and remove the breakpoint from the previous location.

This means that the original protocol response will have an actualLocation property to describe what actually happened, even though that won't be the eventual location of the breakpoint. It also means that we should WONTFIX bug 737803. Jim, does this sound reasonable or am I missing something?

Another way to address this without repercussions to the protocol spec, is to have a UI option to pause when new globals & scripts are found. This way the user will be given the opportunity to set breakpoints in the scripts present in the debuggees, without having to keep a breakpoint store in the server. This would arguably result in an clumsier UX, but could cover the use case of breakpoints in an injected script from a dynamically-created script tag.

As a middle ground we could even do the first option for dynamically created scripts and the second for dynamically created globals.
Comment 3 Panos Astithas [:past] 2012-05-03 10:53:13 PDT
Created attachment 620773 [details] [diff] [review]
WIP

This should work, but I need to write some tests first to be sure.
Comment 4 Jim Blandy :jimb 2012-05-03 21:19:04 PDT
(In reply to Panos Astithas [:past] from comment #2)
> var a = eval("var b = 1;" +   // line: 1
>              "var c = b++;" + // line: 2
>              "var d = 6;");   // line: 3

Is it common for code in the wild to use eval this way? I really don't know, but it seems obscure. I certainly would like to be able to set a breakpoint on line 2 and have it hit before the initialization of c, but the way SpiderMonkey attributes the i'th line of code passed to eval to e+i, where e is the location of the eval, seems unlikely to be helpful in the real world. (In fact, as written, because there are no newlines, that'll all be attributed to line 1.)  (Jason really hates this SpiderMonkey behavior, too.)

We should be able to enumerate scripts generated by the call to eval at a particular location. And we should save all the source code we're passed. Then the user could set the breakpoint by finding the actual script (imagine it has newlines):

var b = 1;
var c = b++;
var d = 6;

(perhaps listed under "eval at foo.js:10", or appearing in a context menu on the eval line!) and clicking on the second line directly.

I guess I'm concerned that we might be architecting for a strange, obscure case.
Comment 5 Jim Blandy :jimb 2012-05-03 21:20:45 PDT
However, my extensive use of emotive words in that comment ("obscure"; "unlikely... in the real world"; "hates"; "strange, obscure") suggests that I could well be wrong. Please read with an especially critical eye.
Comment 6 Panos Astithas [:past] 2012-05-04 10:13:03 PDT
Created attachment 621085 [details] [diff] [review]
Patch v2

Fixed the basic use case, but removing a breakpoint that has been set after a reload no longer works, since the frontend is not notified of the new breakpoint actor.
Comment 7 Panos Astithas [:past] 2012-05-04 10:29:34 PDT
(In reply to Jim Blandy :jimb from comment #4)
> Is it common for code in the wild to use eval this way? I really don't know,
[...]
> I guess I'm concerned that we might be architecting for a strange, obscure
> case.

My example is indeed contrived, but I only used it as a means to illustrate a protocol issue with my approach. Let me try to clear it up.

STR:
1) Apply this patch
2) Visit http://astithas.com/test/bp/index.html
3) Set a breakpoint to line 27
4) Reload
5) Page loading is paused in that breakpoint

This is the first use case I'm trying to cater for here, inspecting code that executes on page load. The thing is that after a page load, the breakpoint actors associated with the previous thread are gone, so setting a new breakpoint creates a new breakpoint actor, scoped to the new thread. The problem is that the protocol does not give me a means of notifying the client that a new breakpoint has been set and a new breakpoint actor has been created, so that a new front object can be created to communicate with that.

My rambling in comment 2 was an attempt at side-stepping the issue, but I'm not too fond of it. One better way to fix this that I can see now, is if I could return a list of breakpoints with a newScript notification. These breakpoints would only be present in the reload-the-same-page scenario and only for the purposes of supporting breakpoints in startup code. Alternatively, we would need a client/server breakpoint store synchronization request.
Comment 8 Rob Campbell [:rc] (:robcee) 2012-05-07 06:37:53 PDT
(In reply to Jim Blandy :jimb from comment #4)
> (In reply to Panos Astithas [:past] from comment #2)
> > var a = eval("var b = 1;" +   // line: 1
> >              "var c = b++;" + // line: 2
> >              "var d = 6;");   // line: 3
> 
> Is it common for code in the wild to use eval this way? I really don't know,
> but it seems obscure. I certainly would like to be able to set a breakpoint
> on line 2 and have it hit before the initialization of c, but the way
> SpiderMonkey attributes the i'th line of code passed to eval to e+i, where e
> is the location of the eval, seems unlikely to be helpful in the real world.
> (In fact, as written, because there are no newlines, that'll all be
> attributed to line 1.)  (Jason really hates this SpiderMonkey behavior, too.)

it does occur, unfortunately. A cursory search on github shows a number of occurrences of eval(something) in various shapes and sizes. My hunch is that a number of libraries use this as a means of constructing code.

https://github.com/search?langOverride=&language=JavaScript&q=%22eval%28%22&repo=&start_value=4&type=Code

(aside: I wish google code search was still a thing)

I know Firebug went to a lot of trouble to figure out how to retain and display eval chunks. I also know that the list of eval'd code snippets that could appear on any given site were often non-zero.

I think we need a mechanism for this despite various levels of distaste.
Comment 9 Jim Blandy :jimb 2012-05-11 10:36:50 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> it does occur, unfortunately. A cursory search on github shows a number of
> occurrences of eval(something) in various shapes and sizes. My hunch is that
> a number of libraries use this as a means of constructing code.

Oh --- I understand that eval is widely used for this kind of thing; I hope it didn't sound like I thought 'eval' was icky. (Well, eval is slightly icky, but it's the JavaScript Way, and we want to do the best we can with that.) And our debugger definitely needs to be capturing the source code passed to eval, to present it directly.

I had a hard time believing, *specifically* that people pass eval strings containing newlines placed just so to make the lines of the string's *contents* line up with the lines of the *source code* that produces the string. Which is what's necessary for Panos's example to work: he wanted setting a breakpoint on line 2 of the script containing the eval to set a breakpoint on line 2 of the script the eval executes.

But Panos explained that he had larger issues in mind, so this is kind of a red herring. I just don't want anyone thinking I'm, like, "Screw eval." I'm not.
Comment 10 Jim Blandy :jimb 2012-05-11 10:39:33 PDT
... and I should say, the "carefully aligned content and source newlines" kludge does work at present, because SpiderMonkey gives the code eval runs the same url as the script containing the call to eval, and the starting line of the call to eval.

But it's a total kludge. We should just retain the source code.
Comment 11 Rob Campbell [:rc] (:robcee) 2012-05-13 10:25:58 PDT
ok, I get you. And no, we have no guarantees that things are going to line up perfectly there. Probably won't, and I have no idea if people are munging strings together like that in practice. If you can imagine it, somewhere, someone is probably doing it on the internet.
Comment 12 Panos Astithas [:past] 2012-05-15 07:55:30 PDT
Created attachment 624042 [details] [diff] [review]
Patch v3

I have implemented my protocol change described in comment 7. A 'newScript' notification returns any breakpoints set by the server like this:

{
  "from": "conn0.context43",
  "type": "newScript",
  "url": "http://harthur.github.com/wwcode/wwcode.js",
  "startLine": 1,
  "breakpoints": [
    {
      "url": "http://harthur.github.com/wwcode/wwcode.js",
      "line": 37,
      "actor": "conn0.breakpoint45"
    }
  ]
}

Using that information, the client can sync its breakpoint cache with the server's, leading to a working breakpoint removal. I've confirmed that the use cases described in comment 7 and bug 754251 work for both setting and removing breakpoints.

Jim, does that look like something you can consider putting in the spec?
Comment 13 Jim Blandy :jimb 2012-05-28 14:44:09 PDT
Comment on attachment 624042 [details] [diff] [review]
Patch v3

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

An alternative approach would be to allow a single BreakpointActor to be inserted as the handler in all the appropriate scripts.

The ThreadActor would need to maintain a table of breakpoints as is done in this patch. When new scripts are loaded, the ThreadActor would look up breakpoints set at that URL and insert any applicable breakpoints in the new script.

Each BreakpointActor would need to maintain a list of the scripts into which it had been inserted, so that its 'delete' method could call each script's clearBreakpoint method.

The protocol could remain unchanged: the single breakpoint actor could represent the breakpoint set in all the applicable scripts.

It seems like this approach might be simpler than this patch. Does it seem practical?
Comment 14 Panos Astithas [:past] 2012-05-29 12:57:38 PDT
(In reply to Jim Blandy :jimb from comment #13)
> Comment on attachment 624042 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 624042 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> An alternative approach would be to allow a single BreakpointActor to be
> inserted as the handler in all the appropriate scripts.
> 
> The ThreadActor would need to maintain a table of breakpoints as is done in
> this patch. When new scripts are loaded, the ThreadActor would look up
> breakpoints set at that URL and insert any applicable breakpoints in the new
> script.
> 
> Each BreakpointActor would need to maintain a list of the scripts into which
> it had been inserted, so that its 'delete' method could call each script's
> clearBreakpoint method.
> 
> The protocol could remain unchanged: the single breakpoint actor could
> represent the breakpoint set in all the applicable scripts.
> 
> It seems like this approach might be simpler than this patch. Does it seem
> practical?

I think we can use your approach, even though some minor spec changes will still be required, since these two issues came up:

1) since invoking Debugger.Script.prototype.setBreakpoint can happen long after the server receives the request to set a breakpoint (e.g. after a page reload), the 'actualLocation' information in the response packet can no longer be counted upon (for instance when setting a breakpoint in a comment line, in a script that executes and finishes on page load).
2) all error responses are no longer meaningful to the client ("noScript", "noCodeAtLineColumn"), since they don't guarantee that a script cannot appear after a page reload on which the breakpoint will apply fine.
Comment 15 Jim Blandy :jimb 2012-05-29 13:43:05 PDT
(In reply to Panos Astithas [:past] from comment #14)
> 1) since invoking Debugger.Script.prototype.setBreakpoint can happen long
> after the server receives the request to set a breakpoint (e.g. after a page
> reload), the 'actualLocation' information in the response packet can no
> longer be counted upon (for instance when setting a breakpoint in a comment
> line, in a script that executes and finishes on page load).
> 2) all error responses are no longer meaningful to the client ("noScript",
> "noCodeAtLineColumn"), since they don't guarantee that a script cannot
> appear after a page reload on which the breakpoint will apply fine.

Yes, those are good points.
Comment 16 Panos Astithas [:past] 2012-06-01 07:59:37 PDT
Created attachment 629189 [details] [diff] [review]
Patch v4

This patch works as advertised and follows Jim's proposed plan. It turned out that some more architectural changes were required in the server:

- breakpoints may or may not be associated with scripts
- error responses from the setBreakpoint protocol operation need to return the breakpoint actor for later deletion by the client
- breakpoint actors can lo longer be thread-scoped if they are to be reused across page reloads, so I had to store them in a root-actor-scoped pool
Comment 17 Rob Campbell [:rc] (:robcee) 2012-06-02 08:56:52 PDT
Comment on attachment 629189 [details] [diff] [review]
Patch v4

+    for each (let bp in DebuggerController.Breakpoints.store) {

can use a for .. of there.

in dbg-client.jsm:

+      let type = aRequest.type ? aRequest.type : "";

you could just do aRequest.type || "", but this is also acceptable.

+    let scriptBreakpoints = this._breakpointStore[location.url];
+    scriptBreakpoints[line] = {

I guess we're not going to worry about multiple breakpoints per line this go-around.

+    let existing = this._breakpointStore[aScript.url];
+    if (existing) {
+      for each (let bp in existing) {
+        this._setBreakpoint(bp);
+      }

and can use for .. of here as well.

looks good!
Comment 18 Panos Astithas [:past] 2012-06-02 10:45:53 PDT
Created attachment 629496 [details] [diff] [review]
Patch v5

(In reply to Rob Campbell [:rc] (:robcee) from comment #17)
> Comment on attachment 629189 [details] [diff] [review]
> Patch v4
> 
> +    for each (let bp in DebuggerController.Breakpoints.store) {
> 
> can use a for .. of there.

Done.

> in dbg-client.jsm:
> 
> +      let type = aRequest.type ? aRequest.type : "";
> 
> you could just do aRequest.type || "", but this is also acceptable.

Done.

> +    let scriptBreakpoints = this._breakpointStore[location.url];
> +    scriptBreakpoints[line] = {
> 
> I guess we're not going to worry about multiple breakpoints per line this
> go-around.

Right, we have bug 676602 for this.

> +    let existing = this._breakpointStore[aScript.url];
> +    if (existing) {
> +      for each (let bp in existing) {
> +        this._setBreakpoint(bp);
> +      }
> 
> and can use for .. of here as well.

Done.
Comment 19 Panos Astithas [:past] 2012-06-03 06:44:41 PDT
https://hg.mozilla.org/integration/fx-team/rev/7e578ad2c6f9
Comment 20 Rob Campbell [:rc] (:robcee) 2012-06-03 13:47:41 PDT
https://hg.mozilla.org/mozilla-central/rev/7e578ad2c6f9

Note You need to log in before you can comment on or make changes to this bug.