Closed Bug 603045 Opened 14 years ago Closed 14 years ago

JM: don't optimize away JSFRAME_HAS_RVAL check in debug mode

Categories

(Core :: JavaScript Engine, defect)

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: luke, Unassigned)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

fix
2.43 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review 
mxr says that JS_SetFrameReturnValue (in jsdbgapi.cpp) is not used.  Even if it was used, we have a method-jit optimization (http://mxr.mozilla.org/mozilla-central/source/js/src/methodjit/Compiler.cpp#1795) that would ignore it in some cases.

In related news, nsJSEnvironment calls JSStackFrame::setReturnValue (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1106).  This may be ignored for the same reason as above.  Does anyone know if this code is actually exercised and whether it could be removed?
That jsenvironment code could be exercised by firebug or venkman.  Whether it is, I don't know...
Drive-by, may file separately but if it gets fixed here, even better.

Just noticed the DOMOperationCallback has an inner-block fp shadowing the outer one, which is iterated once from the top frame down, to get filename and lineno (starting at line 1048). This seems like left-over code -- the outer fp should be used instead, if non-null.

Nit: else after return heading into the code in question:

1096     return JS_TRUE;
1097   }
1098   else if ((buttonPressed == 2) && debugPossible) {
1099     // Debug the script

/be
I don't believe Firebug using any of this. Sadly, as far as I know the button on the slow-script dialog is not operable, clicking does nothing. We've never been able to figure out what jsContext is, let alone the even more obscure nsJSContext.
(In reply to comment #3)
> I don't believe Firebug using any of this.

This code originated with bug 341764.

> Sadly, as far as I know the button
> on the slow-script dialog is not operable, clicking does nothing. We've never
> been able to figure out what jsContext is, let alone the even more obscure
> nsJSContext.

It's not that mysterious. But does the code work? Cc'ing gijs.

/be
Luke, aren't these APIs still good to go with the interpreter, and current plan is for debuggers to keep the methodjit (and tracejit) at bay?

/be
(In reply to comment #4)
> (In reply to comment #3)
> > I don't believe Firebug using any of this.
> 
> This code originated with bug 341764.
> 
> > Sadly, as far as I know the button
> > on the slow-script dialog is not operable, clicking does nothing. We've never
> > been able to figure out what jsContext is, let alone the even more obscure
> > nsJSContext.
> 
> It's not that mysterious. But does the code work? Cc'ing gijs.
> 
> /be

From what I recall it uses the debugger; keyword instruction to break into any attached JS debugger. 

It does seem to work - somewhat? Trivial testcase:

0. Open Fx4bwhatever w/ Vnk 0.9.88.1
1. Create a data URI and/or / simple html testpage with:

<body onload="foo()"><script>
function foo() {
var counter = 1;
while (counter > 0) {
counter++;
}
}</script></body>

After a few seconds, the dialog will pop up. Clicking "debug script" breaks on one of the lines of the while loop. Changing counter to -10; then stops it from hanging your browser.

However, if instead you try stepping, that just goes back to hanging. Really unsure why that is, may be a bug in Venkman, may be JIT, or something else (you and Luke seem better people to comment on that than I am, TBH).

(fwiw, I couldn't get a dialog like this to show up if there is DOM manipulation going on inside the loop, which I guess is an issue with the slow script dialog logic in general?)

I don't know why this doesn't work in Firebug. I suspect that having the same window (presumably without a nested event loop?) trying to break into code like this may not be happy-making. That or FB may not have the same hooks running -- the button will only appear, IIRC, if the code detects "someone is listening" to JSD.

Hope that helps!
(In reply to comment #5)
> Luke, aren't these APIs still good to go with the interpreter, and current plan
> is for debuggers to keep the methodjit (and tracejit) at bay?

I general, yes: before the debugger does anything, a global 'debug mode' must be set, which causes us to be slightly more conservative in our codegen.  The particular optimization mentioned in comment 0 forgot about ad hoc return-value-setting, and would be easy enough to fix, I was just hoping that perhaps the fix wasn't necessary.

On a side note, if we want this 'debug script' button to work well with JM, we will need to already have debug mode set, before any code is running.  Rob, do you know if this will be the case with your recent work?
(In reply to comment #5)
> 
> On a side note, if we want this 'debug script' button to work well with JM, we
> will need to already have debug mode set, before any code is running.  Rob, do
> you know if this will be the case with your recent work?

That is the case.
According to 
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#985
the Debug button is not shown unless jsd.isOn is true, and that value can't be true unless jsd was active when the dialog was popuped up.
Cool -- so I think this bug is missummarized if not invalid. We should keep these APIs for now, right?

/be
(In reply to comment #10)
The former.

We should also have nsJSEnvironment use JS_SetFrameReturnValue instead of touching our privatest privates.
Summary: remove JS_SetFrameReturnValue → JM: don't optimize away JS_HAS_RVAL check in debug mode
Summary: JM: don't optimize away JS_HAS_RVAL check in debug mode → JM: don't optimize away JSFRAME_HAS_RVAL check in debug mode
Attached patch fix — — Splinter Review 

        Attachment #488086 -
        Flags: review?(bhackett1024)

        Attachment #488086 -
        Flags: review?(bhackett1024) → review+

    
    

    
  
http://hg.mozilla.org/tracemonkey/rev/af75274bc041
Whiteboard: fixed-in-tracemonkey

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/af75274bc041
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: