111352 - Breakpoints cannot be set for some valid JavaScript lines in switch statement

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect, P1)

Description

[Peter Wilson]

From Bugzilla Helper:
User-Agent: Mozilla/4.75 [en] (Windows NT 5.0; U)
BuildID:    2001101117

See attached jpeg image - particularly line 63 in the source window.
The lines in the case statement are shown as unavailable for break points. The 
debugger steps over these lines in single step mode.
The code shown actually executes. If I insert an alert at line 63, the alert is 
executed but is also not available for setting a breakpoint.

Reproducible: Always
Steps to Reproduce:
1.run debugger on javascript code with switch statement.

Comment 1

[Peter Wilson]

Attachment: view of debugger for Bug#111352

Comment 2

[Peter Wilson]

Build 2001112009 has the problem mostly solved.

1. The lines in question may now have breakpoints set and the debugger does step 
through them.

2. The "=" sign is still missing for these lines.

Comment 3

[Robert Ginda]

So the screenshot looks the same, but you can set breakpoints now?  Strange.

Comment 4

[Peter Wilson]

Build 2001201109. I spoke in haste. The contents of "case label:" statements
seem to be generally unreachable for setting/removing breakpoints. However, the 
debugger does consistently single step through the these statements.

Comment 5

[Robert Ginda]

This seems to be fixed, probably since the pretty print landing which changed
the way we determine if a line is executable.  Can anyone confirm or deny this?

Comment 6

[Robert Ginda]

Pete, does this still not work for you?  If so, would you please provide a
sample JS file which shows the un-breakable case statements?

Comment 7

[Peter Wilson]

After getting the Debugger running again I finid that the source window does not 
load when a source file or function are selected. Unable to set breakpoints to 
test this.

Comment 8

[Peter Wilson]

OK Solved: You now need a double click to open the source - it used to be a 
single click. I am still unable to set breakpoints between the case's of the 
buildPattern function (see attachment).

Comment 9

[Peter Wilson]

Attachment: Image of debugger source window showing non-reachable statements

Comment 10

[Peter Wilson]

Attachment: Javascript source file containing the buildPattern function

Comment 11

[Robert Ginda]

Ah, yes, I see the same problem here.  Can you confirm that the last two cases
(START: and default:) are breakable in the expected places?

Comment 12

[Peter Wilson]

Confirmed: START and default cases are breakable at all lines.

Comment 13

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 14

[Robert Ginda]

Attachment: image comparing variable case label to consts

This seems to be related to the fact that the case labels refer to variables
instead of constants.  Switch blocks with const case labels are fine, once you
introduce a case with a lookup, things get out of sync.

Comment 15

[Robert Ginda]

Attachment: jsapi testcase showing the bogus source notes

This test case compiles two simple functions with switch statements and
compares the resulting source notes.  Here are the results...

function bar () {
    switch (x) {
	case 1:
	    return true;
	case 2:
	    return false;
    }
}

Source notes:
  0:	 3 [   3] switch   length 15
  3:	14 [  11] xdelta  
  4:	14 [   0] newline 
  5:	14 [   0] newline 
  6:	16 [   2] newline 
  7:	16 [   0] newline 


function foo () {
    switch (x) {
	case bogus:
	    return true;
	case 2:
	    return false;
    }
}

Source notes:
  0:	 3 [   3] switch   length 20 first case offset 4
  3:	 4 [   1] newline 
  4:	 7 [   3] pcdelta  offset 6
  6:	10 [   3] setline  lineno 4
  8:	13 [   3] pcdelta  offset 0
 10:	19 [   6] setline  lineno 2
 12:	19 [   0] newline 
 13:	21 [   2] newline 
 14:	21 [   0] newline

Comment 16

[Robert Ginda]

brendan/ shaver, do these source notes look correct to you guys?

Comment 17

[Steve Chapel]

Fixing summary so this bug can be more easily found, nominating for Mozilla 1.2.

Comment 18

[Brendan Eich [:brendan]]

I see this disassembly with annotations for the second case:

main:
00000:  name "x"
00003:  condswitch
00004:  name "bogus"
00007:  case 19 (12)
00010:  uint16 2
00013:  case 21 (8)
00016:  default 23 (7)
00019:  true
00020:  return
00021:  false
00022:  return

