Last Comment Bug 851836 - breakpoints[aLocation.line] is undefined after bug 820012
: breakpoints[aLocation.line] is undefined after bug 820012
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 23
Assigned To: Eddy Bruel [:ejpbruel]
:
:
Mentors:
Depends on:
Blocks: 820012
  Show dependency treegraph
 
Reported: 2013-03-16 13:51 PDT by Panos Astithas [:past]
Modified: 2013-05-23 01:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Fix + comments (4.63 KB, patch)
2013-04-04 15:20 PDT, Eddy Bruel [:ejpbruel]
past: review-
Details | Diff | Splinter Review
Fix + comments (version 2) (4.66 KB, patch)
2013-04-22 08:44 PDT, Eddy Bruel [:ejpbruel]
past: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2013-03-16 13:51:02 PDT
STR:

1) Visit http://harthur.github.com/wwcode/
2) Set a breakpoint at wwcode.js:36
3) Reload and wait for the breakpoint to hit
4) Resume

The terminal says:

TypeError: breakpoints[aLocation.line] is undefined

And this is the stack trace:

TA__setBreakpoint@chrome://global/content/devtools/dbg-script-actors.js:517
TA__addScript@chrome://global/content/devtools/dbg-script-actors.js:1156
TA__discoverScriptsAndSources@chrome://global/content/devtools/dbg-script-actors.js:604
TA_onSources@chrome://global/content/devtools/dbg-script-actors.js:609
DSC_onPacket@chrome://global/content/devtools/dbg-server.js:653
LDT_send/<.run@chrome://global/content/devtools/dbg-transport.js:224
Comment 1 Panos Astithas [:past] 2013-04-03 00:22:33 PDT
Eddy, any progress here? You mentioned in your blog that you were working on this.
Comment 2 Eddy Bruel [:ejpbruel] 2013-04-03 03:28:50 PDT
(In reply to Panos Astithas [:past] from comment #1)
> Eddy, any progress here? You mentioned in your blog that you were working on
> this.

Oops. This one dropped from my radar somehow. Sorry about that. I'll look into this tomorrow at the latest.
Comment 3 Eddy Bruel [:ejpbruel] 2013-04-04 15:20:41 PDT
Created attachment 733578 [details] [diff] [review]
Fix + comments

Looks like I was on the right track with my observation with my assertion that breakpoints[aLocation] and aLocation refer to the same object, but the problem was quite a bit subtler than that.

The problem is in the following three lines:
    breakpoints[actualLocation.line] = breakpoints[aLocation.line];  
    breakpoints[actualLocation.line].line = actualLocation.line;           
    delete breakpoints[aLocation.line];

The second line overwrites aLocation.line, after which the third line deletes the breakpoint actor at the new location, not the old one, as intended.

The fix simply moves the third line above the other two. I've tested it on my machine and the problem seems to have gone away, but by all means double check this. I've also added some direly needed comments while I was at it.
Comment 4 Eddy Bruel [:ejpbruel] 2013-04-04 15:22:56 PDT
Oh, for what it's worth: the crash you observed was caused by a different issue: the debugger API did not properly handle uncaught exceptions thrown from the onNewScript hook. Bug 858170 contains a fix for that problem too :-)
Comment 5 Panos Astithas [:past] 2013-04-08 03:24:36 PDT
Comment on attachment 733578 [details] [diff] [review]
Fix + comments

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

I see that the error no longer appears after resuming, but I observed that the breakpoint is no longer hit in subsequent reloads. Also, xpcshell tests fail with this patch. Try running 'mach xpcshell-test toolkit/devtools' to see them.

The oldLine temporary variable that was there before bug 820012 was because I had hit the same bug as well in the past. Maybe reusing that will be better?

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +500,5 @@
>     */
>    _setBreakpoint: function TA__setBreakpoint(aLocation) {
>      let breakpoints = this._breakpointStore[aLocation.url];
>  
> +    // Get or create the breakpoint actor for the given location

If I was suffering from OCD I might have pointed out that comments in this file end with a period, but fortunately I'm not that weird.
Comment 6 Panos Astithas [:past] 2013-04-19 01:42:23 PDT
Eddy, did you make any progress here?
Comment 7 Eddy Bruel [:ejpbruel] 2013-04-19 04:53:50 PDT
(In reply to Panos Astithas [:past] from comment #6)
> Eddy, did you make any progress here?

I was occupied with prototyping symbols in SpiderMonkey for a while. This bug is still on my radar though. I'll put up a new patch beginning next week.
Comment 8 Eddy Bruel [:ejpbruel] 2013-04-22 08:44:26 PDT
Created attachment 740308 [details] [diff] [review]
Fix + comments (version 2)

The problem was simple. I accidentally moved the line "delete breakpoints[aLocation.line];" above the line "breakpoints[actualLocation.line] = breakpoints[aLocation.line];". That obviously doesn't work.

It should be just below that line, and just above the line "breakpoints[actualLocation.line].line = actualLocation.line;". I've updated the patch, and added another warning comment for good measure.

Setting a breakpoint on an empty line now seems to work properly on my machine. However, I noticed that sometimes the breakpoint is also moved down to the next line in the debugger UI, but sometimes its not (the breakpoint still gets hit on the correct line after a refresh in that case, though). Is this behavior expected?

I also ran the xpcshell tests on my machine this time, and didn't run into any more failing tests.
Comment 9 Panos Astithas [:past] 2013-04-23 02:48:11 PDT
Comment on attachment 740308 [details] [diff] [review]
Fix + comments (version 2)

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

Looks good!

(In reply to Eddy Bruel [:ejpbruel] from comment #8)
> Setting a breakpoint on an empty line now seems to work properly on my
> machine. However, I noticed that sometimes the breakpoint is also moved down
> to the next line in the debugger UI, but sometimes its not (the breakpoint
> still gets hit on the correct line after a refresh in that case, though). Is
> this behavior expected?

Indeed, we want to let the user know that the line doesn't contain any bytecode, but we can't do that for scripts that are scavenged before the debugger is opened (bug 799070).

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +713,5 @@
>      }
>  
> +    /**
> +     * If we get here, no line matching the given line was found, so just
> +     * epically.

"just fail epically"
Comment 10 Panos Astithas [:past] 2013-04-23 02:58:29 PDT
https://hg.mozilla.org/integration/fx-team/rev/7e4501974b5b
Comment 11 Panos Astithas [:past] 2013-04-23 03:00:19 PDT
(In reply to Panos Astithas [:past] from comment #9)
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +713,5 @@
> >      }
> >  
> > +    /**
> > +     * If we get here, no line matching the given line was found, so just
> > +     * epically.
> 
> "just fail epically"

And of course I forgot to fix it before pushing...
*sigh*
Comment 12 Tim Taubert [:ttaubert] 2013-04-23 10:03:43 PDT
https://hg.mozilla.org/mozilla-central/rev/7e4501974b5b
Comment 13 Panos Astithas [:past] 2013-04-24 02:21:30 PDT
Eddy, will you request aurora approval now that this has landed, or do you want me to?
Comment 14 Panos Astithas [:past] 2013-05-15 00:59:19 PDT
Comment on attachment 740308 [details] [diff] [review]
Fix + comments (version 2)

This missed 22 when it was in Aurora, so requesting approval for beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 820012
User impact if declined: breakpoints in the debugger will sometimes not work
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk, doesn't affect regular users, only developers, long bake time in m-c
String or IDL/UUID changes made by this patch: none
Comment 15 Alex Keybl [:akeybl] 2013-05-15 13:03:53 PDT
Comment on attachment 740308 [details] [diff] [review]
Fix + comments (version 2)

Early enough in the cycle to take this.
Comment 16 Panos Astithas [:past] 2013-05-16 01:35:43 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/b0fc9358f5b3
Comment 17 Simona B [:simonab ] 2013-05-23 01:44:03 PDT
Verified as fixed on Firefox 22 beta 2: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:22.0) Gecko/20100101 Firefox/22.0 (20130521223249)

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