Closed Bug 793214 Opened 12 years ago Closed 12 years ago

A breakpoint doesn't stop JS Execution

Categories

(DevTools :: Debugger, defect, P2)

x86
Windows Vista
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 19

People

(Reporter: Honza, Assigned: past)

Details

Attachments

(2 files, 1 obsolete file)

Attached file a test file
STR

1) Load http://janodvarko.cz/firebug/jsd2/tests/scopes.html
2) Open built-in debugger
3) Create a breakpoint on line 18
4) Click the button on the page
5) JS execution doesn't stop

If you create a breakpoint at line 17, it works.

The same test-file is attached (in case the online file would change or disappear)

Honza
Maybe I'm missing something, but I don't see that load function being called or bound anywhere.
Wait, no, i missed the onclick=load in the html. Don't mind me :)
From reading the code, my guess is that the breakpoint is set in function _func1, which is never called. Line 18 is present in 3 different scripts: the outer script that corresponds to the script tag, the function load, and the function _func1. The server sets the breakpoint on the innermost script, which in this case is _func1.
(In reply to Panos Astithas [:past] from comment #3)
> From reading the code, my guess is that the breakpoint is set in function
> _func1, which is never called. Line 18 is present in 3 different scripts:
> the outer script that corresponds to the script tag, the function load, and
> the function _func1. The server sets the breakpoint on the innermost script,
> which in this case is _func1.

Maybe if a breakpoint is added in a script that Does Nothing or Is Never Called, we could defer it in the parent script. I think the 'Is Never Called' option would be more accurate, but is there a way to determine this (apart from parsing the source)?
Or, maybe this is something that should be tackled with at a lower level.
(In reply to Victor Porof [:vp] from comment #4)
> (In reply to Panos Astithas [:past] from comment #3)
> > From reading the code, my guess is that the breakpoint is set in function
> > _func1, which is never called. Line 18 is present in 3 different scripts:
> > the outer script that corresponds to the script tag, the function load, and
> > the function _func1. The server sets the breakpoint on the innermost script,
> > which in this case is _func1.
> 
> Maybe if a breakpoint is added in a script that Does Nothing or Is Never
> Called, we could defer it in the parent script. I think the 'Is Never
> Called' option would be more accurate, but is there a way to determine this
> (apart from parsing the source)?
> Or, maybe this is something that should be tackled with at a lower level.

I don't think you can infer functions that are never called in a dynamic language that supports eval. Maybe the more intuitive thing to do in this case (where the inner script is contained in one line) is to set the breakpoints in both inner and parent scripts, but this is a broader issue that will be better served by supporting breakpoints in columns.

I still need to instrument the code better and verify my hypothesis first, though.
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
Status: ASSIGNED → NEW
Attached patch Patch v1 (obsolete) — Splinter Review
This is a fix that makes sure we set a breakpoint in every script that contains the specified line and has JS bytecode in it.

I took the opportunity to add a few helper methods that I keep around to aid me in debugging the server, so that others can benefit as well. If you don't like the idea, I can continue to paste them when I need to.
Attachment #676664 - Flags: review?(rcampbell)
Comment on attachment 676664 [details] [diff] [review]
Patch v1

   _setBreakpoint: function TA__setBreakpoint(aLocation) {
     // Fetch the list of scripts in that url.
     let scripts = this._scripts[aLocation.url];
     // Fetch the specified script in that list.
     let script = null;
-    for (let i = aLocation.line; i >= 0; i--) {
+    for (let i = 0; i <= aLocation.line; i++) {
       // Stop when the first script that contains this location is found.
       if (scripts[i]) {
         // If that first script does not contain the line specified, it's no
         // good.
         if (i + scripts[i].lineCount < aLocation.line) {
           continue;
         }

that logic looks really funny to me. You reversed the counting order, that's fine (thought not clear why it's necessary).

The if (i + scripts[i].lineCount < aLocation.line) is what I find strange though:

Adding a counter to a script's line count and comparing that to aLocation.line seems kind of random.

e.g.,

0 + 5 // five lines
1 + 16
2 + 3
3 + 18 ...

I guess I just don't see how aLocation.line relates to those values.

...

+    if (!codeFound) {
+      // No code at that line in any script, skipping forward in the innermost
+      // script.

You're not really skipping forward, more like returning to it. Why do we need to treat the innermost script any differently from the others? Do we need inner? Don't the same rules apply to it?

This _setBreakpoint is quite a method with a lot of mystery to it. Strange stuff.

-   * Get the innermost script that contains this line, by looking through child
-   * scripts of the supplied script.
+   * A generator function for iterating over the scripts that contain the
+   * specified line, by looking through child scripts of the supplied script.
    *

might want to specify that this is a recursive generator function just to make sure readers are terrified.

so if I'm reading this correctly, a Debugger.Script has 1..n child scripts that are also Debugger.Scripts.

+  _getContainers: function TA__getContainers(aScript, aLine) {
     let children = aScript.getChildScripts();
     if (children.length > 0) {
       for (let i = 0; i < children.length; i++) {
         let child = children[i];

is children an array? You could use forEach and get rid of the counter.

Should the two Debugger extensions (.Script.toString and .Frame.prototype.line) live here or on the JSDBG2 API itself? We don't want to encourage rampant extension, but I wouldn't object to a follow-up.

clearing review request pending greater knowledge.
Attachment #676664 - Flags: review?(rcampbell)
Attached patch Patch v2Splinter Review
These are all reasonable questions that prove this method needs even more comments. I started by adding a big explanatory comment in the declaration of the _scripts cache, which tries to explain how it is supposed to work.

(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> Comment on attachment 676664 [details] [diff] [review]
> Patch v1
> 
>    _setBreakpoint: function TA__setBreakpoint(aLocation) {
>      // Fetch the list of scripts in that url.
>      let scripts = this._scripts[aLocation.url];
>      // Fetch the specified script in that list.
>      let script = null;
> -    for (let i = aLocation.line; i >= 0; i--) {
> +    for (let i = 0; i <= aLocation.line; i++) {
>        // Stop when the first script that contains this location is found.
>        if (scripts[i]) {
>          // If that first script does not contain the line specified, it's no
>          // good.
>          if (i + scripts[i].lineCount < aLocation.line) {
>            continue;
>          }
> 
> that logic looks really funny to me. You reversed the counting order, that's
> fine (thought not clear why it's necessary).

The reason I changed it is because the previous way was an optimization that no longer works. I don't think a comment about the new order is appropriate (since it's the obvious way to count), but I wish I had commented the logic behind the reverse counting before.

Before this patch we were only interested in the innermost script that contained the line where the breakpoint was set. In order to find that, I started counting from that line backwards, since the sparse array almost guaranteed that the first script found would be the one that contained the line in question, and in any case, for a file with a huge number of scripts counting backwards from that line number made the worst case much faster than always starting from the start and counting forward. I'll explain the "almost guaranteed" bit below.

In order to fix this bug however, we can no longer use that optimization, because now we want to include all the scripts that contain that line (if they have bytecode in it). So we just start from the beginning, essentially looking for the outermost script.

> The if (i + scripts[i].lineCount < aLocation.line) is what I find strange
> though:
> 
> Adding a counter to a script's line count and comparing that to
> aLocation.line seems kind of random.
> 
> e.g.,
> 
> 0 + 5 // five lines
> 1 + 16
> 2 + 3
> 3 + 18 ...
> 
> I guess I just don't see how aLocation.line relates to those values.

I hope the comment I made at the point of the script cache declaration will clarify things a bit. Like I said above, the goal in this first part of the _setBreakpoint function is to find the outermost script that contains the line we are going to set a breakpoint at.

If you are setting a breakpoint in, say, line 20 that happens to be the second line of a 10-line function, then the script we are looking for will have a startLine of 19 and a lineCount of 10. If, however, line 20 is only contained in the parent <script> tag, which also contains a 10-line function that starts at line 3, then we should skip this function's script from our loop, because 3+10<20 and this script cannot be our outermost script.

Note that i === scripts[i].startLine essentially, since scripts are stored in the sparse array at the index that is equal to their startLine. Note that |i| will not take arbitrary values, because of the if (scripts[i]) guard in the previous line.

I have added more comments around that code, hopefully clearing up some of the ambiguity.

> +    if (!codeFound) {
> +      // No code at that line in any script, skipping forward in the
> innermost
> +      // script.
> 
> You're not really skipping forward, more like returning to it. Why do we
> need to treat the innermost script any differently from the others? Do we
> need inner? Don't the same rules apply to it?

This is the only case where we want to be smart, when the user sets a breakpoint in an empty line, or a line with a comment. In that case we know that execution can never stop in that exact line, but we can be proactive and move the breakpoint to the next line that has bytecode. Our new algorithm from this bug ("set breakpoints in all the scripts that contain this line and have bytecode in it") obviously results in the empty set (no bytecode in empty/comment lines), so we just need to do our magic in the inner script.

> This _setBreakpoint is quite a method with a lot of mystery to it. Strange
> stuff.

Indeed, lots of corner cases and we haven't even added column support yet!

> -   * Get the innermost script that contains this line, by looking through
> child
> -   * scripts of the supplied script.
> +   * A generator function for iterating over the scripts that contain the
> +   * specified line, by looking through child scripts of the supplied
> script.
>     *
> 
> might want to specify that this is a recursive generator function just to
> make sure readers are terrified.

Done.

> so if I'm reading this correctly, a Debugger.Script has 1..n child scripts
> that are also Debugger.Scripts.

Yes, for example an inline <script> tag has the top-level functions declared in it as its children.

> +  _getContainers: function TA__getContainers(aScript, aLine) {
>      let children = aScript.getChildScripts();
>      if (children.length > 0) {
>        for (let i = 0; i < children.length; i++) {
>          let child = children[i];
> 
> is children an array? You could use forEach and get rid of the counter.

for loops are faster than forEach (I think mainly due to the closure overhead), so I opted for raw speed since this is backend code:

http://www.symphonious.net/2010/10/09/javascript-performance-for-vs-foreach/

If you feel that clarity should trump speed in this case however, I can change it to forEach.

> Should the two Debugger extensions (.Script.toString and
> .Frame.prototype.line) live here or on the JSDBG2 API itself? We don't want
> to encourage rampant extension, but I wouldn't object to a follow-up.

I mostly got these from jorendb, so Jim and Jason apparently didn't feel like adding them to JSDBG2. I used to have frame.line in the initial stepping patches, but monkey-patching Debugger.Frame caused leaks for some reason, so I had to take it out. I can't reproduce that any longer though, which is why I put them back.

They are not currently used anywhere (except when I dumpn(script)), but that could change if we leave them in. If you prefer me adding them in a followup, I can do that, too.
Attachment #676664 - Attachment is obsolete: true
Attachment #677434 - Flags: review?(rcampbell)
(In reply to Panos Astithas [:past] from comment #8)
> Yes, for example an inline <script> tag has the top-level functions declared
> in it as its children.
> 
> > +  _getContainers: function TA__getContainers(aScript, aLine) {
> >      let children = aScript.getChildScripts();
> >      if (children.length > 0) {
> >        for (let i = 0; i < children.length; i++) {
> >          let child = children[i];
> > 
> > is children an array? You could use forEach and get rid of the counter.
> 
> for loops are faster than forEach (I think mainly due to the closure
> overhead), so I opted for raw speed since this is backend code:

[ignorable mostly useless comment] Isn't that iterable via for..of?
(In reply to Victor Porof [:vp] from comment #9)
> (In reply to Panos Astithas [:past] from comment #8)
> > Yes, for example an inline <script> tag has the top-level functions declared
> > in it as its children.
> > 
> > > +  _getContainers: function TA__getContainers(aScript, aLine) {
> > >      let children = aScript.getChildScripts();
> > >      if (children.length > 0) {
> > >        for (let i = 0; i < children.length; i++) {
> > >          let child = children[i];
> > > 
> > > is children an array? You could use forEach and get rid of the counter.
> > 
> > for loops are faster than forEach (I think mainly due to the closure
> > overhead), so I opted for raw speed since this is backend code:
> 
> [ignorable mostly useless comment] Isn't that iterable via for..of?

Sure, I could do that.
Comment on attachment 677434 [details] [diff] [review]
Patch v2

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

We had some chatter in IRC about how we'd like to see this setBreakpoints function be improved. It boils down to replacing the sparse array with an object and just using object property iteration rather than the somewhat convoluted set of loops here.

In the meantime, this fixes the problem and should probably land.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +35,5 @@
>        this.global = aHooks.browser.contentWindow.wrappedJSObject;
>      }
>    }
> +
> +  /**

doesn't really need to be a javadoc comment, but no biggie.

@@ +50,5 @@
> +   * of the scripts that contain a particular line number. For example, if a
> +   * cache holds two scripts with the URL http://foo.com/ starting at lines 4
> +   * and 10, then the corresponding cache will be:
> +   * this._scripts: {
> +   *   'http://foo.com/': [,,,,[Debugger.Script],,,,,,[Debugger.Script]]

ok, this is weird. It's going to be really sparse in a lot of cases and potentially, really empty. We could have arrays that are thousands of entries with only one Debugger.Script in it.

Thanks for documenting it though. I'm also not going to r- this but I'm curious what this buys us over an actual map of line numbers to scripts.
Attachment #677434 - Flags: review?(rcampbell) → review+
Whiteboard: [land-in-fx-team]
Rebased and landed:
https://hg.mozilla.org/integration/fx-team/rev/a0c6d66368a3
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a0c6d66368a3
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 19
Try run for 7a317f298423 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7a317f298423
Results (out of 264 total builds):
    success: 237
    warnings: 27
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rcampbell@mozilla.com-7a317f298423
Try run for 7a317f298423 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7a317f298423
Results (out of 265 total builds):
    exception: 1
    success: 237
    warnings: 27
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rcampbell@mozilla.com-7a317f298423
Try run for 47ca41a155dd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=47ca41a155dd
Results (out of 264 total builds):
    success: 251
    warnings: 10
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rcampbell@mozilla.com-47ca41a155dd
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: