148181 - F10, F11 malfunctioning on DOM setTimeout() function

Status: RESOLVED INCOMPLETE

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

Description

[Phil Schwartau]

Set a breakpoint on the first line of test1():

<SCRIPT>
var firstRun = true;
test1();

function test1()
{
  if (firstRun)  //<<<<<<<<<<<<<<<<<<<<< set breakpoint here
  {
    alert('In if-block of test1');
    firstRun = false;
    test1();
  }
  else
  {
    alert('In else-block of test1');
  }
}
</SCRIPT>


STEPS TO REPRODUCE
1. Reload the script
2. The breakpoint gets hit
3. Continue via F5, F10, or F11
4. The first alert comes up
5. The breakpoint gets hit again
6. The second alert comes up


So far, so good. But now include a setTimeout() in the script:


<SCRIPT>
var firstRun = true;
test1();

function test1()
{
  if (firstRun)  //<<<<<<<<<<<<<<<<<<<<< set breakpoint here
  {
    alert('In if-block of test1');
    firstRun = false;
    setTimeout('test1()', 500);
  }
  else
  {
    alert('In else-block of test1');
  }
}
</SCRIPT>


In this script, it matters how you proceed after the breakpoint is first hit.
If you choose F5 ("Run"), the breakpoint gets hit again, as above, and there
is no bug. But if you continue via F10 ("Step Over") or F11 ("Step Into"),
the breakpoint is NOT hit again !!! The second alertbox comes up WITHOUT
the breakpoint being hit. Very confusing for people trying to step through
this type of code...We expect to hit the breakpoint a second time, whether
we choose to step through the code via F5, F10, or F11.

Comment 1

[Phil Schwartau]

I should say, I'm using Mozilla trunk binary 20020520xx on WinNT.
Will attach the two testcases below -

Comment 2

[Phil Schwartau]

Attachment: Testcase #1 - call test1() from itself directly; no bug

Comment 3

[Phil Schwartau]

Attachment: Testcase #2 - call test1() from itself via setTimeout()

Comment 4

[Robert Ginda]

this is probably due to the timer firing while you're stepping through the first
run.  I'm not sure there is anything I can do about this.  You told the time to
fire in 1/2 second.  If you single step 1/2 second later, and the timer gets a
chance to fire *while* you're executing the single step, it'll execute.

Comment 5

[timeless]

jst: would it be possible to disable scripts for a window and still force 
scripts to execute for that same window? (I don't think you can just disable 
timeouts.)

Comment 6

[Johnny Stenback (:jst)]

You can disable script temporarily and execute timeouts that fired while script
was disabled upon enabling again (controlled by the aFireTimeouts argument to
nsIScriptContext::SetScriptsEnabled()). But that's about as much control as
you've got here.

Comment 7

[timeless]

jst: thanks, that should be good enough for venkman. it should probably do that
at the same time that it disables the window (i'm pretty certain it has to take
effort to prevent me from clicking on the debugged window).

for other purposes i plan to expose the internals of timers (to my own consumer
and venkman).

Comment 8

[James Ross]

I've not a not-that-pretty solution working here, I believe.

It tweaks jsdIContext to have a |timeoutsEnabled| attribute, and makes
jsdContext remember this value for use in SetScriptsEnabled. It then disables
and imediately re-enables scripts if you set |timeoutsEnabled| to |true| when
scripts are enabled, so the timeouts fire then.

The Venkman code then disables timeouts when it disables scripts and the target
window. It, however, doesn't re-enable them when it re-enables the others, as
that is precicely why it's broken at the moment (this happens inside the JSD
execution hook, which is why I believe it doesn't get called - re-entrancy). So
instead, it (irronically, really) sets a timeout to clear |timeoutsEnabled|
right after it finishes what it's doing.

Basically, it just means the timeouts are run outside of the JSD execution hook,
instead of inside it.

Patch upcoming.

Comment 9

[James Ross]

Attachment: Add timeoutsEnabled to jsdIContext and use

Disclaimer: I've never touched an IDL file before, so I don't really know what
I should change besides adding the attribute. I've also not gone near JSD
before. :)

Also, there lies within the problem of Venkman running on a Mozilla without the
IDL change...

Comment 10

[James Ross]

Attachment: Updated |timeoutsEnabled| patch

This update to the patch adds some checking in Venkman's code to only poke the
|timeoutsEnabled| attribute if it exists, and flushes out the docs for
|timeoutsEnabled| in the IDL, hopefully explaining how it affects timeout
execution.

Comment 11

[James Ross]

Comment on attachment 154030 [details] [diff] [review]
Updated |timeoutsEnabled| patch

Not sure who should review/sr the JSD bits.

Comment 12

[Alex Vincent [:WeirdAl]]

This patch is over six months old.  Is it still current?

Comment 13

[James Ross]

Sure. Whether it works, I'm not sure, but it doesn't look like it will have run
in to any particular new issues.

Comment 14

[Colin Blake]

Can we get this reviewed and checked in? I'd really like to be able to disable 
timeouts too. It looks like all the hard work has already been done.

Comment 15

[Colin Blake]

I mailed James Ross last week but haven't had a reply yet. I'm willing to check
this in if we can get some reviews done. Is rginda still the right person for this?

Comment 16

[James Ross]

I really felt it unnessessary to reply. Bugzilla is where you comment. And *I*
am not the person to mail. I will check the patch in when it has appropriate
reviews.

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 154030 [details] [diff] [review]
Updated |timeoutsEnabled| patch

sr=shaver

Comment 18

[Colin Blake]

Can this get checked in now or are more reviews and/or approvals required?

Comment 19

[:Gijs (he/him)]

As far as I can tell, the patch no longer works :-(. If I try second testcase, and step through until after the setTimeout call, then hit Continue, the breakpoint doesn't get hit, but the timeout doesn't fire either (so the second alert never pops up).

Comment 20

[Till Schneidereit [:till]]

Venkman is long gone, as is the debugger protocol it used to interact with the JS engine.