Closed Bug 351021 Opened 18 years ago Closed 18 years ago

cannot step out of breakpoints

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jacob, Assigned: crowderbt)

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 2 obsolete files)

testcase
546 bytes, text/html
Details
Same patch, better comment
1.76 KB, patch
brendan
: review+
mtschrep
: approval1.8.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2

if i set a breakpoint in either venkman or firebug, if it is triggered i cannot step out of it.

Reproducible: Always

Steps to Reproduce:
1.  set a breakpoint in a debugger
2.  do something to hit that breakpoint
3.  try to step over or step in


Actual Results:  
it stays on that line

Expected Results:  
it should go to the next line

if i remove the breakpoint, then step, then add it again, debugging can continue.

this problem appeared between 2.0a3 and 2.0b1, and persists in 2.0b2.

it happens with both firebug and venkman, even with one of them disabled.

i can reproduce it in a 20 line html+js test case by adding the breakpoint on line 9:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
  <head><title>test debug!</title></head>
  <body>
    <input id="inpute" value="Click me!" type="button">
    <script language="javascript">
      var input = document.getElementById ('inpute');
      function observer() {
          var s = ''; // add breakpoint here
          for (var i = 0; i < 5; i++) {
              s += i + ' ';
          }
          document.title = s;
      }
      input.addEventListener('click', observer, false);
    </script>
  </body>
</html>
Problem in threaded JS interpreter?  Can you or someone else watching this bug please test on Windows, which lacks computed goto support, so uses the old switch-in-loop interpreter?

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file testcase — 
works for me in bonecho/winxp from today but not on linux.
Brian, can you take a look?  The bug is in the JS_THREADED_INTERP code, used on Mac and Linux.  It tries to use a second jump table to interrupt to the debugger on every cycle, but obviously something's wrong.

/be
Assignee: general → crowder
Flags: blocking1.8.1?
Plusing - we have to have venkman/firebug working properly for FF2 - they are core features of FF as a dev platform.
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 236495 [details]
testcase

Changing MIME type for convenience
Attachment #236495 - Attachment mime type: text/plain → text/html
The way this code is written seems to depend on interruptHandler sometimes being 0, otherwise jumpTable never == normalJumpTable.  I don't see any code that ever sets interruptHandler to 0 in jsinterp.c

What I seem to be seeing as a result of this, is that if the debugger is active I am always returning from JS_TrapHandler, and then immediately calling it again
Attached patch fix (obsolete) — — Splinter Review 
This patch seems to fix the bug.  I'm deeply embarrassed to say I don't officially know why.
Attached patch real patch (obsolete) — — Splinter Review 
Woops, the last patch was an earlier attempt.  This is shorter and sweeter and still not understood by me.  I'll remove the ASSERT here if it is felt to be extraneous.
Attachment #238916 - Attachment is obsolete: true
Comment on attachment 238919 [details] [diff] [review]
real patch

Dunno if you're available today or not, Brendan, but if you could review this patch (and educate me about why it works), that would be great.
Attachment #238919 - Flags: review?(brendan)
Comment on attachment 238919 [details] [diff] [review]
real patch

>     /*
>      * This is a loop, but it does not look like a loop.  The loop-closing
>      * jump is distributed throughout interruptJumpTable, and comes back to
>      * the interrupt label.  The dispatch on op is through normalJumpTable.
>      * The trick is LOAD_INTERRUPT_HANDLER setting jumpTable appropriately.
>      */
>+    op = (JSOp) *pc;

This has to happen before the interruptHandler is called, since that may be implemented by a debugger that plants a breakpoint (JSOP_TRAP).  That must be what is happening with Venkman -- can you confirm in gdb?

I would comment this anti-dependency briefly in a second paragraph in the major comment shown above.

Thanks for finding this!  Going for 1.8.1 approval.

/be
Attachment #238919 - Flags: review?(brendan)
Attachment #238919 - Flags: review+
Attachment #238919 - Flags: approval1.8.1?
Here's the same patch with a better comment.  I can confirm from what I have seen in gdb today that what Brendan suggests here is true.  JSOP_TRAP depends on being able to set "op" for the next iteration of the hidden loop here, and we were previously smashing that op inside the loop.  op needs to be initialized for the first iteration, but subsequent iterations of the loop will have op correctly initialized by DO_NEXT_OP()
Attachment #238919 - Attachment is obsolete: true
Attachment #238919 - Flags: approval1.8.1?
Comment on attachment 238924 [details] [diff] [review]
Same patch, better comment

Thanks, I'm landing this on the trunk now.

/be
Attachment #238924 - Flags: review+
Attachment #238924 - Flags: approval1.8.1?
Fixed on the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.290; previous revision: 3.289
done

/be
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 238924 [details] [diff] [review]
Same patch, better comment

a=schrep please land on 181 asap.  Thanks for the quick work Brian and Brendan.
Attachment #238924 - Flags: approval1.8.1? → approval1.8.1+
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: