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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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

Comment 3

7 years ago
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

Comment 6

7 years ago
(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!
(Reporter)

Comment 7

7 years ago
(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?

Comment 8

7 years ago
(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.

Comment 9

7 years ago
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
(Reporter)

Comment 11

7 years ago
(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
(Reporter)

Comment 12

7 years ago
Created attachment 488086 [details] [diff] [review]
fix
Attachment #488086 - Flags: review?(bhackett1024)
Attachment #488086 - Flags: review?(bhackett1024) → review+
(Reporter)

Comment 13

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

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/af75274bc041
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.