Source notes:
  0:     0 [   0] newline
  1:     3 [   3] switch   length 20 first case offset 4
  4:     4 [   1] newline
  5:     7 [   3] pcdelta  offset 6
  7:    10 [   3] setline  lineno 5
  9:    13 [   3] pcdelta  offset 0
 11:    19 [   6] setline  lineno 3
 13:    19 [   0] newline
 14:    21 [   2] newline
 15:    21 [   0] newline

It looks from the line numbers in comment #15 like you started from line 0, or
something.  Anyway, these notes are all wrong.  When CONDSWITCH went in, no one
tested the line-number notes.  I'll patch this bug today.

/be

Comment 19

[Robert Ginda]

over to brendan

Comment 20

[Brendan Eich [:brendan]]

Attachment: proposed fix

I hope shaver's still up.

Rob, can you test?

/be

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 121099 [details] [diff] [review]
proposed fix

The problem was in the parser.	Hmm, I should have used more context in the
diff.  Here are the lines leading up to those shown in the patch:

	    MUST_MATCH_TOKEN(TOK_COLON, JSMSG_COLON_AFTER_CASE);

	    pn4 = NewParseNode(cx, &CURRENT_TOKEN(ts), PN_LIST, tc);
	    if (!pn4)
		return NULL;
	    pn4->pn_type = TOK_LC;
	    PN_INIT_LIST(pn4);
	    while ((tt = js_PeekToken(cx, ts)) != TOK_RC &&
		    tt != TOK_CASE && tt != TOK_DEFAULT) {

and here's the patched source after that point:

		if (tt == TOK_ERROR)
		    return NULL;
		pn5 = Statement(cx, ts, tc);
		if (!pn5)
		    return NULL;
		pn4->pn_pos.end = pn5->pn_pos.end;
		PN_APPEND(pn4, pn5);
	    }

	    /* Fix the PN_LIST so it doesn't begin at the TOK_COLON. */
	    if (pn4->pn_head)
		pn4->pn_pos.begin = pn4->pn_head->pn_pos.begin;
	    pn3->pn_pos.end = pn4->pn_pos.end;
	    pn3->pn_right = pn4;
	}

So pn4 is NewParseNode'd with the current token being the colon after the case
label.	That's usually the line before the first line of statements labeled by
that case.  We really want this list, which will be passed to js_EmitTree, to
have as its beginning line number and column offset the beginning position of
the first element in the statement list under pn4.

Clear as mud?

/be

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 121099 [details] [diff] [review]
proposed fix

Amazingly, I actually understand that.	I wonder what sort of important
learning is being excluded from my brain by leftover parsenode details.

r=shaver

Comment 23

[Brendan Eich [:brendan]]

Thanks, shaver.  Fix checked in.  Rob, can you VERIFY?  Thanks.

/be

Comment 24

[Robert Ginda]

This doesn't fix it for me.  The file displayed in the "image comparing variable
case label to consts" attachment is still marked exactly the same as that
screenshot.

Comment 25

[Robert Ginda]

Attachment: HTML testcase

Start venkman, load this file in navigator, and view the file in venkman.  The
switch blocks should have ticks in the margin at the same places.

Comment 26

[Brendan Eich [:brendan]]

Trying to get this for 1.6final.

/be

Comment 27

[Brendan Eich [:brendan]]

Happy new year!

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: fix

js_LineNumberToPC must cope with out-of-line-order code due to JSOP_CONDSWITCH,
by preferring the pc for the nearest lineno above target, if it can't match
target exactly.

/be

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 172767 [details] [diff] [review]
fix

Looks good, though I'd prefer at least a comment if not a symbolic define for
0x1000000 to indicate that it's just a really big number, and not a
specifically-magic value.

r=shaver

Comment 30

[Brendan Eich [:brendan]]

#define SN_LINE_LIMIT   (SN_3BYTE_OFFSET_FLAG << 16)

I added this just before js_LineNumberToPC, and used it instead of 0x1000000
(which was a factor of 2 too large).

/be

Comment 31

[Brendan Eich [:brendan]]

*** Bug 313922 has been marked as a duplicate of this bug. ***

Comment 32

[Bob Clary [:bc] (inactive)]

I don't know why I blocked js16 with this bug. attachment 172767 [details] [diff] [review] is already in 1.5 release.