Last Comment Bug 754251 - Can't set breakpoint in Script Debugger
: Can't set breakpoint in Script Debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 16
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on: 737808 761223
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 06:17 PDT by Heather Arthur [:harth]
Modified: 2012-06-18 07:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
WIP (6.41 KB, patch)
2012-06-07 11:00 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v2 (4.49 KB, patch)
2012-06-08 08:24 PDT, Panos Astithas [:past]
rcampbell: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Heather Arthur [:harth] 2012-05-11 06:17:21 PDT
STR:
1) visit http://harthur.github.com/wwcode/
2) open Web Developer > Script Debugger
3) select "wwcode.js" to debug
4) try to set a breakpoint at line 35 by clicking to the left of the line number

Expected:
Dot appears to left of line number and breakpoint set

Actual:
No dot and no breakpoint set.
Comment 1 Panos Astithas [:past] 2012-05-11 06:23:01 PDT
This will be fixed by bug 737808.

The problem is that when you first open the debugger, the script that corresponds to that piece of code is already GC'd. When you reload the page, we attach early and get a chance to get to the script. With the fix in that bug, the debugger frontend will allow you to set a breakpoint there, even though the debuggee won't stop until the page is reloaded.
Comment 2 Panos Astithas [:past] 2012-06-07 11:00:14 PDT
Created attachment 631044 [details] [diff] [review]
WIP

This patch fixes almost all problems for me, but I need to keep working on it a bit more.
Comment 3 Panos Astithas [:past] 2012-06-08 08:24:43 PDT
Created attachment 631404 [details] [diff] [review]
Patch v2

So this patch fixes the test case, modulo one annoying issue: setting the breakpoint at line 35, makes it slide to line 36 after a reload (not visually, due to bug 737803). If however we reload the page once more, it slides again to line 37. From what I can see after instrumenting the code, SpiderMonkey no longer sees any bytecode in line 36 and the debugger server correctly slides the breakpoint forward. I don't know whether this is a normal optimization for forEach or not. jorendorff may be able to shed some light on this.

The patch fixes a couple of issues with the current code:
(a) updating the cached breakpoint when skipping forward
(b) iterating backwards over the breakpoint cache when setting breakpoints, in order to avoid messing with any cache updating due to (a)

I tried to write a test for breakpoints on startup, but apparently I can't get to the debuggee early enough on reload *for scripts served locally* to set the breakpoint (the script is already GC'd). I wonder if reinstating the workaround from bug 697040 would help with such a test.
Comment 4 Panos Astithas [:past] 2012-06-08 08:27:13 PDT
Jason, see if the SpiderMonkey issue I mention in comment 3 makes any sense to you.
Comment 5 Rob Campbell [:rc] (:robcee) 2012-06-11 12:02:56 PDT
Comment on attachment 631404 [details] [diff] [review]
Patch v2

ok
Comment 6 Panos Astithas [:past] 2012-06-12 00:46:37 PDT
https://hg.mozilla.org/integration/fx-team/rev/c823ffa89a33
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-06-13 06:39:21 PDT
https://hg.mozilla.org/mozilla-central/rev/c823ffa89a33
Comment 8 Panos Astithas [:past] 2012-06-14 03:01:04 PDT
Comment on attachment 631404 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: breakpoints will not work in some cases in the debugger
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): I suppose in theory breakpoint handling could become more buggy, but this claim is unfounded 
String or UUID changes made by this patch: none
Comment 9 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-15 16:00:40 PDT
Comment on attachment 631404 [details] [diff] [review]
Patch v2

[Triage Comment]
Approving, there will be lots of time to watch this for any potential fallout.

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